webRequest.getSecurityInfo should not return localised values

RESOLVED FIXED in Firefox 62

Status

defect
P1
normal
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: Will, Assigned: mixedpuppy)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62+ fixed, firefox63 fixed)

Details

Attachments

(1 attachment)

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
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)
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]?
(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.
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
Assignee: nobody → mixedpuppy
Keywords: dev-doc-needed
Priority: -- → P1
Blocks: 1322748
Blocks: 1472766
See Also: → 1472766
Comment on attachment 8989238 [details]
Bug 1470516 - remove or fix localized values in securityInfo,

https://reviewboard.mozilla.org/r/254288/#review261126
Attachment #8989238 - Flags: review?(lgreco) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce1755e40f5b
remove or fix localized values in securityInfo, r=rpl
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
Attachment #8989238 - Flags: approval-mozilla-beta?
Checking in with the l10n team since uplifting this means string changes on beta. 
flod, what do you think?
Flags: needinfo?(francesco.lodolo)
https://hg.mozilla.org/mozilla-central/rev/ce1755e40f5b
Status: UNCONFIRMED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(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.
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy) → qe-verify-
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.
Attachment #8989238 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1474626
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?
Flags: needinfo?(mixedpuppy)
It looks like docs were done after the fix, and uplifted, so I don't think anything needs to happen here.
Flags: needinfo?(mixedpuppy)
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.