Skip to content
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

crypto/hmac: Don't check length in Equal #16336

Closed
leonklingele opened this issue Jul 12, 2016 · 5 comments
Closed

crypto/hmac: Don't check length in Equal #16336

leonklingele opened this issue Jul 12, 2016 · 5 comments

Comments

@leonklingele
Copy link
Contributor

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
Copy link
Contributor Author

leonklingele commented Jul 12, 2016

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
Copy link
Member

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)
+   }
+}

@quentinmit quentinmit changed the title Don't check length in crypto/hmac.Equal crypto/hmac: Don't check length in Equal Jul 19, 2016
@quentinmit quentinmit added this to the Go1.8Maybe milestone Jul 19, 2016
@odeke-em
Copy link
Member

odeke-em commented Aug 1, 2016

@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
screen shot 2016-07-31 at 11 58 11 pm

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
Copy link
Contributor

agl commented Aug 7, 2016

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.

@agl agl self-assigned this Aug 7, 2016
@gopherbot
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants