Closed
Bug 1034856
Opened 10 years ago
Closed 10 years ago
Add support for DH to WebCrypto API
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: rbarnes, Assigned: ttaubert)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(6 files, 12 obsolete files)
12.55 KB,
patch
|
rbarnes
:
review+
smaug
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
12.64 KB,
patch
|
rbarnes
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
11.12 KB,
patch
|
rbarnes
:
review+
|
Details | Diff | Splinter Review |
7.84 KB,
patch
|
rbarnes
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
16.20 KB,
patch
|
rbarnes
:
review+
|
Details | Diff | Splinter Review |
13.23 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ttaubert
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Key/bit derivation should probably work but key import is making life hard again. Looks like NSS needs a public key internally or else importing DH keys won't work. The spec doesn't require public keys for pkcs8 import though - computing the public key from the private key and the DH params wouldn't be hard but seems like a little too much work to get a key ID (not sure what that's even used for).
Assignee | ||
Comment 6•10 years ago
|
||
FTR, bug 1041328 introduced a crash test that currently passes "RSA-OAEP" as the algorithm. This needs to be changed to "DH" because actually pass a pkcs8 DH key but that wouldn't crash as long as we don't support "DH". This should be done in the patch here.
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8459338 -
Attachment is obsolete: true
Attachment #8459339 -
Attachment is obsolete: true
Attachment #8459340 -
Attachment is obsolete: true
Attachment #8459341 -
Attachment is obsolete: true
Attachment #8476681 -
Flags: review?(rlb)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8476682 -
Flags: review?(rlb)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•10 years ago
|
||
Test vector corrected.
Attachment #8476682 -
Attachment is obsolete: true
Attachment #8476682 -
Flags: review?(rlb)
Attachment #8476730 -
Flags: review?(rlb)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8476731 -
Flags: review?(rlb)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8477360 -
Flags: review?(rlb)
Assignee | ||
Comment 12•10 years ago
|
||
Oops, small fix.
Attachment #8477360 -
Attachment is obsolete: true
Attachment #8477360 -
Flags: review?(rlb)
Attachment #8477402 -
Flags: review?(rlb)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8477403 -
Flags: review?(rlb)
Assignee | ||
Comment 14•10 years ago
|
||
Test vector cleanup.
Attachment #8477403 -
Attachment is obsolete: true
Attachment #8477403 -
Flags: review?(rlb)
Attachment #8477463 -
Flags: review?(rlb)
Reporter | ||
Comment 15•10 years ago
|
||
Comment on attachment 8476681 [details] [diff] [review] 0001-Bug-1034856-Introduce-DhKeyAlgorithm.patch, v2 Review of attachment 8476681 [details] [diff] [review]: ----------------------------------------------------------------- Just a note: The latest WebCrypto spec has changed KeyAlgorithm from an interface to a dictionary type. https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#key-algorithm-dictionary So there's going to be some refactoring of how KeyAlgorithms work in the patches for bug 1037892. But that's still a little while out from landing, so this can go ahead for now. ::: dom/webidl/SubtleCrypto.webidl @@ +44,5 @@ > [NoInterfaceObject] > +interface DhKeyAlgorithm : KeyAlgorithm { > + [Throws] > + readonly attribute BigInteger prime; > + [Throws] What's the reason for the [Throws] annotation here? I'm a little surprised that WebIDL lets you apply that to attributes (vs. methods).
Attachment #8476681 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 16•10 years ago
|
||
Comment on attachment 8476730 [details] [diff] [review] 0002-Bug-1034856-Implement-generateKey.patch, v3 Review of attachment 8476730 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/CryptoBuffer.cpp @@ +161,5 @@ > return item; > } > > +bool > +CryptoBuffer::ToSECItem(PLArenaPool *aArena, SECItem* aItem) const Would it be better just to make this the new form of ToSECItem in general? I'm not an expert, but I don't think there are cases where using moz_malloc() is better than SECITEM_AllocItem().
Attachment #8476730 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 17•10 years ago
|
||
Comment on attachment 8476731 [details] [diff] [review] 0003-Bug-1034856-Implement-deriveBits-for-DH.patch Review of attachment 8476731 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/CryptoBuffer.cpp @@ +77,5 @@ > return nullptr; > } > > +bool > +CryptoBuffer::Equals(const CryptoBuffer& aOther) const Is this needed? The parent class nsTArray_impl has an operator== that does the same thing. (And the corresponding operator!=, which is what you want below) http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#798 Note that neither of these should be used when either of the values being compared is supposed to be secret, since the fact that they exit early creates a timing vulnerability. In those cases, you should use NSS_SecureMemcmp, as in HmacTask::Resolve(). Things look OK here, though. ::: dom/crypto/CryptoBuffer.h @@ +36,5 @@ > aArray.ComputeLengthAndData(); > return Assign(aArray.Data(), aArray.Length()); > } > > + bool Equals(const CryptoBuffer& aOther) const; Ditto here. ::: dom/crypto/WebCryptoTask.cpp @@ +2661,5 @@ > + virtual nsresult DoCrypto() MOZ_OVERRIDE > + { > + ScopedPK11SymKey symKey(PK11_PubDerive( > + mPrivKey, mPubKey, PR_FALSE, nullptr, nullptr, CKM_DH_PKCS_DERIVE, > + // TODO CKM_CONCATENATE_DATA_AND_BASE? This and the subsequent argument (CKA_DERIVE) are used by NSS to assign a type to the derived symmetric key. Since we don't rely on these attributes of the symmetric key, it doesn't matter what we set them to. I would just pick a consistent set (e.g., an HMAC CKM and CKA_SIGN), and add a comment that they don't matter. The same should apply to the PK11_PubDeriveWithKDF call in DeriveEcdhBitsTask::DoCrypto(). Which is my other question: Why is this using PK11_PubDerive, while ECDH uses PK11_PubDeriveWithKDF? It seems like they should be the same.
Attachment #8476731 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 18•10 years ago
|
||
Comment on attachment 8477463 [details] [diff] [review] 0005-Bug-1034856-Implement-SPKI-public-key-import-export-.patch, v2 Review of attachment 8477463 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/test/test-vectors.js @@ +630,5 @@ > + "038184000281804fc9904887ac7fabff87f054003547c2d9458c1f6f584c140d" + > + "7271f8b266bb390af7e3f625a629bec9c6a057a4cbe1a556d5e3eb2ff1c6ff67" + > + "7a08b0c7c509110b9e7c6dbc961ca4360362d3dbcffc5bf2bb7207e0a5922f77" + > + "cf5464b316aa49fb62b338ebcdb30bf573d07b663bb7777b69d6317df0a4f636" + > + "ba3d9acbf9e8ac" Where did this SPKI come from? I decoded it and was surprised to find that the included generator is quite large (1018 bits), rather than 2. I guess this is still a legal value, but it looked a bit off.
Attachment #8477463 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 19•10 years ago
|
||
Comment on attachment 8477402 [details] [diff] [review] 0004-Bug-1034856-Implement-raw-public-key-import-export-f.patch, v2 Review of attachment 8477402 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/WebCryptoTask.cpp @@ +1896,5 @@ > + // Check permissions for the requested operation > + if (mKey->GetKeyType() == CryptoKey::PRIVATE && > + mKey->HasUsageOtherThan(CryptoKey::DERIVEBITS | CryptoKey::DERIVEKEY)) { > + return NS_ERROR_DOM_DATA_ERR; > + } It seems like restricting this check to private keys is wrong, if only because public keys aren't good for anything else either. Let's apply this check to public keys as well, and I'll see if there's a spec bug to open.
Attachment #8477402 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 20•10 years ago
|
||
> > + "038184000281804fc9904887ac7fabff87f054003547c2d9458c1f6f584c140d" +
> > + "7271f8b266bb390af7e3f625a629bec9c6a057a4cbe1a556d5e3eb2ff1c6ff67" +
> > + "7a08b0c7c509110b9e7c6dbc961ca4360362d3dbcffc5bf2bb7207e0a5922f77" +
> > + "cf5464b316aa49fb62b338ebcdb30bf573d07b663bb7777b69d6317df0a4f636" +
> > + "ba3d9acbf9e8ac"
>
> Where did this SPKI come from? I decoded it and was surprised to find that
> the included generator is quite large (1018 bits), rather than 2. I guess
> this is still a legal value, but it looked a bit off.
Aha, I see now that it's from the NIST test vectors. Carry on.
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8477402 [details] [diff] [review] 0004-Bug-1034856-Implement-raw-public-key-import-export-f.patch, v2 Olli, can you please take a look at the IDL changes?
Attachment #8477402 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8477402 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 22•10 years ago
|
||
Re-asking for review due to changing ToSECItem() to do |aItem->data = nullptr| so that SECITEM_AllocItem() doesn't assert. We could also require the caller to do that but that seems a little uglier? We will override .data anyway.
Attachment #8476730 -
Attachment is obsolete: true
Attachment #8483505 -
Flags: review?(rlb)
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #16) > > +bool > > +CryptoBuffer::ToSECItem(PLArenaPool *aArena, SECItem* aItem) const > > Would it be better just to make this the new form of ToSECItem in general? > I'm not an expert, but I don't think there are cases where using > moz_malloc() is better than SECITEM_AllocItem(). Did that.
Attachment #8483507 -
Flags: review?(rlb)
Reporter | ||
Updated•10 years ago
|
Attachment #8483507 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 24•10 years ago
|
||
Comment on attachment 8483505 [details] [diff] [review] 0002-Bug-1034856-Implement-generateKey.patch, v4 Review of attachment 8483505 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/CryptoBuffer.cpp @@ +164,5 @@ > +bool > +CryptoBuffer::ToSECItem(PLArenaPool *aArena, SECItem* aItem) const > +{ > + aItem->type = siBuffer; > + aItem->data = nullptr; I guess this is the right thing to do. I have some nagging worry that someone will pass in a SECItem with an allocated buffer, which raises a risk of leaks if we don't SECITEM_FreeItem. But we don't really have a way to distinguish that from a SECItem that's badly initialized, so this seems OK.
Attachment #8483505 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #17) > > +CryptoBuffer::Equals(const CryptoBuffer& aOther) const > > Is this needed? The parent class nsTArray_impl has an operator== that does > the same thing. (And the corresponding operator!=, which is what you want > below) Thanks, I was sure I tried that but whatever. Works now :) > > + ScopedPK11SymKey symKey(PK11_PubDerive( > > + mPrivKey, mPubKey, PR_FALSE, nullptr, nullptr, CKM_DH_PKCS_DERIVE, > > + // TODO CKM_CONCATENATE_DATA_AND_BASE? > > Which is my other question: Why is this using PK11_PubDerive, while ECDH > uses PK11_PubDeriveWithKDF? It seems like they should be the same. Using PK11_PubDeriveWithKDF() key now and fixed the other call as well.
Assignee | ||
Comment 26•10 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #15) > > +interface DhKeyAlgorithm : KeyAlgorithm { > > + [Throws] > > + readonly attribute BigInteger prime; > > + [Throws] > > What's the reason for the [Throws] annotation here? I'm a little surprised > that WebIDL lets you apply that to attributes (vs. methods). We use the same annotation for RsaKeyAlgorithm's BigInteger (publicExponent) so that's why I thought I should use it too? We could be OOM when trying to create the Uint8Array to return for BigInteger attributes. Does that make sense or should I remove [Throws]?
Flags: needinfo?(rlb)
Reporter | ||
Comment 27•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #26) > (In reply to Richard Barnes [:rbarnes] from comment #15) > > > +interface DhKeyAlgorithm : KeyAlgorithm { > > > + [Throws] > > > + readonly attribute BigInteger prime; > > > + [Throws] > > > > What's the reason for the [Throws] annotation here? I'm a little surprised > > that WebIDL lets you apply that to attributes (vs. methods). > > We use the same annotation for RsaKeyAlgorithm's BigInteger (publicExponent) > so that's why I thought I should use it too? We could be OOM when trying to > create the Uint8Array to return for BigInteger attributes. Does that make > sense or should I remove [Throws]? I see. Makes sense to keep it, then.
Flags: needinfo?(rlb)
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8476681 [details] [diff] [review] 0001-Bug-1034856-Introduce-DhKeyAlgorithm.patch, v2 Hey Olli, can you please review the WebIDL changes? Here's a link to the spec: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#dh-DhKeyAlgorithm
Attachment #8476681 -
Flags: review?(bugs)
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8476731 [details] [diff] [review] 0003-Bug-1034856-Implement-deriveBits-for-DH.patch Here are some more IDL changes, spec: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#dh-DhKeyDeriveParams
Attachment #8476731 -
Flags: review?(bugs)
Comment 30•10 years ago
|
||
Comment on attachment 8476681 [details] [diff] [review] 0001-Bug-1034856-Introduce-DhKeyAlgorithm.patch, v2 Could you make sure we have a bug to convert all the *KeyAlgorithm stuff to use dictionaries as it is in the spec.
Attachment #8476681 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8476731 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #30) > Could you make sure we have a bug to convert all the > *KeyAlgorithm stuff to use dictionaries as it is in the spec. That's bug 1037892 and Richard is on it. Shouldn't take too long.
Assignee | ||
Comment 32•10 years ago
|
||
This doesn't compile on Windows and Linux unfortunately. It seems that SECKEY_DHParamKeyTemplate and SECKEY_DHPublicKeyTemplate are not exported by NSS and we can't access them through SEC_ASN1_GET() like I did for ECDH either. Any idea?
Assignee | ||
Comment 33•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #32) > This doesn't compile on Windows and Linux unfortunately. It seems that > SECKEY_DHParamKeyTemplate and SECKEY_DHPublicKeyTemplate are not exported by > NSS and we can't access them through SEC_ASN1_GET() like I did for ECDH > either. Any idea?
Flags: needinfo?(rlb)
Reporter | ||
Comment 34•10 years ago
|
||
Mostly punting this to keeler. You might look at whether there's a difference in how those two items are handled in nss.def: http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/nss/nss.def
Flags: needinfo?(rlb) → needinfo?(dkeeler)
Doesn't look like those are exported at all (I think they still have to be exported as data for SEC_ASN1_GET to work). A few options: (a) file a bug to have them exported in NSS. (b) duplicate their definitions. (c) both, and undo (b) after (a) happens.
Flags: needinfo?(dkeeler)
Reporter | ||
Comment 36•10 years ago
|
||
I would note that we basically did (b) for RSA public keys. http://dxr.mozilla.org/mozilla-central/source/dom/crypto/CryptoKey.cpp?from=CryptoKey.cpp#740 So maybe let's do the same thing here for DH just to get things done, then circle back and file bugs for them both. (Actually, the RSA one might be doable with SEC_ASN1_GET, I haven't checked.)
Assignee | ||
Comment 37•10 years ago
|
||
Guess I should ask for review again, things have changed a bit. https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#dh-DhKeyAlgorithm
Attachment #8476681 -
Attachment is obsolete: true
Attachment #8500756 -
Flags: review?(rlb)
Attachment #8500756 -
Flags: review?(bugs)
Assignee | ||
Comment 38•10 years ago
|
||
Went with comment #36 and copied the templates.
Attachment #8477463 -
Attachment is obsolete: true
Attachment #8500800 -
Flags: review?(rlb)
Updated•10 years ago
|
Attachment #8500756 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 39•10 years ago
|
||
Try likes it: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee02b0672bc6
Assignee | ||
Comment 40•10 years ago
|
||
Pushed together with bug 1078847 of course.
Reporter | ||
Updated•10 years ago
|
Attachment #8500756 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 41•10 years ago
|
||
Comment on attachment 8500800 [details] [diff] [review] 0005-Bug-1034856-Implement-SPKI-public-key-import-export-.patch, v3 Review of attachment 8500800 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these comments addressed. ::: dom/crypto/CryptoKey.cpp @@ +13,5 @@ > #include "mozilla/dom/ToJSValue.h" > > +// Templates taken from security/nss/lib/cryptohi/seckey.c > +// These would ideally be exported by NSS and until that > +// happens we have to keep our own copies. File follow-up bug? @@ +411,5 @@ > + if (isECDHAlgorithm || isDHAlgorithm) { > + SECOidTag oid = SEC_OID_UNKNOWN; > + if (isECDHAlgorithm) { > + oid = SEC_OID_ANSIX962_EC_PUBLIC_KEY; > + } else if (isDHAlgorithm) { Add an "else" branch with MOZ_ASSERT(false), in case the top-level if-statement changes without a change here. @@ +452,5 @@ > + ScopedPLArenaPool arena; > + ScopedCERTSubjectPublicKeyInfo spki; > + > + // NSS doesn't support exporting DH public keys. > + if (aPubKey->keyType == dhKey) { It would be a little cleaner to isolate the body of this branch in a separate function. @@ +466,5 @@ > + if (!spki) { > + return NS_ERROR_DOM_OPERATION_ERR; > + } > + > + SECItem* params = PORT_ArenaZNew(arena, SECItem); Null-check params? @@ +467,5 @@ > + return NS_ERROR_DOM_OPERATION_ERR; > + } > + > + SECItem* params = PORT_ArenaZNew(arena, SECItem); > + SECItem* rv_item = SEC_ASN1EncodeItem(arena, params, aPubKey, Nit: rvItem @@ +485,5 @@ > + if (!rv_item) { > + return NS_ERROR_DOM_OPERATION_ERR; > + } > + > + spki->subjectPublicKey.len <<= 3; I'm assuming this is converting from bytes to bits? Why do you need to do that? A comment here would be good. @@ +501,5 @@ > + if (aPubKey->keyType == ecKey || aPubKey->keyType == dhKey) { > + const SECItem* oidData = nullptr; > + if (aPubKey->keyType == ecKey) { > + oidData = &SEC_OID_DATA_EC_DH; > + } else if (aPubKey->keyType == dhKey) { Same need for "else" with MOZ_ASSERT(false) as above.
Attachment #8500800 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 42•10 years ago
|
||
Need to ask for review here again because I had to make some non-trivial changes to AsymmetricSignVerifyTask when rebasing due to ECDSA landing. I would have liked to use a single scoped arena and SECItem*s but unfortunately there is no way to pass an arena to DSAU_EncodeDerSigWithLen(). I additionally converted all the PORT_Alloc() calls to SECITEM_AllocItem().
Attachment #8483507 -
Attachment is obsolete: true
Attachment #8506100 -
Flags: review?(rlb)
Assignee | ||
Comment 43•10 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #41) > r=me with these comments addressed. All addressed, thanks! > > +// Templates taken from security/nss/lib/cryptohi/seckey.c > > +// These would ideally be exported by NSS and until that > > +// happens we have to keep our own copies. > > File follow-up bug? Filed bug 1083765.
Attachment #8500800 -
Attachment is obsolete: true
Attachment #8506118 -
Flags: review+
Assignee | ||
Comment 44•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=50912138a942
Reporter | ||
Updated•10 years ago
|
Attachment #8506100 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 45•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/638e81b7b11f https://hg.mozilla.org/integration/mozilla-inbound/rev/f5557f9935fb https://hg.mozilla.org/integration/mozilla-inbound/rev/0100d140637a https://hg.mozilla.org/integration/mozilla-inbound/rev/9527980656c9 https://hg.mozilla.org/integration/mozilla-inbound/rev/d866ac7f8606 https://hg.mozilla.org/integration/mozilla-inbound/rev/a02fdcc10dd9
Assignee | ||
Comment 46•10 years ago
|
||
Pushed a follow-up to fix a small SECItem leak on ASAN builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/ace6aa02d442 Richard, can you please do a post-review to sanity check that? The issue here was that the SECITEM_CopyItem() call in CryptoKey::PublicKeyToSpki() was passing spki->arena as the PLArenaPool to use. That is null however as stated above for DH keys.
Flags: needinfo?(rlb)
Assignee | ||
Comment 47•10 years ago
|
||
I backed the follow-up out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/d78ab6a8c92f Turns out that arena goes out of scope before spki, so it frees the PLArenaPool and seems to override spki->arena with rubbish and that makes us crash when spki goes out of scope because that tries to free its arena. I landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6233f10333 ... to really just mimic how the SPKI object is created below and we can use the scoped pointer to free that. I unfortunately forgot to update the PublicDhKeyToSpki() call so I had to push some more: https://hg.mozilla.org/integration/mozilla-inbound/rev/cc77c762d6a0 If that all still leaks I'll back it out but it really shouldn't. Sorry for the hassle :/
Reporter | ||
Comment 48•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #46) > Pushed a follow-up to fix a small SECItem leak on ASAN builds: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/ace6aa02d442 > > Richard, can you please do a post-review to sanity check that? The issue > here was that the SECITEM_CopyItem() call in CryptoKey::PublicKeyToSpki() > was passing spki->arena as the PLArenaPool to use. That is null however as > stated above for DH keys. That looks sane to me. Seems like in generally allocating the arena at the top of the method is the right idea, so that it remains in scope for the method.
Flags: needinfo?(rlb)
Comment 49•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/638e81b7b11f https://hg.mozilla.org/mozilla-central/rev/f5557f9935fb https://hg.mozilla.org/mozilla-central/rev/0100d140637a https://hg.mozilla.org/mozilla-central/rev/9527980656c9 https://hg.mozilla.org/mozilla-central/rev/d866ac7f8606 https://hg.mozilla.org/mozilla-central/rev/a02fdcc10dd9 https://hg.mozilla.org/mozilla-central/rev/3c6233f10333 https://hg.mozilla.org/mozilla-central/rev/cc77c762d6a0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Reporter | ||
Comment 50•10 years ago
|
||
Comment on attachment 8476731 [details] [diff] [review] 0003-Bug-1034856-Implement-deriveBits-for-DH.patch Approval Request Comment [Feature/regressing bug #]: WebCrypto (865789) [User impact if declined]: DH will be unavailable in WebCrypto [Describe test coverage new/current, TBPL]: Mochitests in patch [Risks and why]: Low risk, only affects WebCrypto [String/UUID change made/needed]: None If approved, please also uplift the other patches in this bug.
Attachment #8476731 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8476731 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 51•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/526f7c0682e1 https://hg.mozilla.org/releases/mozilla-aurora/rev/ce89df7057f0 https://hg.mozilla.org/releases/mozilla-aurora/rev/0112f0547513 https://hg.mozilla.org/releases/mozilla-aurora/rev/7c17b33485f0 https://hg.mozilla.org/releases/mozilla-aurora/rev/f6adbda3e224 https://hg.mozilla.org/releases/mozilla-aurora/rev/29cf6a01532f https://hg.mozilla.org/releases/mozilla-aurora/rev/2cfa6e659ae6
Comment 52•10 years ago
|
||
Backed out for test failures. https://hg.mozilla.org/releases/mozilla-aurora/rev/86f3ce4cf56a https://treeherder.mozilla.org/ui/logviewer.html#?job_id=355453&repo=mozilla-aurora
Reporter | ||
Comment 53•10 years ago
|
||
Tests are failing on Aurora: https://treeherder.mozilla.org/ui/index.html#/jobs?repo=mozilla-aurora&revision=569c89370744 It appears that the promise returned by the doGenerateDH() method on line 95 of test_WebCrypto_DH.html is being resolved with the value undefined. I tried making WebCryptoTask.cpp the same as in mozilla-central (there was only a minor difference), and this didn't resolve the issue. Tim: Could you take a look at this?
Assignee | ||
Comment 54•10 years ago
|
||
We're missing bug 1078847. That needs to be uplifted as well... WFM locally after applying that patch too.
Comment 55•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #54) > We're missing bug 1078847. That needs to be uplifted as well... WFM locally > after applying that patch too. Were you going to request approval on that? :)
Assignee | ||
Comment 56•10 years ago
|
||
Yes, thanks for the reminder. Got a little sidetracked :)
Comment 57•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/af5fea2deedb https://hg.mozilla.org/releases/mozilla-aurora/rev/5ec58576edf3 https://hg.mozilla.org/releases/mozilla-aurora/rev/4db14b0721a2 https://hg.mozilla.org/releases/mozilla-aurora/rev/8e01a459f532 https://hg.mozilla.org/releases/mozilla-aurora/rev/cc040db9b406 https://hg.mozilla.org/releases/mozilla-aurora/rev/9b74e7cf7995 https://hg.mozilla.org/releases/mozilla-aurora/rev/372443d17537
You need to log in
before you can comment on or make changes to this bug.
Description
•