-
Notifications
You must be signed in to change notification settings - Fork 18k
crypto/hmac: Don't check length in Equal #16336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Looks like this check was added to |
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 Benchmark resultsname 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) Diffsdiff --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)
+ }
+} |
@leonklingele I commented with the result while I was at a conference and hadn't completed my reply. 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. |
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. |
CL https://golang.org/cl/27239 mentions this issue. |
As
crypto/hmac.Equal
is relying oncrypto/subtle.ConstantTimeCompare
, which already checks the length of the two byte slices, we do not need such a length check incrypto/hmac.Equal
.crypto/subtle.ConstantTimeCompare
: https://github.com/golang/go/blob/master/src/crypto/subtle/constant_time.go#L12crypto/hmac.Equal
: https://github.com/golang/go/blob/master/src/crypto/hmac/hmac.go#L97I think the length check was used in
crypto/hmac.Equal
as it was previously not included incrypto/subtle.ConstantTimeCompare
.The text was updated successfully, but these errors were encountered: