1026398 - Add support for RSA-OAEP to WebCrypto API

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0009-RSA-OAEP.patch (WIP)

Comment 3

[Richard Barnes [:rbarnes]]

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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.patch

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1026398-Add-RSA-OAEP-tests.patch

Comment 6

[Richard Barnes [:rbarnes]]

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, &param,
> +             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.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.patch, v2

Carrying over r=rbarnes.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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!

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Richard Barnes [:rbarnes]]

Attachment: 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.1.patch r=rbarnes,bzbarsky

Comment 12

[Richard Barnes [:rbarnes]]

Attachment: 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.0-1.interdiff

Comment 13

[Richard Barnes [:rbarnes]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for these patches? :)

Comment 15

[Richard Barnes [:rbarnes]]

Attachment: 0001-Bug-1026398-Add-support-for-RSA-OAEP-to-WebCrypto-AP.2.patch r=rbarnes,bzbarsky

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)

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5460c4558253
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3359ddd831

Comment 18

[Ed Morley [:emorley]]

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

Comment 19

[Richard Barnes [:rbarnes]]

Attachment: 0002-Bug-1026398-Add-RSA-OAEP-tests.1.patch r=rbarnes

Disabling tests on b2g and updating Bug 1010743 to re-enable include re-enabling.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing :(

Comment 21

[Richard Barnes [:rbarnes]]

Attachment: bug-1026398.1.patch

Rebased and consolidated.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9c96d0f03d

Richard, thanks for finishing the patch and addressing the review comments!

Comment 23

[Richard Barnes [:rbarnes]]

Glad to help!

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b9c96d0f03d

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "WebCrypto API: RSA-OAEP, PBKDF2 and AES-KW support" as wording.
bug 1021607 + bug 1034852 + bug 1026398