crypto/hmac: detect reuse of hash.Hash value

[dvsekhvalnov]

Good time of the day,

with go 1.15 release it seems there is some regression issue introduced with respect to crypto/hmac package.

Was troubleshooting dvsekhvalnov/jose2go#26 and found that hmac.Reset() is no longer behave same way as it was before (at least when called first), crafted minimal test case:

package main

import (
	"crypto/hmac"
	"crypto/sha256"
	"fmt"
	"hash"
)

func main() {
	sha := sha256.New()
	hmac := hmac.New(func() hash.Hash { return sha }, []byte("anything"))

	hmac.Reset()        // if you try to comment / uncomment that line, go 1.15 will produce different results
	hmac.Write([]byte("salt"))

	test := hmac.Sum(nil)

	fmt.Printf("test = %v\n", test)
}

Is it producing output:

1. [169 238 50 31 23 163 57 12 228 112 77 219 51 95 12 6 185 17 156 244 116 243 186 227 89 79 64 19 227 242 86 186] on go v1.15
2. [213 21 105 177 57 151 62 247 23 137 16 75 59 26 241 187 229 148 88 219 30 222 223 77 186 120 81 74 247 237 232 66] on go v.14 and below

, more over toggling leading hmac.Reset() will produce different results with 1.15 vs. previous versions.

It seems change was introduced by 97240d5
So will tag @magical and @FiloSottile here.

Honestly i don't know may be calling hmac.Reset before everything else is not sane idea, but it's not documented anywhere and clearly was behaving differently before.

Thank you.

[FiloSottile]

Thank you for the report, this should absolutely work and is a regression. @gopherbot, open a Go 1.15 backport issue, please.

[gopherbot]

Backport issue(s) opened: #41097 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[FiloSottile]

Hmm, at a first look I had not noticed that the first argument to New is func() hash.Hash { return sha }, a function that returns always the same hash instance. That is not how it's meant to be used, it should be func() hash.Hash { return sha256.New() } or simply sha256.New.

Now I'm surprised this ever worked, because that makes the inner and outer hash share state.

With the correct argument, calling Reset before any input works as expected. https://play.golang.org/p/PbneuUK9F0Y

[FiloSottile]

This sort of used to work because a single HMAC used to serialize to these operations

h.inner.Write(hm.ipad)      // New
h.inner.Reset()             // Reset
h.inner.Write(hm.ipad)      // Reset
h.inner.Write(p)            // Write
h.inner.Sum(in)             // Sum
h.outer.Reset()             // Sum
h.outer.Write(h.opad)       // Sum
h.outer.Write(in[origLen:]) // Sum
h.outer.Sum(in[:origLen])   // Sum

unless the key was longer than the blocksize (in which case outer gets used in New).

If you tried calling Sum a second time, it would also have broken, because inner gets corrupted by the use of outer.

So this never worked. We could add a panic to make sure the returned values are different, so no one else makes this mistake.

[dvsekhvalnov]

Thanks @FiloSottile ! Changing from value to New func made it.

[katiehockman]

We're going to panic if inner and outer are the same in the hmac.New function, indicating that the hash function doesn't produce unique outputs.

[gopherbot]

Change https://golang.org/cl/261960 mentions this issue: crypto/hmac: panic if reusing hash.Hash values

[magical]

SGTM. My only worry is that the check may panic if the underlying values are not comparable, but that seems unlikely to ever come up in practice.

e.g. https://play.golang.org/p/ZtuAvkQZMFX

[katiehockman]

@magical Thanks for pointing that out. I went ahead and updated the PR to reflect that concern.