Closed
Bug 1470516
Opened 6 years ago
Closed 6 years ago
webRequest.getSecurityInfo should not return localised values
Categories
(WebExtensions :: Request Handling, defect, P1)
WebExtensions
Request Handling
Tracking
(firefox62+ fixed, firefox63 fixed)
RESOLVED
FIXED
mozilla63
People
(Reporter: Will, Assigned: mixedpuppy)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
rpl
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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
Assignee | ||
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
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]?
Assignee | ||
Comment 3•6 years ago
|
||
(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•6 years ago
|
||
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
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 8•6 years ago
|
||
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?
status-firefox62:
--- → affected
status-firefox63:
--- → affected
tracking-firefox62:
--- → +
Flags: needinfo?(francesco.lodolo)
Comment 10•6 years ago
|
||
bugherder |
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 11•6 years ago
|
||
(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)
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 13•6 years ago
|
||
(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+
Comment 15•6 years ago
|
||
bugherder uplift |
Comment 16•6 years ago
|
||
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)
Assignee | ||
Comment 17•6 years ago
|
||
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.
Description
•