[webcrypto] Implement RSA-OAEP using BoringSSL.

[webcrypto] Implement RSA-OAEP using BoringSSL.

BUG=395840
R=davidben@chromium.org, rsleevi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286599

[Ryan Sleevi, 2014-07-25 19:33:02]

Redirecting to David

[Ryan Sleevi, 2014-07-25 19:34:56]

https://codereview.chromium.org/419673006/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/rsa_oaep_openssl.cc (right):

https://codereview.chromium.org/419673006/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:32: ScopedEVP_PKEY_CTX;
Should probably move this into crypto/scoped_openssl_types. It certainly meets
the criteria, and I suspect you'll need it there.

https://codereview.chromium.org/419673006/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:35: void operator()(uint8_t*
ptr) const { OPENSSL_free(ptr); }
Probably this too?

[eroman, 2014-07-25 20:11:48]

https://codereview.chromium.org/419673006/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/rsa_oaep_openssl.cc (right):

https://codereview.chromium.org/419673006/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:32: ScopedEVP_PKEY_CTX;
On 2014/07/25 19:34:56, Ryan Sleevi wrote:
> Should probably move this into crypto/scoped_openssl_types. It certainly meets
> the criteria, and I suspect you'll need it there.

Done.

https://codereview.chromium.org/419673006/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:35: void operator()(uint8_t*
ptr) const { OPENSSL_free(ptr); }
On 2014/07/25 19:34:56, Ryan Sleevi wrote:
> Probably this too?

Done.

I created a typedef named "ScopedOpenSSLBytes"

https://codereview.chromium.org/419673006/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:70: if (label.size()) {
Note that I slightly cleaned up the label handling code (only call
rsa_oeap_label() when there is a non-empty label)

[eroman, 2014-07-30 05:10:57]

Ping

[davidben, 2014-07-30 16:08:48]

lgtm with comment

https://codereview.chromium.org/419673006/diff/110001/content/child/webcrypto...
File content/child/webcrypto/openssl/rsa_oaep_openssl.cc (right):

https://codereview.chromium.org/419673006/diff/110001/content/child/webcrypto...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:46: EVP_PKEY* pkey = pkey =
AsymKeyOpenSsl::Cast(key)->key();
I'm not very familiar with the WebCrypto code; do you also need to check that
this is an RSA key, or is it taken care of for you? (I suppose there actually
isn't any other EVP_PKEY implementation in BoringSSL right now that provides
encrypt and decrypt, and the EVP_PKEY_CTX_set_rsa* ctrls will fail.)

https://codereview.chromium.org/419673006/diff/110001/content/child/webcrypto...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:66: // calling
set0_rsa_oaep_label().
This is a quirk we inherited from upstream. We should probably add a set1
variant later because this is just obnoxious. :-)

[eroman, 2014-07-30 16:27:59]

Thanks for the review!

https://codereview.chromium.org/419673006/diff/110001/content/child/webcrypto...
File content/child/webcrypto/openssl/rsa_oaep_openssl.cc (right):

https://codereview.chromium.org/419673006/diff/110001/content/child/webcrypto...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:46: EVP_PKEY* pkey = pkey =
AsymKeyOpenSsl::Cast(key)->key();
On 2014/07/30 16:08:48, David Benjamin wrote:
> I'm not very familiar with the WebCrypto code; do you also need to check that
> this is an RSA key, or is it taken care of for you? (I suppose there actually
> isn't any other EVP_PKEY implementation in BoringSSL right now that provides
> encrypt and decrypt, and the EVP_PKEY_CTX_set_rsa* ctrls will fail.)

tl;dr: It should be safe. 

Here is how it works:

WebCrypto keys are passed back and forth blink <--> chromium. To hide any
NSS/BoringSSL internals, the WebCryptoKey stores an opaque pointer
(WebCryptoKeyHandle) which can be cast and accessed by the chromium side, and
whose lifetime is tied to the WebCryptoKey.

There are two fields on the WebCryptoKey which imply what the concrete handle
type was:
   * algorithm.id()
    * keyType()

A particular "AlgorithmImplementation" is registered for each
WebCryptoAlgorithmId, and only keys with a matching id are dispatched to it. We
can therefore be guaranteed that the key here was in fact created by
RsaOaepImplementation. We then further assume that RsaOeapImplementation only
ever creates AsymKeyOpenSssl handles. So the only question is whether it
represents a public or private key.

That said, I agree the casting is scary and non-obvious. As a safety-net I use
the function AsymKeyOpenSsl::Cast() which tries to do safer casting (rather than
just re-interpret casting, it uses a common base class + virtual method, which
predictably return NULL if the types don't match). So if something does break in
the future, it should just result in a NULL pointer crash rather than something
exploitable.

https://codereview.chromium.org/419673006/diff/110001/content/child/webcrypto...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:66: // calling
set0_rsa_oaep_label().
On 2014/07/30 16:08:47, David Benjamin wrote:
> This is a quirk we inherited from upstream. We should probably add a set1
> variant later because this is just obnoxious. :-)

I will file a bug, and can investigate as a follow-up.

Would the set1 variant be expected to make a copy itself to save in the ctx, or
simply have the ctx reference unowned bytes? The latter could be confusing since
if the ctx is re-used later the memory could well be gone.

[davidben, 2014-07-30 16:44:13]

https://codereview.chromium.org/419673006/diff/110001/content/child/webcrypto...
File content/child/webcrypto/openssl/rsa_oaep_openssl.cc (right):

https://codereview.chromium.org/419673006/diff/110001/content/child/webcrypto...
content/child/webcrypto/openssl/rsa_oaep_openssl.cc:66: // calling
set0_rsa_oaep_label().
On 2014/07/30 16:27:59, eroman wrote:
> On 2014/07/30 16:08:47, David Benjamin wrote:
> > This is a quirk we inherited from upstream. We should probably add a set1
> > variant later because this is just obnoxious. :-)
> 
> I will file a bug, and can investigate as a follow-up.
> 
> Would the set1 variant be expected to make a copy itself to save in the ctx,
or
> simply have the ctx reference unowned bytes? The latter could be confusing
since
> if the ctx is re-used later the memory could well be gone.

I was thinking it could just make a copy. That seems easiest for the caller.

I suppose this version (set0) is actually more consistent with
EVP_PKEY_CTX_set_rsa_keygen_pubexp which also takes ownership from the caller.
Doesn't seem to be anything else that takes a pointer type as parameter.

[eroman, 2014-07-30 17:38:50]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-07-30 17:40:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/419673006/130001

[eroman, 2014-07-30 19:55:08]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-07-30 19:56:26]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-07-30 19:57:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/419673006/150001

[Ryan Sleevi, 2014-07-30 21:28:17]

lgtm

[eroman, 2014-07-30 21:29:02]

Committed patchset #8 manually as r286599 (presubmit successful).