Grokbase › Groups › Go › golang-dev › October 2012
FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello [email protected] (cc: [email protected]),

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


Description:
crypto/hmac: add Verify 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 Verify function to
make it obvious that MAC values should be compared in constant time.

Please review this at http://codereview.appspot.com/6632044/

Affected files:
M src/pkg/crypto/hmac/hmac.go
M src/pkg/go/build/deps_test.go


Index: src/pkg/crypto/hmac/hmac.go
===================================================================
--- a/src/pkg/crypto/hmac/hmac.go
+++ b/src/pkg/crypto/hmac/hmac.go
@@ -2,13 +2,26 @@
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

-// Package hmac implements the Keyed-Hash Message Authentication Code
(HMAC) as
-// defined in U.S. Federal Information Processing Standards Publication
198.
-// An HMAC is a cryptographic hash that uses a key to sign a message.
-// The receiver verifies the hash by recomputing it using the same key.
+/*
+Package hmac implements the Keyed-Hash Message Authentication Code (HMAC)
as
+defined in U.S. Federal Information Processing Standards Publication 198.
+An HMAC is a cryptographic hash that uses a key to sign a message.
+The receiver verifies the hash by recomputing it using the same key.
+Receivers should be careful to use Verify to compare MACs in order to avoid
+timing side-channels:
+
+ // CheckMAC returns true if messageMAC is a valid HMAC tag for message.
+ func CheckMAC(message, messageMAC, key []byte) bool {
+ mac := hmac.New(sha256.New, key)
+ mac.Write(message)
+ expectedMAC := mac.Sum(nil)
+ return hmac.Verify(messageMAC, expectedMAC)
+ }
+*/
package hmac

import (
+ "crypto/subtle"
"hash"
)

@@ -78,3 +91,11 @@
hm.Reset()
return hm
}
+
+// Verify compares two MACs for equality without leaking timing
information.
+func Verify(a, b []byte) bool {
+ // We don't have to be constant time if the lengths of the MACs are
+ // different as that suggests that a completely different hash function
+ // was used.
+ return len(a) == len(b) && subtle.ConstantTimeCompare(a, b) == 1
+}
Index: src/pkg/go/build/deps_test.go
===================================================================
--- a/src/pkg/go/build/deps_test.go
+++ b/src/pkg/go/build/deps_test.go
@@ -249,18 +249,23 @@
"net/mail": {"L4", "NET", "OS"},
"net/textproto": {"L4", "OS", "net"},

+ // Support libraries for crypto that aren't L2.
+ "CRYPTO-SUPPORT": {
+ "crypto/subtle",
+ },
+
// Core crypto.
"crypto/aes": {"L3"},
"crypto/des": {"L3"},
- "crypto/hmac": {"L3"},
+ "crypto/hmac": {"L3", "CRYPTO-SUPPORT"},
"crypto/md5": {"L3"},
"crypto/rc4": {"L3"},
"crypto/sha1": {"L3"},
"crypto/sha256": {"L3"},
"crypto/sha512": {"L3"},
- "crypto/subtle": {"L3"},

"CRYPTO": {
+ "CRYPTO-SUPPORT",
"crypto/aes",
"crypto/des",
"crypto/hmac",
@@ -269,7 +274,6 @@
"crypto/sha1",
"crypto/sha256",
"crypto/sha512",
- "crypto/subtle",
},

// Random byte, number generation.

Search Discussions

  • Rsc at Oct 9, 2012 at 5:28 pm ⇧
    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#newcode10
    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#newcode95
    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

    https://codereview.appspot.com/6632044/
  • Agl at Oct 9, 2012 at 6:19 pm ⇧
    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#newcode10
    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#newcode95
    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.

    https://codereview.appspot.com/6632044/
  • Maxim Khitrov at Oct 9, 2012 at 7:20 pm ⇧

    On Tue, Oct 9, 2012 at 1:15 PM, 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).
  • Adam Langley at Oct 9, 2012 at 9:23 pm ⇧

    On Tue, Oct 9, 2012 at 2:16 PM, Maxim Khitrov 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 at Oct 9, 2012 at 11:57 pm ⇧
    LGTM

    It's fine to leave Sign and Verify out.


    https://codereview.appspot.com/6632044/
  • Agl at Oct 11, 2012 at 7:28 pm ⇧
    *** 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


    http://codereview.appspot.com/6632044/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 8, '12 at 6:36p
activeOct 11, '12 at 7:28p
posts7
users3
websitegolang.org

3 users in discussion

Agl: 4 posts Rsc: 2 posts Maxim Khitrov: 1 post

Content

People

Support

Translate

site design / logo © 2023 Grokbase