Closed
Bug 1034854
Opened 10 years ago
Closed 10 years ago
Add support for ECDSA to WebCrypto API
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: rbarnes, Assigned: rbarnes)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 2 obsolete files)
31.33 KB,
patch
|
bzbarsky
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rlb
Assignee | ||
Comment 1•10 years ago
|
||
This is going to want to wait for Bug 1037892 and Bug 1057161 to land, but I think it's mostly complete, modulo rebasing on top of those.
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
This patch rebases the earlier WIP on top of Bug 1037892 and Bug 1057161. The main interesting things are:
(1) the PublicKeyValid() method, which tests for a public key's validity by using the check added in Bug 1057161, and
(2) the transformation of RsassaPkcs1Task to a more generic AsymmetricVerifyTask, adding ECDSA and simplifying the NSS verification calls.
Attachment #8484213 -
Attachment is obsolete: true
Attachment #8499102 -
Flags: review?(ttaubert)
Attachment #8499102 -
Flags: review?(dkeeler)
Comment 3•10 years ago
|
||
Comment on attachment 8499102 [details] [diff] [review]
bug-1034854.1.patch
Review of attachment 8499102 [details] [diff] [review]:
-----------------------------------------------------------------
NSS-related things look good to me.
::: dom/crypto/CryptoKey.cpp
@@ +898,5 @@
> + if (!slot.get()) {
> + return false;
> + }
> +
> + CK_OBJECT_HANDLE id = PK11_ImportPublicKey(slot, aPubKey, PR_FALSE);
Maybe we should add a comment that this code assumes that if calling PK11_ImportPublicKey and checking for a valid handle succeeds, the key is valid.
::: dom/crypto/WebCryptoTask.cpp
@@ +64,5 @@
> TA_SHA_384 = 17,
> TA_SHA_512 = 18,
> // Later additions
> TA_AES_KW = 19,
> + TA_ECDSA = 20
nit: include the trailing ',' so we don't have to touch this line if/when we add something else
@@ +997,5 @@
> + case CKM_SHA384:
> + mOidTag = SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION; break;
> + case CKM_SHA512:
> + mOidTag = SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION; break;
> + default: {
are the braces necessary here?
@@ +1037,5 @@
> + case CKM_SHA384:
> + mOidTag = SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE; break;
> + case CKM_SHA512:
> + mOidTag = SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE; break;
> + default: {
same with the braces
@@ +1069,5 @@
> virtual nsresult DoCrypto() MOZ_OVERRIDE
> {
> nsresult rv;
> if (mSign) {
> ScopedSECItem signature((SECItem*) PORT_Alloc(sizeof(SECItem)));
we should probably null-check signature here
@@ +1081,5 @@
> +
> + if (mEcdsa) {
> + // DER-decode the signature
> + int signatureLength = PK11_SignatureLen(mPrivKey);
> + ScopedSECItem rawSignature(DSAU_DecodeDerSigToLen(signature.get(),
Don't forget to add this and the other DSAU_ function to config/external/nss/nss.def
@@ +1102,5 @@
> + if (!rawSignature.get()) {
> + return NS_ERROR_DOM_UNKNOWN_ERR;
> + }
> +
> + signature = (SECItem*) PORT_Alloc(sizeof(SECItem));
null-check signature here
::: dom/crypto/test/test_WebCrypto_ECDSA.html
@@ +51,5 @@
> +);
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(
> + "ECDSA JWK import and verify a known-good signature",
Would it be worthwhile to also test rejecting a known-tampered signature?
@@ +55,5 @@
> + "ECDSA JWK import and verify a known-good signature",
> + function() {
> + var that = this;
> + var alg = { name: "ECDSA", namedCurve: "P-256", hash: "SHA-256" };
> +
nit: trailing whitespace
::: dom/webidl/SubtleCrypto.webidl
@@ +89,5 @@
>
> +dictionary EcdsaParams : Algorithm {
> + required AlgorithmIdentifier hash;
> +};
> +
nit: unnecessary extra blank line
Attachment #8499102 -
Flags: review?(dkeeler) → review+
Comment 4•10 years ago
|
||
Comment on attachment 8499102 [details] [diff] [review]
bug-1034854.1.patch
Review of attachment 8499102 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/crypto/CryptoKey.cpp
@@ +820,5 @@
> if (!arena) {
> return nullptr;
> }
>
> + ScopedSECKEYPublicKey key(PORT_ArenaZNew(arena, SECKEYPublicKey));
That seems magic, I assume this is only used to free key->pkcs11Slot after calling PublicKeyValid()? If PublicKeyValid() succeeds then key->pkcs11Slot always != NULL. So I think we'd only need this if PK11_DestroyObject() fails? It doesn't look like key->pkcs11Slot is set to NULL when PK11_DestroyObject() succeeds? Would we double-free in that case?
How about copying the key param to a ScopedSECKEYPublicKey only in PublicKeyValid()? So we wouldn't confuse readers here and would free pkcs11Slot a little closer to where it's allocated?
Maybe I'm getting things completely wrong here but in either case maybe add a comment as to why we're using a scoped public key here when allocating in a scoped arena?
Attachment #8499102 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #4)
> Comment on attachment 8499102 [details] [diff] [review]
> bug-1034854.1.patch
>
> Review of attachment 8499102 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/crypto/CryptoKey.cpp
> @@ +820,5 @@
> > if (!arena) {
> > return nullptr;
> > }
> >
> > + ScopedSECKEYPublicKey key(PORT_ArenaZNew(arena, SECKEYPublicKey));
>
> That seems magic, I assume this is only used to free key->pkcs11Slot after
> calling PublicKeyValid()? If PublicKeyValid() succeeds then key->pkcs11Slot
> always != NULL. So I think we'd only need this if PK11_DestroyObject()
> fails? It doesn't look like key->pkcs11Slot is set to NULL when
> PK11_DestroyObject() succeeds? Would we double-free in that case?
>
> How about copying the key param to a ScopedSECKEYPublicKey only in
> PublicKeyValid()? So we wouldn't confuse readers here and would free
> pkcs11Slot a little closer to where it's allocated?
>
> Maybe I'm getting things completely wrong here but in either case maybe add
> a comment as to why we're using a scoped public key here when allocating in
> a scoped arena?
I think this was just a result of me generally switching unscoped pointers to scoped ones. But that seems unnecessary in this case because the pointer is pointing to arena-managed memory, and the arena is scoped. I'll revert it to unscoped.
Assignee | ||
Comment 6•10 years ago
|
||
bz: Could you please review the WebIDL so that the build scripts will let me land this?
Happy try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b76a1f13c8ab
Attachment #8499102 -
Attachment is obsolete: true
Attachment #8504520 -
Flags: review?(bzbarsky)
Comment 7•10 years ago
|
||
Comment on attachment 8504520 [details] [diff] [review]
bug-1034854.2.patch r=ttaubert,dkeeler,bz
r=me on the IDL bits
Attachment #8504520 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8504520 -
Attachment description: bug-1034854.2.patch → bug-1034854.2.patch r=ttaubert,dkeeler,bz
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8504520 [details] [diff] [review]
bug-1034854.2.patch r=ttaubert,dkeeler,bz
Approval Request Comment
[Feature/regressing bug #]: WebCrypto (865789)
[User impact if declined]: ECDSA 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
Attachment #8504520 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8504520 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 11•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•