[webcrypto] Add symmetric key export for NSS and OpenSSL.

[webcrypto] Add symmetric key export for NSS and OpenSSL.
Also add test to check randomness of generated symmetric keys.

BUG=245025
TEST=content_unittests --gtest_filter="WebCryptoImpl*"

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

[eroman, 2013-12-04 19:04:00]

I am happy with this other than the randomness testing.

We can discuss that further on this thread, or move it to a follow up
changelist.

https://codereview.chromium.org/100593002/diff/20001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_openssl.cc (right):

https://codereview.chromium.org/100593002/diff/20001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_openssl.cc:171: *buffer =
blink::WebArrayBuffer::create(sym_key->key().size(), 1);
[optional] Consider extracting this to a helper in webcrypto_util (there are 3
places which use it now). ex: CreateArrayBuffer(data, data_size).

https://codereview.chromium.org/100593002/diff/20001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/100593002/diff/20001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:123: // calculated entropy
is > 0.75, which solves to 0.2145 ~< p ~< 0.7855.
Doesn't this mean that truly random buffers will fail the heuristic 25% of the
time?

That would be an unacceptably high failure rate for unittests, which are
expected to pass 100% of the time.

The existing tests for crypto.getRandomValues() take the approach of generating
multiple random buffers, and then seeing if they match exactly (the probability
of two randomly selected 128-bit values being the same is extremely low). This
does mean that a sequence of incrementing numbers would be classified as
"random", but at least it isn't flaky.

My recommendation is to use some form of iteration to improve the odds. If there
is a 25% probability to fail with 1 pass, then iterating that 10 times would
drop the chance to 0.0001%.

[padolph, 2013-12-04 22:22:13]

https://codereview.chromium.org/100593002/diff/20001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_openssl.cc (right):

https://codereview.chromium.org/100593002/diff/20001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_openssl.cc:171: *buffer =
blink::WebArrayBuffer::create(sym_key->key().size(), 1);
On 2013/12/04 19:04:00, eroman wrote:
> [optional] Consider extracting this to a helper in webcrypto_util (there are 3
> places which use it now). ex: CreateArrayBuffer(data, data_size).

Done.

https://codereview.chromium.org/100593002/diff/20001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/100593002/diff/20001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:123: // calculated entropy
is > 0.75, which solves to 0.2145 ~< p ~< 0.7855.
On 2013/12/04 19:04:00, eroman wrote:
> Doesn't this mean that truly random buffers will fail the heuristic 25% of the
> time?

No, the math does not work that way. However, I realize I did not
apply the correct technique here.

Binary entropy measures the 'randomness' of a given bit sequence.
The sequence with the most entropy is one that has an equal number
of 1's and 0's. A bit sequence with all 0's for example is not very
'random'.

However, what I am testing here is a single output of the PRNG. A
sample size of 1 can't say much about the quality of the PRNG, and
also as you note, we are interested in the variation of output to
output, not the variation in the bits in a single output. And it
is possible that the PRNG will occasionally produce a 'low binary
entropy' output of mostly 1's or mostly 0's too.

I apologize I did not think this through further.

What we really need is to compute the Shannon Entropy on a large sample set of
outputs from the PRNG. But I don't think that level
of diligence is required here. I think your approach of taking a
small sample set and just comparing each value will be sufficient
and is easy to do. I'll code that up and re-upload.

Thanks for your help!

[eroman, 2013-12-05 01:47:53]

lgtm

lgtm after these nits

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_nss.cc:338: SymKeyHandle* const
sym_key = reinterpret_cast<SymKeyHandle*>(key.handle());
I don't see any value in making the pointer "const" here.
I wouldn't object to "const SymKeyHandle*" however.

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_nss.cc:343: SECItem* const key_data =
PK11_GetKeyData(sym_key->key());
It is uncommon in chromium code to make the pointer itself const rather than a
pointer to const data. My suggestion is to remove it.

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_nss.cc:346: DCHECK(key_data->data);
Would it ever be possible for key_data->len to be 0?

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_openssl.cc (right):

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_openssl.cc:173:
webcrypto::CreateArrayBuffer(&sym_key->key()[0], sym_key->key().size());
Is it ever possible for the key length to be 0?

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:106: return
std::equal(a_begin, a_end, b_begin);
[no change necessary] My inclination would have been to write a 1 liner:

return a.byteLength() == b.byteLength() && memcmp(a.data(), b.data(),
a.byteLength() == 0);

