crypto/hmac: Don't check length in Equal

[leonklingele]

As crypto/hmac.Equal is relying on crypto/subtle.ConstantTimeCompare, which already checks the length of the two byte slices, we do not need such a length check in crypto/hmac.Equal.

crypto/subtle.ConstantTimeCompare: https://github.com/golang/go/blob/master/src/crypto/subtle/constant_time.go#L12
crypto/hmac.Equal: https://github.com/golang/go/blob/master/src/crypto/hmac/hmac.go#L97

I think the length check was used in crypto/hmac.Equal as it was previously not included in crypto/subtle.ConstantTimeCompare.

[leonklingele]

Looks like this check was added to crypto/hmac.Equal as crypto/subtle.ConstantTimeCompare. was panicking before if the two slices had different lengths: 446bfff

[odeke-em]

Hello @leonklingele, I just took out the length check out in crypto/hmac and it seems it suffers when the lengths are unequal due to that obligatory function overhead of invoking subtle.ConstantTimeCompare. See the diffs below

Benchmark results

name                      old time/op    new time/op    delta
HMACSHA256_1K-4             4.26µs ± 0%    4.22µs ± 0%   ~     (p=1.000 n=1+1)
HMACSHA256_32-4             1.11µs ± 0%    1.09µs ± 0%   ~     (p=1.000 n=1+1)
HMACEqualEqualLength-4      14.4ns ± 0%    13.5ns ± 0%   ~     (p=1.000 n=1+1)
HMACEqualUnequalLength-4    3.41ns ± 0%    6.38ns ± 0%   ~     (p=1.000 n=1+1)

name                      old speed      new speed      delta
HMACSHA256_1K-4            241MB/s ± 0%   242MB/s ± 0%   ~     (p=1.000 n=1+1)
HMACSHA256_32-4           28.7MB/s ± 0%  29.3MB/s ± 0%   ~     (p=1.000 n=1+1)

Diffs

diff --git a/src/crypto/hmac/hmac.go b/src/crypto/hmac/hmac.go
index a748107..9ef9c44 100644
--- a/src/crypto/hmac/hmac.go
+++ b/src/crypto/hmac/hmac.go
@@ -94,5 +94,5 @@ func Equal(mac1, mac2 []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(mac1) == len(mac2) && subtle.ConstantTimeCompare(mac1, mac2) == 1
+   return subtle.ConstantTimeCompare(mac1, mac2) == 1
 }

diff --git a/src/crypto/hmac/hmac_test.go b/src/crypto/hmac/hmac_test.go
index aac9aa9..c2ae170 100644
--- a/src/crypto/hmac/hmac_test.go
+++ b/src/crypto/hmac/hmac_test.go
@@ -594,3 +594,19 @@ func BenchmarkHMACSHA256_32(b *testing.B) {
        buf[0] = mac[0]
    }
 }
+
+func BenchmarkHMACEqualEqualLength(b *testing.B) {
+   bs1 := []byte("testing1")
+   bs2 := []byte("tested1a")
+   for i := 0; i < b.N; i++ {
+       _ = Equal(bs1, bs2)
+   }
+}
+
+func BenchmarkHMACEqualUnequalLength(b *testing.B) {
+   bs1 := []byte("testing1")
+   bs2 := []byte("tested1")
+   for i := 0; i < b.N; i++ {
+       _ = Equal(bs1, bs2)
+   }
+}

[odeke-em]

@leonklingele I commented with the result while I was at a conference and hadn't completed my reply.
I had forgotten to mention that in the CL 446bfff aka CL https://golang.org/cl/118750043 that introduced crypto/hmac.Equal, @agl declared the intention that we don't have to be constant time if the lengths of the MACs ie https://github.com/golang/go/blob/master/src/crypto/hmac/hmac.go#L92-L98

If we make the suggested change, we'll even incur an unnecessary penalty of a function call that just performs the same check and eventually rejects it, as you've seen from the benchmarks above. Therefore, I don't think we should change this, but I'll leave it to the discretion of @agl to close the issue.

[agl]

It's certainly true that we don't have to run in constant time if the inputs lengths are unequal. (I don't think that's even possible.) It's a weird corner case however, so I wouldn't optimise for it either. I guess it's a very minor reduction in code size to drop the check and I'll put this on my list for when Go 1.8 opens.

[gopherbot]

CL https://golang.org/cl/27239 mentions this issue.