[webcrypto] Implement AES-CTR using BoringSSL.

[webcrypto] Implement AES-CTR using BoringSSL.

BUG=399084
Committed: https://crrev.com/4d7a0e041f0b446265e4b9261396342e9f77fab5
Cr-Commit-Position: refs/heads/master@{#292053}

[eroman, 2014-08-22 19:23:55]

Tentatively changing reviewer rsleevi --> davidben (Ryan doesn't have time for
this ATM).

[davidben, 2014-08-25 19:25:45]

davidben@chromium.org changed reviewers:
+ agl@chromium.org

[davidben, 2014-08-25 19:25:46]

+agl. Could we teach BoringSSL to deal with < 128-bit counters and perhaps
overflow too? Implementing WebCrypto's API seems to be rather awkward.

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

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_cbc_openssl.cc:82: cipher_operation)) {
Nit: I probably would have just written this as
  cipher_operation == ENCRYPT
rather than require the enum match, though adding a comment that they need to
match is fine too.

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

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:74: return
Status::OperationError();
Nit: I think we usually put curlies if the conditional took multiple lines?
(Maybe I'm making this up or it was just one reviewer I had a bit ago.)

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:98: crypto::ScopedBIGNUM
result;
Unused

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:152: // value, and is
therefore permitted to wrap around to zero on overflow.
Should we teach BoringSSL to do this internally? Maybe also to check for
wraparound. For the caller to check this kind of thing is rather a nuisance and
BoringSSL can do it in the ctr128_inc function which is less of a nuisance over
the wraparound at least. Overflow I guess we'd conceivably want to be cleverer
than checking on every increment.

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:156: // encrypt/decrypt.
This is because WebCrypto doesn't maintain any context across encrypt calls?

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:204: if
(BN_cmp(num_output_blocks.get(), num_counter_values.get()) > 0)
Seems odd to use BIGNUMs for this (and some of the other logic) when we know
num_counter_values is bounded by size_t or so. Unfortunately, size_t(1) <<
output_max_len doesn't seem to behave sensibly when output_max_len > NUM_BITS,
so the check might be a little screwy...

...or we could make this BoringSSL's problem...

[agl, 2014-08-25 20:36:15]

The reason why I didn't want this inside BoringSSL is that it's not clear where
to put it. More ctrl values for EVP_CIPHER? Also, the AES code is already
complex and I don't see this level of flexibility being needed elsewhere.

In terms of this package, if it were me, I'd require that counter sizes be a
multiple of 8 bits. Also, since we know that the length of a single write is
limited to an int (or, at most, a size_t) I bet the use of BIGNUMs could be
avoided if we wished.

[eroman, 2014-08-26 00:25:28]

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

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_cbc_openssl.cc:82: cipher_operation)) {
On 2014/08/25 19:25:45, David Benjamin wrote:
> Nit: I probably would have just written this as
>   cipher_operation == ENCRYPT
> rather than require the enum match, though adding a comment that they need to
> match is fine too.

Acknowledged.

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

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:74: return
Status::OperationError();
On 2014/08/25 19:25:46, David Benjamin wrote:
> Nit: I think we usually put curlies if the conditional took multiple lines?
> (Maybe I'm making this up or it was just one reviewer I had a bit ago.)

Done. I agree

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:98: crypto::ScopedBIGNUM
result;
On 2014/08/25 19:25:46, David Benjamin wrote:
> Unused

D'oh! Done.

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:152: // value, and is
therefore permitted to wrap around to zero on overflow.
On 2014/08/25 19:25:46, David Benjamin wrote:
> Should we teach BoringSSL to do this internally? Maybe also to check for
> wraparound. For the caller to check this kind of thing is rather a nuisance
and
> BoringSSL can do it in the ctr128_inc function which is less of a nuisance
over
> the wraparound at least. Overflow I guess we'd conceivably want to be cleverer
> than checking on every increment.

I will defer to you and Adam. We discussed this briefly over email, I will
forward you the thread.

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:156: // encrypt/decrypt.
On 2014/08/25 19:25:46, David Benjamin wrote:
> This is because WebCrypto doesn't maintain any context across encrypt calls?

Since the WebCrypto API doesn't describe any errors to reject with when re-using
a counter block, and exposes this through the "crypto.subtle" interface, I think
it is fair game to leave this as a responsibility of the caller.

Enforcing across multiple calls could get tricky, since I believe it would
require per-Key state which could grow boundlessly if many different counter
blocks are used with the key. This would also have to be propagated when cloning
keys, yet is easily bypassed by exporting then re-importing the key.

The reason I added it for single-call case is because it was easily detectable.

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:204: if
(BN_cmp(num_output_blocks.get(), num_counter_values.get()) > 0)
On 2014/08/25 19:25:46, David Benjamin wrote:
> Seems odd to use BIGNUMs for this (and some of the other logic) when we know
> num_counter_values is bounded by size_t or so. Unfortunately, size_t(1) <<
> output_max_len doesn't seem to behave sensibly when output_max_len > NUM_BITS,
> so the check might be a little screwy...
> 
> ...or we could make this BoringSSL's problem...

Agreed.

This check can be written without BIGNUMs, but it is uglier in my opinion. When
I tried that I ended up with some compile asserts to make sure I knew something
about the size of the datatypes.

[davidben, 2014-08-26 20:53:27]

lgtm. The BIGNUMs make me unhappy, but I can see how it'd might be even messier
without them.

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

https://codereview.chromium.org/491763002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_ctr_openssl.cc:156: // encrypt/decrypt.
On 2014/08/26 00:25:27, eroman wrote:
> On 2014/08/25 19:25:46, David Benjamin wrote:
> > This is because WebCrypto doesn't maintain any context across encrypt calls?
> 
> Since the WebCrypto API doesn't describe any errors to reject with when
re-using
> a counter block, and exposes this through the "crypto.subtle" interface, I
think
> it is fair game to leave this as a responsibility of the caller.
> 
> Enforcing across multiple calls could get tricky, since I believe it would
> require per-Key state which could grow boundlessly if many different counter
> blocks are used with the key. This would also have to be propagated when
cloning
> keys, yet is easily bypassed by exporting then re-importing the key.
> 
> The reason I added it for single-call case is because it was easily
detectable.

Well, putting it in the Key wouldn't really work anyway since the encrypt call
takes its own AesCtrParams parameter. So, yeah, this seems sound. If there were
some kind of EncryptionContext that you made out of an AesCtrParams once and
used repeatedly, we could stick state there, but meh.

[eroman, 2014-08-26 21:41:17]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-08-26 21:42:51]

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

[commit-bot: I haz the power, 2014-08-27 00:30:48]

Committed patchset #4 (80001) as 962a49d00467e81be6a31080330f5a4c510df007

[commit-bot: I haz the power, 2014-09-10 02:48:11]

Patchset 4 (id:??) landed as
https://crrev.com/4d7a0e041f0b446265e4b9261396342e9f77fab5
Cr-Commit-Position: refs/heads/master@{#292053}