code review 6632044: crypto/hmac: add Verify function.

crypto/hmac: add Equal function.

It was suggested that it's too easy to use crypto/hmac insecurely and
I think that has some merit. This change adds a Equal function to
make it obvious that MAC values should be compared in constant time.

[agl1, 2012-10-08 18:31:00]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-10-09 15:43:28]

Even though it's trivial, a test or two would be nice.

The deps_test changes are fine.

https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go
File src/pkg/crypto/hmac/hmac.go (right):

https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go#...
src/pkg/crypto/hmac/hmac.go:10: Receivers should be careful to use Verify to
compare MACs in order to avoid
new paragraph here (insert blank line above this one)

https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go#...
src/pkg/crypto/hmac/hmac.go:95: // Verify compares two MACs for equality without
leaking timing information.
I'd prefer to use a name that is more clearly boolean. Also the variables could
be named to make clearer that the arguments are the MACs.

func Equal(mac1, mac2 []byte) bool 
?

On a related note, I was expecting Verify to be more of a helper. If you want to
make it dead simple to use correctly, I wonder if you should add:

func Sign(h func() hash.Hash, key, msg []byte) (mac []byte) 
func Verify(h func() hash.Hash, key, msg, mac []byte) error

[agl1, 2012-10-09 17:15:23]

https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go
File src/pkg/crypto/hmac/hmac.go (right):

https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go#...
src/pkg/crypto/hmac/hmac.go:10: Receivers should be careful to use Verify to
compare MACs in order to avoid
On 2012/10/09 15:43:28, rsc wrote:
> new paragraph here (insert blank line above this one)

Done.

https://codereview.appspot.com/6632044/diff/2002/src/pkg/crypto/hmac/hmac.go#...
src/pkg/crypto/hmac/hmac.go:95: // Verify compares two MACs for equality without
leaking timing information.
On 2012/10/09 15:43:28, rsc wrote:
> I'd prefer to use a name that is more clearly boolean. Also the variables
could
> be named to make clearer that the arguments are the MACs.
> 
> func Equal(mac1, mac2 []byte) bool 
> ?

Yes, that's clearly better. Thanks.
 
> func Sign(h func() hash.Hash, key, msg []byte) (mac []byte) 
> func Verify(h func() hash.Hash, key, msg, mac []byte) error

The problem is that it's very common (almost required) to hash in additional
information to the HMAC. If the interface only supports passing in a single
slice, people will either work around it or they'll have to alloc and copy a new
slice.

[mxk, 2012-10-09 18:16:37]

On Tue, Oct 9, 2012 at 1:15 PM,  <agl@golang.org> wrote:
>> func Sign(h func() hash.Hash, key, msg []byte) (mac []byte)
>> func Verify(h func() hash.Hash, key, msg, mac []byte) error
>
>
> The problem is that it's very common (almost required) to hash in
> additional information to the HMAC. If the interface only supports
> passing in a single slice, people will either work around it or they'll
> have to alloc and copy a new slice.

I'm currently working on a project that relies on the HMAC functions.
I have a read-verify-read-verify-... loop, which is further
complicated by the fact that certain HMAC comparisons are truncated to
some number of most significant bytes. I think a single set of signing
and verification functions is never going to fit all use cases, but a
reminder that the comparison needs to be done in constant time is very
useful.

If you want a function that does more than a simple comparison of two
byte slices, I would do something like this (what I use in my code):

// Verify compares the HMAC tag in b with the current hash value
// returned by h, truncated to t bytes. If len(b) > t, all preceding
// bytes are written to h before the comparison. h.Size() is used
// for t if t <= 0.
func Verify(b []byte, t int, h hash.Hash)

You can obviously remove t if you think that complicates the interface
beyond what is acceptable in the standard library, but truncation is a
normal part of HMAC operations (RFC 2104 section 5).

[agl1, 2012-10-09 21:01:12]

On Tue, Oct 9, 2012 at 2:16 PM, Maxim Khitrov <max@mxcrypt.com> wrote:
> // Verify compares the HMAC tag in b with the current hash value
> // returned by h, truncated to t bytes. If len(b) > t, all preceding
> // bytes are written to h before the comparison. h.Size() is used
> // for t if t <= 0.
> func Verify(b []byte, t int, h hash.Hash)
>
> You can obviously remove t if you think that complicates the interface
> beyond what is acceptable in the standard library, but truncation is a
> normal part of HMAC operations (RFC 2104 section 5).

I quite like that too, but I worry that it precludes avoiding the
allocation each time when calling Sum. Without this function, users
would are verifying many MACs can reuse storage. But, since
escape-analysis isn't going to work through the interface, I think
this would have to allocate every time.


Cheers

AGL

[rsc, 2012-10-09 21:05:01]

LGTM

It's fine to leave Sign and Verify out.

[agl1, 2012-10-11 19:28:23]

*** Submitted as http://code.google.com/p/go/source/detail?r=18dffd0c07b2 ***

crypto/hmac: add Equal function.

It was suggested that it's too easy to use crypto/hmac insecurely and
I think that has some merit. This change adds a Equal function to
make it obvious that MAC values should be compared in constant time.

R=rsc, max
CC=golang-dev
http://codereview.appspot.com/6632044