Add a Verify routine for HMAC

Add a Verify routine for HMAC

BUG=none
TEST=crypto_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92824

[Ryan Sleevi, 2011-06-28 01:15:25]

wtc: PTAL. I noticed in http://crrev.com/90606 that SecureP2PSocket was
implementing HMAC verification within itself (
http://src.chromium.org/viewvc/chrome/trunk/src/remoting/protocol/secure_p2p_...
)

Given that the HMAC key of the SecureP2PSocket is long-lived, it would seem
possible that a timing attack might be mounted on the socket. While the attack
surface here is probably rather low, I wanted to err on the side of
defense-in-depth and provide a reliable implementation for verification.

This is a constant-time comparison, similar to NSS_SecureMemcmp or to KeyCzar's
implementation (mentioned in the code comment), implemented by xoring the two
digests together and seeing if any bits end up being set. I'm not aware of any
compiler optimizations that might confound this implementation - it should be
fine as is.

I haven't yet checked other callers of HMAC::Sign() to see if they might
disclose anything, just noticed it while I was seeing commits go by.

[Ryan Sleevi, 2011-07-13 00:33:25]

Ping?

[wtc, 2011-07-14 00:00:22]

LGTM.

http://codereview.chromium.org/7277024/diff/1/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/7277024/diff/1/crypto/hmac.cc#newcode43
crypto/hmac.cc:43: tmp |= *digest_ptr++ ^ *computed_ptr++;
We should add a SecureMemcmp function.

http://codereview.chromium.org/7277024/diff/1/crypto/hmac_unittest.cc
File crypto/hmac_unittest.cc (right):

http://codereview.chromium.org/7277024/diff/1/crypto/hmac_unittest.cc#newcode7
crypto/hmac_unittest.cc:7: #include "base/string_piece.h"
"base/string_piece.h" doesn't need to be included.

http://codereview.chromium.org/7277024/diff/1/crypto/hmac_unittest.cc#newcode37
crypto/hmac_unittest.cc:37: }
Add a blank line before this line.

Add a namespace comment:

}  // namespace

This is recommended by the Style Guide.

BUT: we don't need the anonymous namespace because
all of these constants are file static.  We should use
only one of anonymous namespace and static to make these
constants invisible outside this file.

[Ryan Sleevi, 2011-07-14 00:18:19]

http://codereview.chromium.org/7277024/diff/1/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/7277024/diff/1/crypto/hmac.cc#newcode43
crypto/hmac.cc:43: tmp |= *digest_ptr++ ^ *computed_ptr++;
On 2011/07/14 00:00:22, wtc wrote:
> We should add a SecureMemcmp function.

Was this remark for a high-level, crypto/ API, or just a function within this
file?

[wtc, 2011-07-14 00:49:42]

http://codereview.chromium.org/7277024/diff/1/crypto/hmac.cc
File crypto/hmac.cc (right):

http://codereview.chromium.org/7277024/diff/1/crypto/hmac.cc#newcode37
crypto/hmac.cc:37: //
http://groups.google.com/group/keyczar-discuss/browse_thread/thread/5571eca09...
This URL doesn't work for me.

http://codereview.chromium.org/7277024/diff/1/crypto/hmac.cc#newcode43
crypto/hmac.cc:43: tmp |= *digest_ptr++ ^ *computed_ptr++;
On 2011/07/14 00:18:19, Ryan Sleevi wrote:
>
> Was this remark for a high-level, crypto/ API, or just a function within this
> file?

Sorry I wasn't clear.  I meant a function that can be used
by other files.  We can do this when some other file also
need a constant-time memcmp().

[Ryan Sleevi, 2011-07-14 01:04:07]

wtc: Thanks for the clarification. I pulled it out into it's own function, with
the same variable names as memcmp, and cleaned up the for loop a little to
separate out the increment , deref, and xor

As far as the URL not working, I had the same problem when trying to right-click
load from the codereview, because of the newline introduced by wrapping. If you
copy/paste the URL though, you should find it loads fine. 

My primary intent was to provide more context about why, for something that
subtle, but I have no problem omitting it if the assumption is the reader
understands such attacks and the existing literature.

[commit-bot: I haz the power, 2011-07-18 00:39:46]

Change committed as 92824