1470516 - webRequest.getSecurityInfo should not return localised values

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Will Elwood (:Will)]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180605171542

Steps to reproduce:

See https://bugzilla.mozilla.org/show_bug.cgi?id=1322748#c107 and the following comment tagged as spam showing localised values for validity and keyUsages. Given that the primary consumer of the webRequest.getSecurityInfo API is addon code, it should be easy to parse the data.


Actual results:

A relevant snippet of the example security info object referred to above:
      "validity": {
        "startGMT": "Donnerstag, 8. Februar 2018, 00:00:00",
        "endGMT": "Donnerstag, 2. Mai 2019, 12:00:00"
      },
      "keyUsages": "unterzeichne,Schlüssel-Verschlüsselung",

Mock addon code failing to make use of the security info object:

Date.parse(cert.validity.startGMT) // NaN
Date.parse(cert.validity.endGMT) // NaN
cert.keyUsages.toLowerCase().split(',').includes('signing') // false


Expected results:

Non-localised example snippet:
      "validity": {
        "startGMT": "08 February 2018, 00:00:00 GMT",
        "endGMT": "02 May 2019, 12:00:00 GMT"
      },
      "keyUsages": "Signing,Key Encipherment",

Mock addon code successfully making use of the security info object:

Date.parse(cert.validity.startGMT) // 1518048000000
Date.parse(cert.validity.endGMT) // 1556798400000
cert.keyUsages.toLowerCase().split(',').includes('signing') // true

Comment 1

[Shane Caraveo (:mixedpuppy)]

The interfaces we're using are indeed documented that they are localized strings.  

For validity we could probably add a timestamp since nsIX509CertValidity has a PRTime for these.  

For keyUsages, I don't see any alternate non-localized access to that.  You would probably have to get the rawDER and use a js library to parse it.

Comment 2

[April King [:April]]

Yeah, having it return a Javascript date object would be great.

As for keyUsages, could we use an enum based on what the X.509 specification lists:

KeyUsage ::= BIT STRING {
  digitalSignature        (0),
  nonRepudiation          (1), -- recent editions of X.509 have
                               -- renamed this bit to contentCommitment
  keyEncipherment         (2),
  dataEncipherment        (3),
  keyAgreement            (4),
  keyCertSign             (5),
  cRLSign                 (6),
  encipherOnly            (7),
  decipherOnly            (8) }


So keyUsages === "Signing, Key Encipherment" would return [0, 2]?

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to April King [:April] from comment #2)

> As for keyUsages, could we use an enum based on what the X.509 specification
> lists:

Is there a xpcom interface to the non-string value for keyUsages?  I didn't see one.  It should be split out to a separate bug to get that part done.

Comment 4

[April King [:April]]

Not that I know of. In the meantime, for people that need to know what the localized strings map to, I believe this is where they arise from:

https://dxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/chrome/pipnss/pipnss.properties#132

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1470516 - remove or fix localized values in securityInfo,

Review commit: https://reviewboard.mozilla.org/r/254288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254288/

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8989238 [details]
Bug 1470516 - remove or fix localized values in securityInfo,

https://reviewboard.mozilla.org/r/254288/#review261126

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce1755e40f5b
remove or fix localized values in securityInfo, r=rpl

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8989238 [details]
Bug 1470516 - remove or fix localized values in securityInfo,

Approval Request Comment
[Feature/Bug causing the regression]: webRequest securityInfo
[User impact if declined]: extensions may be unable to perform some security checks on requests, user impact is dependent on what extension features are supported by this.  securityInfo is a new api, we want to fix this issue prior to release.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: minimal change tested on unit tests
[String changes made/needed]: none

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Checking in with the l10n team since uplifting this means string changes on beta. 
flod, what do you think?

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ce1755e40f5b

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #9)
> Checking in with the l10n team since uplifting this means string changes on
> beta. 
> flod, what do you think?

There are no string changes in this bug. The discussion is about showing localized dates, but no real l10n changes, so it can go ahead.

Comment 12

[marius.santa]

Considering that https://bugzilla.mozilla.org/show_bug.cgi?id=1322748#c107 is part of a bug that has the "qe-verify-" flag set and https://bugzilla.mozilla.org/show_bug.cgi?id=1470516#c8 can we set the "qe-verify-" flag for this issue as well? 
Just making sure since this seems to be a security issue.

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to marius.santa from comment #12)
> Considering that https://bugzilla.mozilla.org/show_bug.cgi?id=1322748#c107
> is part of a bug that has the "qe-verify-" flag set and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1470516#c8 can we set the
> "qe-verify-" flag for this issue as well? 
> Just making sure since this seems to be a security issue.

It's not a security issue, and it has a test added with the change.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8989238 [details]
Bug 1470516 - remove or fix localized values in securityInfo,

Fixes in support of new API, let's uplift for beta 7 so that these extensions will work with 62.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/97d009f65f60

Comment 16

[Irene Smith]

I can't find anywhere in the documentation that it says these are localized strings. If we aren't saying the value is localized, is this just a release notes comment?

Comment 17

[Shane Caraveo (:mixedpuppy)]

It looks like docs were done after the fix, and uplifted, so I don't think anything needs to happen here.