Allow empty keys in hmac_openssl.cc.

Allow empty keys in hmac_openssl.cc.

PrefHashCalculator uses empty keys in developer builds. This fixes
Chrome startup in debug builds.

BUG=none

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

[davidben, 2014-04-09 20:10:42]

Okay, updated to not run afoul of OpenSSL's screwy handling of NULL keys. Also
added a test to make sure NSS and OpenSSL interoperate on this (totally useless,
were it not exercised in dev builds) case. Conveniently Wikipedia had a test
vector.

[wtc, 2014-04-09 21:30:58]

Patch set 3 LGTM.

Please ask the owner of IDR_PREF_HASH_SEED_BIN whether he or she realizes that
the resource is empty in Developer builds, and whether we should use an empty
seed or a fixed seed of the right length in that case.

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_openssl.cc
File crypto/hmac_openssl.cc (right):

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_openssl.cc#n...
crypto/hmac_openssl.cc:37: // equivalent.

It would be nice to note what a NULL key means to OpenSSL.

I confirm that an empty key and a key of one zero byte are equivalent.

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_openssl.cc#n...
crypto/hmac_openssl.cc:56: &plat_->key[0], plat_->key.size(),

Can you try passing a non-NULL key pointer and a zero key size to ::HMAC()? Does
that work?

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_unittest.cc
File crypto/hmac_unittest.cc (right):

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_unittest.cc#...
crypto/hmac_unittest.cc:289: EXPECT_TRUE(hmac.Sign("", digest,
kSHA1DigestSize));

Please use base::StringPiece() instead of "" here and in the hmac.Verify() call,
to eliminate the doubt that the null terminator in an empty C string may get
used.

[davidben, 2014-04-09 21:44:28]

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_openssl.cc
File crypto/hmac_openssl.cc (right):

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_openssl.cc#n...
crypto/hmac_openssl.cc:37: // equivalent.
On 2014/04/09 21:30:58, wtc wrote:
> 
> It would be nice to note what a NULL key means to OpenSSL.
> 
> I confirm that an empty key and a key of one zero byte are equivalent.

Done.

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_openssl.cc#n...
crypto/hmac_openssl.cc:56: &plat_->key[0], plat_->key.size(),
On 2014/04/09 21:30:58, wtc wrote:
> 
> Can you try passing a non-NULL key pointer and a zero key size to ::HMAC()?
Does
> that work?

That works too; if the key size is 0, it never actually dereferences the key,
assuming memcpy(dst, key, 0) doesn't. So something like casting "" to an
unsigned char* would work fine too. This lets us keep the DCHECK and avoid the
cast, but the alternative doesn't make assumptions about key padding. *shrug* I
don't have much preference over which to use.

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_unittest.cc
File crypto/hmac_unittest.cc (right):

https://codereview.chromium.org/231603002/diff/40001/crypto/hmac_unittest.cc#...
crypto/hmac_unittest.cc:289: EXPECT_TRUE(hmac.Sign("", digest,
kSHA1DigestSize));
On 2014/04/09 21:30:58, wtc wrote:
> 
> Please use base::StringPiece() instead of "" here and in the hmac.Verify()
call,
> to eliminate the doubt that the null terminator in an empty C string may get
> used.

Done. Amusingly, I had to use base::StringPiece("", 0) rather than
base::StringPiece(NULL, 0). The latter upsets NSS. I think I have to conclude C
code by and large doesn't deal with empty byte arrays very well. :-)

[wtc, 2014-04-09 22:10:48]

Patch set 4 LGTM. We should fix the NSS problem.

https://codereview.chromium.org/231603002/diff/50001/crypto/hmac_unittest.cc
File crypto/hmac_unittest.cc (right):

https://codereview.chromium.org/231603002/diff/50001/crypto/hmac_unittest.cc#...
crypto/hmac_unittest.cc:293: "", base::StringPiece(kExpectedDigest,
kSHA1DigestSize)));

We should also pass a StringPiece here. I suggest we use a local variable:

  base::StringPiece key("", 0);

and pass |key| to hmac.Sign() and hmac.Verify().

[davidben, 2014-04-09 23:01:52]

> We should fix the NSS problem.

Might just be as easy as removing this block:
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11c...

Also changed 0 to 0u in hopes that appeases Android's STL. It's complaining it
can't tell if I'm using the iterator or the ptr/len StringPiece constructor. Its
std::string::const_iterator is just a const char*, so probably this is hitting
the weird C++ special-case where the 0 literal can cast into a pointer. (Or
something like that... I don't know the details, but I recall there's some weird
kludge here. In C++11 we have nullptr with a dedicated type which resolves
this.)

https://codereview.chromium.org/231603002/diff/50001/crypto/hmac_unittest.cc
File crypto/hmac_unittest.cc (right):

https://codereview.chromium.org/231603002/diff/50001/crypto/hmac_unittest.cc#...
crypto/hmac_unittest.cc:293: "", base::StringPiece(kExpectedDigest,
kSHA1DigestSize)));
On 2014/04/09 22:10:49, wtc wrote:
> 
> We should also pass a StringPiece here. I suggest we use a local variable:
> 
>   base::StringPiece key("", 0);
> 
> and pass |key| to hmac.Sign() and hmac.Verify().

Oh, missed that one. Done.

[wtc, 2014-04-10 18:43:41]

Patch set 6 LGTM.

https://codereview.chromium.org/231603002/diff/80001/crypto/hmac_unittest.cc
File crypto/hmac_unittest.cc (right):

https://codereview.chromium.org/231603002/diff/80001/crypto/hmac_unittest.cc#...
crypto/hmac_unittest.cc:284: base::StringPiece data("", 0u);

You can also try using a std::string:

  std::string data;

My experiments show that data.data() is a non-null pointer on Linux and Windows.
Your current solution is fine, too.

[davidben, 2014-04-10 18:54:29]

https://codereview.chromium.org/231603002/diff/80001/crypto/hmac_unittest.cc
File crypto/hmac_unittest.cc (right):

https://codereview.chromium.org/231603002/diff/80001/crypto/hmac_unittest.cc#...
crypto/hmac_unittest.cc:284: base::StringPiece data("", 0u);
On 2014/04/10 18:43:41, wtc wrote:
> 
> You can also try using a std::string:
> 
>   std::string data;
> 
> My experiments show that data.data() is a non-null pointer on Linux and
Windows.
> Your current solution is fine, too.

Yeah, per C++11, std::string::data and std::string::c_str are the same and both
include the trailing NUL. I think it's actually not required to in C++03, but
putting the NUL byte in when calling c_str would be insane so I doubt anyone
does that. Though std::string::data apparently always returns non-NULL per C++03
too:

"If size() is zero, the member returns a non-null pointer that is copyable and
can have zero added to it"

which does make me wonder what that pointer is supposed to point to... something
random?


Anyway... empty byte arrays. They are ridiculous. :-)

[davidben, 2014-04-10 18:54:41]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-04-10 18:55:21]

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

[commit-bot: I haz the power, 2014-04-10 21:17:02]

Change committed as 263085