Not sure how the boolean specialization of std::equal gets optimized, but
memcmp() is always pretty good.

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:125: for
(std::vector<blink::WebArrayBuffer>::const_iterator it = bufs.begin();
I much prefer the old-fashioned double-for-loop here:

for (size_t i = 0; i < bufs.size(); ++i) {
  for (size_t j = i + 1; j < bufs.size(); ++j) {
    if (ArrayBuffersEqual(bufs[i], bufs[j])
      return false;
  }
}

Which has the advantage of doing half as many arraybuffer comparisons, doesn't
require the ArrayBufferEqualTo helper class, and in my humble opinion is more
readable :)

[padolph, 2013-12-05 02:45:57]

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_nss.cc:338: SymKeyHandle* const
sym_key = reinterpret_cast<SymKeyHandle*>(key.handle());
On 2013/12/05 01:47:53, eroman wrote:
> I don't see any value in making the pointer "const" here.
> I wouldn't object to "const SymKeyHandle*" however.

The Scott Meyers books trained me to make as much stuff const as possible. It
can provide some safety in future modifications, in some (remote) cases can
allow compile time optimizations, and gives the reader the hint that I don't
plan on ever changing the value.

I can't use "const SymKeyHandle" in this case because PK11_ExtractKeyValue()
cannot support a const input. Strangely, it actually changes the underlying
PK11_SymKey structure, depositing the raw key bytes within it so they can then
be read by PK11_GetKeyData().

Anyway, I'll take out the const pointer stuff to maintain the style of the
chromium code as you mention below.

Done.

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_nss.cc:343: SECItem* const key_data =
PK11_GetKeyData(sym_key->key());
On 2013/12/05 01:47:53, eroman wrote:
> It is uncommon in chromium code to make the pointer itself const rather than a
> pointer to const data. My suggestion is to remove it.

Done.

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_nss.cc:346: DCHECK(key_data->data);
On 2013/12/05 01:47:53, eroman wrote:
> Would it ever be possible for key_data->len to be 0?

Yes. Empty symmetric keys are possible and already part of the unit tests.

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_openssl.cc (right):

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_openssl.cc:173:
webcrypto::CreateArrayBuffer(&sym_key->key()[0], sym_key->key().size());
On 2013/12/05 01:47:53, eroman wrote:
> Is it ever possible for the key length to be 0?

Yes. Empty symmetric keys are supported and already used in the unit tests.

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:106: return
std::equal(a_begin, a_end, b_begin);
On 2013/12/05 01:47:53, eroman wrote:
> [no change necessary] My inclination would have been to write a 1 liner:
> 
> return a.byteLength() == b.byteLength() && memcmp(a.data(), b.data(),
> a.byteLength() == 0);
> 
> Not sure how the boolean specialization of std::equal gets optimized, but
> memcmp() is always pretty good.

I'm sure std::equal optimizes down to memcmp in this case. But I like your
1-liner better.

Done.

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:125: for
(std::vector<blink::WebArrayBuffer>::const_iterator it = bufs.begin();
On 2013/12/05 01:47:53, eroman wrote:
> I much prefer the old-fashioned double-for-loop here:
> 
> for (size_t i = 0; i < bufs.size(); ++i) {
>   for (size_t j = i + 1; j < bufs.size(); ++j) {
>     if (ArrayBuffersEqual(bufs[i], bufs[j])
>       return false;
>   }
> }
> 
> Which has the advantage of doing half as many arraybuffer comparisons, doesn't
> require the ArrayBufferEqualTo helper class, and in my humble opinion is more
> readable :)

I can't argue with that. 1/3 the lines and faster. Hard to shake my STL habit.
Done.

[commit-bot: I haz the power, 2013-12-05 02:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/100593002/100001

[eroman, 2013-12-05 02:53:58]

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_openssl.cc (right):

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_openssl.cc:173:
webcrypto::CreateArrayBuffer(&sym_key->key()[0], sym_key->key().size());
On 2013/12/05 02:45:57, padolph wrote:
> On 2013/12/05 01:47:53, eroman wrote:
> > Is it ever possible for the key length to be 0?
> 
> Yes. Empty symmetric keys are supported and already used in the unit tests.

If the key is empty, then I don't think we should be taking the address of
element zero.

https://codereview.chromium.org/100593002/diff/100001/content/renderer/webcry...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/100593002/diff/100001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_impl_nss.cc:346: DCHECK(key_data->data);
If it is possible to have empty keys, then is this DCHECK valid? (i.e. data
could be NULL if length is 0).

[eroman, 2013-12-05 03:02:02]

Regarding the presubmit failure: I believe the commit queue is broken right now.

So we will just have to resubmit later once it is fixed :)

[padolph, 2013-12-05 03:34:15]

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
File content/renderer/webcrypto/webcrypto_impl_openssl.cc (right):

https://codereview.chromium.org/100593002/diff/80001/content/renderer/webcryp...
content/renderer/webcrypto/webcrypto_impl_openssl.cc:173:
webcrypto::CreateArrayBuffer(&sym_key->key()[0], sym_key->key().size());
On 2013/12/05 02:53:58, eroman wrote:
> On 2013/12/05 02:45:57, padolph wrote:
> > On 2013/12/05 01:47:53, eroman wrote:
> > > Is it ever possible for the key length to be 0?
> > 
> > Yes. Empty symmetric keys are supported and already used in the unit tests.
> 
> If the key is empty, then I don't think we should be taking the address of
> element zero.

Done.

https://codereview.chromium.org/100593002/diff/100001/content/renderer/webcry...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/100593002/diff/100001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_impl_nss.cc:346: DCHECK(key_data->data);
On 2013/12/05 02:53:58, eroman wrote:
> If it is possible to have empty keys, then is this DCHECK valid? (i.e. data
> could be NULL if length is 0).

Removed.

[commit-bot: I haz the power, 2013-12-05 03:37:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/100593002/120001

[commit-bot: I haz the power, 2013-12-05 03:55:27]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-12-05 16:52:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/100593002/120001

[commit-bot: I haz the power, 2013-12-05 17:46:31]

Failed to trigger a try job on android_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-12-05 17:47:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/100593002/130001

[commit-bot: I haz the power, 2013-12-05 22:22:57]

Change committed as 239049