Closed
Bug 1026398
Opened 10 years ago
Closed 10 years ago
Add support for RSA-OAEP to WebCrypto API
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla33
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 33+ |
People
(Reporter: ttaubert, Assigned: rbarnes)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•10 years ago
|
||
Trying to use RSA_EncryptOAEP() I discovered that blapi.h is in freebl's PRIVATE_EXPORTS, so only for internal NSS use: https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/freebl/manifest.mn#58 Should we use the Softoken library here? I'm not sure I understand what exactly this library is supposed to do, offer a simpler API? We don't seem to use Softoken anywhere else in the platform afaict.
Flags: needinfo?(rlb)
Reporter | ||
Comment 2•10 years ago
|
||
Flags: needinfo?(rlb)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8441891 [details] [diff] [review] 0009-RSA-OAEP.patch (WIP) Review of attachment 8441891 [details] [diff] [review]: ----------------------------------------------------------------- This is looking pretty good. Need to patch importKey and generateKey, add the length check, and add some tests. You can find some test vectors from RSA here: http://www.emc.com/emc-plus/rsa-labs/standards-initiatives/pkcs-rsa-cryptography-standard.htm As with RSAES-PKCS1-v1_5, you won't be able to do an encryption known-answer test, since there's randomization. So you'll want to do a decrypt known-answer and an encrypt/decrypt round-trip. Please link to the source of the test vectors when you add them.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•10 years ago
|
||
Attachment #8441891 -
Attachment is obsolete: true
Attachment #8444050 -
Flags: review?(rlb)
Reporter | ||
Comment 5•10 years ago
|
||
Attachment #8444051 -
Flags: review?(rlb)
Assignee | ||
Updated•10 years ago
|
Attachment #8444051 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8444050 [details] [diff] [review] 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.patch Review of attachment 8444050 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks pretty good. Couple of minor comments below. ::: dom/crypto/WebCryptoTask.cpp @@ +517,5 @@ > + // is RSA-OAEP, and that only happens if we've constructed > + // an RsaHashedKeyAlgorithm. > + nsRefPtr<RsaHashedKeyAlgorithm> rsaAlg = > + static_cast<RsaHashedKeyAlgorithm*>(aKey.Algorithm()); > + mHashMechanism = rsaAlg->Hash()->Mechanism(); Should this use MapAlgorithmNameToMechanism? Maybe that's too much trouble given that you have to look down into the hash. @@ +544,5 @@ > + CryptoBuffer mLabel; > + CryptoBuffer mData; > + uint32_t mStrength; > + bool mEncrypt; > + Extra blank line. @@ +575,5 @@ > + if (mEncrypt) { > + rv = MapSECStatus(PK11_PubEncrypt( > + mPubKey.get(), CKM_RSA_PKCS_OAEP, ¶m, > + mResult.Elements(), &outLen, mResult.Length(), > + mData.Elements(), mData.Length(), nullptr)); It would be helpful to have a comment here that PK11_PubEncrypt does the length check on the plaintext.
Attachment #8444050 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 7•10 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #6) > ::: dom/crypto/WebCryptoTask.cpp > @@ +517,5 @@ > > + // is RSA-OAEP, and that only happens if we've constructed > > + // an RsaHashedKeyAlgorithm. > > + nsRefPtr<RsaHashedKeyAlgorithm> rsaAlg = > > + static_cast<RsaHashedKeyAlgorithm*>(aKey.Algorithm()); > > + mHashMechanism = rsaAlg->Hash()->Mechanism(); > > Should this use MapAlgorithmNameToMechanism? Maybe that's too much trouble > given that you have to look down into the hash. Yeah, I don't think it would buy us that much. The RsaHashedKeyAlgorithm instance exists on the key already and accessing the fields here should be almost free. > It would be helpful to have a comment here that PK11_PubEncrypt does the > length check on the plaintext. Will add.
Reporter | ||
Comment 8•10 years ago
|
||
Carrying over r=rbarnes.
Attachment #8444050 -
Attachment is obsolete: true
Attachment #8451131 -
Flags: review+
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8451131 [details] [diff] [review] 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.patch, v2 Boris, can you please review the IDL changes? Thanks!
Attachment #8451131 -
Flags: review?(bzbarsky)
Comment 10•10 years ago
|
||
Comment on attachment 8451131 [details] [diff] [review] 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.patch, v2 >+ mozilla::dom::CryptoKey& aKey, const CryptoOperationData& aData, Do you need the "mozilla::dom::" bit? >+ // static_cast is safe because we only get here if the algorithm name It might be nice to have As* methods on KeyAlgorithm that do the downcasting. Followup? >+dictionary RsaOaepParams : Algorithm { >+ CryptoOperationData label; This doesn't match the spec; the spec has label as nullable. Is the bug in the code, or in the spec? I'm assuming the latter. That said, the spec doesn't define behavior when "label" is null or not passed. Or for that matter if it's a typed array. It needs to. I've raised http://lists.w3.org/Archives/Public/public-webcrypto/2014Jul/0020.html r=me with those addressed.
Attachment #8451131 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8451131 -
Attachment is obsolete: true
Attachment #8453490 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
Attached updated patch and interdiff. (In reply to Boris Zbarsky [:bz] from comment #10) > Comment on attachment 8451131 [details] [diff] [review] > 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.patch, v2 > > >+ mozilla::dom::CryptoKey& aKey, const CryptoOperationData& aData, > > Do you need the "mozilla::dom::" bit? No. Removed all occurrences in WebCryptoTask.cpp. > >+ // static_cast is safe because we only get here if the algorithm name > > It might be nice to have As* methods on KeyAlgorithm that do the > downcasting. Followup? Filed Bug 1036734. > >+dictionary RsaOaepParams : Algorithm { > >+ CryptoOperationData label; > > This doesn't match the spec; the spec has label as nullable. Is the bug in > the code, or in the spec? I'm assuming the latter. I think you're right. It doesn't make sense to have nullable members in a dictionary, since they're already optional. Nontheless, I went ahead and changed this to match the spec for now. It's a quick fix to revert it if/when the spec if fixed.
Assignee | ||
Updated•10 years ago
|
Assignee: ttaubert → rlb
Keywords: checkin-needed
Updated•10 years ago
|
Attachment #8453492 -
Attachment is patch: true
Attachment #8453492 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Comment 15•10 years ago
|
||
Updated to ensure that all necessary NSS symbols are exported. Otherwise, linking fails on some platforms. r? to Keeler for these two lines. Try run (without this patch) showing success on all tests on linux32, with build failures on some platforms: https://tbpl.mozilla.org/?tree=Try&rev=b80d7904b1e7 Try run (with this patch) showing success building on all platforms https://tbpl.mozilla.org/?tree=Try&rev=c0a973528c94 (Following the "T-shaped" test pattern recommended in https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices)
Attachment #8453490 -
Attachment is obsolete: true
Attachment #8454010 -
Flags: review?(dkeeler)
Assignee | ||
Updated•10 years ago
|
Attachment #8453492 -
Attachment is obsolete: true
Comment on attachment 8454010 [details] [diff] [review] 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.2.patch r=rbarnes,bzbarsky Review of attachment 8454010 [details] [diff] [review]: ----------------------------------------------------------------- r=me for security/build/nss.def changes
Attachment #8454010 -
Flags: review?(dkeeler) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5460c4558253 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3359ddd831
Keywords: checkin-needed
Comment 18•10 years ago
|
||
Backed out for test_WebCrypto.html timeouts on B2G, eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=43613761&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6aaa154f38 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e77266536d5
Assignee | ||
Comment 19•10 years ago
|
||
Disabling tests on b2g and updating Bug 1010743 to re-enable include re-enabling.
Attachment #8444051 -
Attachment is obsolete: true
Attachment #8454942 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 21•10 years ago
|
||
Rebased and consolidated.
Attachment #8454010 -
Attachment is obsolete: true
Attachment #8454942 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9c96d0f03d Richard, thanks for finishing the patch and addressing the review comments!
Keywords: checkin-needed
Assignee | ||
Comment 23•10 years ago
|
||
Glad to help!
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6b9c96d0f03d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 25•10 years ago
|
||
Added to the release notes with "WebCrypto API: RSA-OAEP, PBKDF2 and AES-KW support" as wording. bug 1021607 + bug 1034852 + bug 1026398
relnote-firefox:
--- → 33+
Keywords: relnote
Updated•8 years ago
|
Component: Security → DOM: Security
You need to log in
before you can comment on or make changes to this bug.
Description
•