Add constant-time comparison operators for cryptographic uses.

Make constant-time comparison operators for cryptographic uses public.

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

[Ryan Sleevi, 2011-10-03 23:45:33]

I'm inclined to believe this may be better suited under crypto/ then base/. The
only reason for it being in base/ would be if something in base/ actually needed
to use this (as base/ cannot depend on crypto/). This is the underlying reason
for SHA-1/MD-5 still living under base/, but SHA-256 living under crypto/.

You should also look to update crypto/hmac.cc:SecureMemCmp() to use this
routine.

[Chris Palmer, 2011-10-03 23:57:38]

> You should also look to update crypto/hmac.cc:SecureMemCmp() to use this
> routine.

Oh, it already does. I'll just use that, and trash this CL. Thanks!

[Chris Palmer, 2011-10-04 00:03:05]

Well, rather than trashing the CL, I'm re-using it. Turns out I had to make
SecureMemcmp public. So here that is.

[Ryan Sleevi, 2011-10-04 00:10:14]

Right. wtc expressed the same feeling in the original review - that it should be
made public - if/when the time comes that other code needs fixed-time
comparisons ( http://codereview.chromium.org/7277024#msg5 )

I would think it might be best to pull this function out into it's own header -
as it sounds like you're not doing anything related to HMAC (otherwise you could
use Verify() or VerifyTruncated()).

crypto/secure_memcmp.h perhaps? 

Don't forget to include CRYPTO_EXPORT if this method is intended to be used
outside of crypto/ - otherwise it will break shared builds.

[wtc, 2011-10-04 00:32:10]

http://codereview.chromium.org/8124011/diff/4001/crypto/hmac.h
File crypto/hmac.h (right):

http://codereview.chromium.org/8124011/diff/4001/crypto/hmac.h#newcode81
crypto/hmac.h:81: bool SecureMemcmp(const void* s1, const void* s2, size_t n);
This function is not related to HMAC.  We should create a new
header for this function, and perhaps a SecureMemset function
that won't be optimized away by the compiler.

[Chris Palmer, 2011-10-04 00:37:03]

wtc, looks like you did a review right before I uploaded a new rev of the CL. :)

[Ryan Sleevi, 2011-10-04 00:38:28]

LGTM, modulo the namespace issue below.

http://codereview.chromium.org/8124011/diff/5001/crypto/secure_memcmp.cc
File crypto/secure_memcmp.cc (right):

http://codereview.chromium.org/8124011/diff/5001/crypto/secure_memcmp.cc#newc...
crypto/secure_memcmp.cc:6: 
missing namespace crypto {} here

[Chris Palmer, 2011-10-04 00:40:30]

http://codereview.chromium.org/8124011/diff/5001/crypto/secure_memcmp.cc
File crypto/secure_memcmp.cc (right):

http://codereview.chromium.org/8124011/diff/5001/crypto/secure_memcmp.cc#newc...
crypto/secure_memcmp.cc:6: 
On 2011/10/04 00:38:28, Ryan Sleevi wrote:
> missing namespace crypto {} here

Done.

[wtc, 2011-10-04 00:58:22]

LGTM.  It is wasteful to use a header for just one function.
You may want to consider crypto/crypto_util.{h,cc} or
crypto/secure_util.{hc,cc}.

[Chris Palmer, 2011-10-04 16:44:56]

> LGTM.  It is wasteful to use a header for just one function.
> You may want to consider crypto/crypto_util.{h,cc} or
> crypto/secure_util.{hc,cc}.

Done.

[wtc, 2011-10-04 19:17:08]

LGTM on Patch Set 9.

http://codereview.chromium.org/8124011/diff/9001/crypto/secure_util.h
File crypto/secure_util.h (right):

http://codereview.chromium.org/8124011/diff/9001/crypto/secure_util.h#newcode9
crypto/secure_util.h:9: #include "base/basictypes.h"
Nit: for the definition of the size_t type, it is enough to
include <stddef.h>.

[Chris Palmer, 2011-10-04 19:20:20]

> crypto/secure_util.h:9: #include "base/basictypes.h"
> Nit: for the definition of the size_t type, it is enough to
> include <stddef.h>.

Done.

[wtc, 2011-10-04 19:49:06]

http://codereview.chromium.org/8124011/diff/11002/crypto/secure_util.h
File crypto/secure_util.h (right):

http://codereview.chromium.org/8124011/diff/11002/crypto/secure_util.h#newcode25
crypto/secure_util.h:25: CRYPTO_EXPORT bool SecureMemcmp(const void* s1, const
void* s2, size_t n);
When reviewing your patch for SHA1Fingerprint, I noticed that
SecureMemcmp returns bool.  So the function prototype is
different from that of memcmp.

Is it possible to define a SecureMemcmp function that returns
int with the same meaning as memcmp?  If not, perhaps we should
rename this function SecureMemEqual.

[Chris Palmer, 2011-10-04 20:01:07]

> If not, perhaps we should
> rename this function SecureMemEqual.

Done.

[commit-bot: I haz the power, 2011-10-05 22:09:49]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/8124011/14001

[commit-bot: I haz the power, 2011-10-05 23:46:29]

Try job failure for 8124011-14001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-10-06 23:48:17]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/8124011/14001

[commit-bot: I haz the power, 2011-10-07 02:24:22]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 104418, job name
was 8124011-14001 (retry) (previous was lost) (previous was lost) (previous was
lost).

[commit-bot: I haz the power, 2011-10-07 02:27:15]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/8124011/14001

[commit-bot: I haz the power, 2011-10-07 03:29:43]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 104430, job name
was 8124011-14001 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-10-07 15:30:53]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/8124011/14001

[commit-bot: I haz the power, 2011-10-07 17:02:50]

Change committed as 104502