Closed Bug 1158296 Opened 5 years ago Closed 5 years ago

ECDSA public key export doesn't work

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed
firefox-esr38 --- fixed

People

(Reporter: mt, Assigned: mt)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

This is simple, there's a one line change.  Then a test.

Oh, and the ECDSA tests don't run at all.

Patch coming soon.
Attached file MozReview Request: bz://1158296/mt (obsolete) —
/r/7627 - Bug 1158296 - Allow ECDSA key export in WebCrypto, r=rbarnes

Pull down this commit:

hg pull -r 7c0c28a62e86e869593a48529f63211c162173e7 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8597396 - Flags: review?(rlb)
For richard, same as what is on RB.
Attachment #8597410 - Flags: review?(rlb)
Comment on attachment 8597410 [details] [diff] [review]
0001-Bug-1158296-Allow-ECDSA-key-export-in-WebCrypto-r-rb.patch

Review of attachment 8597410 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/test/test_WebCrypto_ECDSA.html
@@ +151,5 @@
>  
>  <div id="content">
> +        <div id="head">
> +                <b>Web</b>Crypto<br>
> +        </div>

Oh, whoops, some tabs were converted to spaces here.
Comment on attachment 8597410 [details] [diff] [review]
0001-Bug-1158296-Allow-ECDSA-key-export-in-WebCrypto-r-rb.patch

Review of attachment 8597410 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/crypto/test/test_WebCrypto_ECDSA.html
@@ +72,5 @@
> +  "ECDSA key generation with public key export",
> +  function() {
> +    var that = this;
> +    var alg = { name: "ECDSA", namedCurve: "P-256", hash: "SHA-256" };
> +    var msg = Uint8Array.from([1]);

It would be more consistent with the rest of these tests to use "new Uint8Array([1])"
Attachment #8597410 - Flags: review?(rlb) → review+
Attachment #8597396 - Flags: review?(rlb)
Attached patch bug1158296.patchSplinter Review
r=rbarnes; minor fixes
Attachment #8597396 - Attachment is obsolete: true
Attachment #8597410 - Attachment is obsolete: true
Attachment #8597417 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1835de92a1bd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8597417 [details] [diff] [review]
bug1158296.patch

Approval Request Comment
[Feature/regressing bug #]:
998471

[User impact if declined]:
Without this patch, the use of ECDSA key export is impossible, making some applications of webcrypto impossible. NOTE: If not uplifted to 38 prior to ESR branching, then certain planned Firefox Hello functions (e.g., Instant Messaging) may not work in 38 ESR.

[Describe test coverage new/current, TreeHerder]:
This patch also enables testing for the associated code, which has been running in CI on m-c for two weeks. This code has also been tested since landing as part of ECDH key export.

[Risks and why]: 
Low. This one-line change enables a key export function. The code being enabled is the same as is currently used for ECDH key export, so the code paths have been well tested.

[String/UUID change made/needed]:
None
Attachment #8597417 - Flags: approval-mozilla-beta?
Attachment #8597417 - Flags: approval-mozilla-aurora?
Looks like this has been broken since Firefox 32. We missed the cutoff date for the 38 ESR release.
Comment on attachment 8597417 [details] [diff] [review]
bug1158296.patch

Approved for uplift to beta (39) as this looks like a low risk fix for a regression and has some tests included.
Attachment #8597417 - Flags: approval-mozilla-beta?
Attachment #8597417 - Flags: approval-mozilla-beta+
Attachment #8597417 - Flags: approval-mozilla-aurora?
Comment on attachment 8597417 [details] [diff] [review]
bug1158296.patch

[Approval Request Comment]

> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

One of the features planned for Loop/Hello is a secure instant messaging feature. The design under consideration will require the use of ECDSA key export using WebCrypto. Without an uplift of this minor patch, ESR users will be unable to take advantage of this Hello feature until the release of 45 ESR.

To be clear, while this bug pertains to WebCrypto, it is not a security bug. It is a missing feature.

> User impact if declined: 

See above.

> Fix Landed on Version:

40, with uplift to 39 planned.

> Risk to taking this patch (and alternatives if risky): 

Low. This one-line change enables a key export function. The code being enabled is the same as is currently used for ECDH key export, so the code paths have been well tested.

> String or UUID changes made by this patch: 

None.
Attachment #8597417 - Flags: approval-mozilla-esr38?
Do we want this for 38.0.5? If so, please nominate for mozilla-release.
Flags: needinfo?(martin.thomson)
Comment on attachment 8597417 [details] [diff] [review]
bug1158296.patch

I don't want to uplift anything.  Riding the trains was good enough for me.

See Adam's Approval Request Comment in Comment 12
Flags: needinfo?(martin.thomson)
Attachment #8597417 - Flags: approval-mozilla-release?
Comment 12 makes me wonder why this was nominated for esr38.
Oh, because I can't read. Carry on :P
Comment on attachment 8597417 [details] [diff] [review]
bug1158296.patch

Hello being disabled in ESR 38. I don't see the point of taking this patch.
Please n-i if I am wrong here.
Attachment #8597417 - Flags: approval-mozilla-release?
Attachment #8597417 - Flags: approval-mozilla-release-
Attachment #8597417 - Flags: approval-mozilla-esr38?
Attachment #8597417 - Flags: approval-mozilla-esr38-
(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> Comment on attachment 8597417 [details] [diff] [review]
> bug1158296.patch
> 
> Hello being disabled in ESR 38. I don't see the point of taking this patch.
> Please n-i if I am wrong here.

There's some confusion here.

There are two components to Hello: the built-in ("desktop") client, and the "standalone" client. When you create a room using the desktop client and send it to another user, that link is usable in any WebRTC-compliant web browser (Firefox, Chrome, or Opera), as it loads the "standalone" client on that side of the call.

What we've disabled in ESR is the desktop client: users cannot create or manage rooms in 38ESR. They *can*, however, click on a Hello link that was sent to them by a (non-ESR) Firefox user, which loads the standalone client in content; they can then use that client to communicate with the Hello user. However, without uplifting this specific bug, ESR users won't be able to use the instant messaging feature that we plan to roll out some time in the next year or so.

To be clear: Chrome users will be able to use this feature; Opera users will be able to use this feature; it's going to be only Firefox ESR users who are going to run into problems unless we uplift this fix to ESR.
Flags: needinfo?(sledru)
OK. Sorry for asking this question but this bug has been reported and fixed 3 weeks ago.
Why did you requested the uplift sooner?
Flags: needinfo?(sledru)
(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> OK. Sorry for asking this question but this bug has been reported and fixed
> 3 weeks ago.
> Why did you requested the uplift sooner?

I requested uplift to Beta on the 8th (literally *during* the conversation in which I found out about the problem), not realizing that the ESR go-to-build had already happened prior to that date. As soon as I found out from Liz that we'd missed the ESR branch cutoff, I nominated for ESR.
Flags: needinfo?(sledru)
Flags: needinfo?(sledru)
Comment on attachment 8597417 [details] [diff] [review]
bug1158296.patch

Ok, taking it as we are going to communicate on Hello screen sharing in 38.0.5
Attachment #8597417 - Flags: approval-mozilla-release-
Attachment #8597417 - Flags: approval-mozilla-release+
Attachment #8597417 - Flags: approval-mozilla-esr38-
Attachment #8597417 - Flags: approval-mozilla-esr38+
Setting qe-verify- as this fix is already covered by automated tests.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.