crypto/hmac: detect reuse of hash.Hash value #41089

dvsekhvalnov · 2020-08-28T09:24:37Z

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:

[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
[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.

The text was updated successfully, but these errors were encountered:

FiloSottile · 2020-08-28T16:06:04Z

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

gopherbot · 2020-08-28T16:06:10Z

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 · 2020-08-28T16:11:20Z

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 · 2020-08-28T16:23:44Z

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 · 2020-08-28T18:51:44Z

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

golang/go#41089

katiehockman · 2020-09-22T16:12:50Z

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 · 2020-10-13T20:36:54Z

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

magical · 2020-10-13T21:38:42Z

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 · 2020-10-15T16:38:40Z

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

This resolves a problem that arises with Go 1.16 where the hmac API has been changed to detect passed hash functions that return a static value instead of distinguish values: golang/go#41089 golang/go@2a206c7#diff-0b6881eef540c340af6cd0a8d8258c625f8c950ff091a22abd0778c4dad5dcaaR138-R144 The new keyring version internally pulls in a bumped jose2go version, which is required to properly handle the passed hmac hash function. dvsekhvalnov/jose2go@85193dd Signed-off-by: Levente Polyak <levente@leventepolyak.net>

Fixes goharbor#14932 Harbor recompiles the notary v0.6.1 with go 1.15 from v2.2.0, which introduces an break change that leads to notary key not found after migration. [Root cause] Notary v0.6.1 consumed an old version dvsekhvalnov/jose2, which is not compatible with go 1.15. [References] dvsekhvalnov/jose2go#26 golang/go#41089 [Resolve] To resolve this issue, we have to roll back go vesrion to v1.14 for notary v0.6.1 binary and keep it until upstream have a patch release to support go 1.15 or above. [Break change] If you're already on Harbor v2.2.0 ~ v2.2.2 and have signed images with notary, you will encouter the same issue after migrate to v2.2.3(or above) or v2.3.1(or above) because of the go version downgrade. We will have a FAQ to help you to resovle this particular scenario. The influence path of the particular case: Harbor v2.1.0(or lower) --> [v2.2.0 ~ v2.2.2] --> v2.2.3(or above) Harbor v2.1.0(or lower) --> v2.3.0 --> v2.3.1(or above) The non influence path of the paticular case: Harbor v2.1.0(or lower) --> v2.2.3(or above) Harbor v2.1.0(or lower) --> v2.3.1(or above) [Fix in Version] Harbor v2.2.3 or above Harbor v2.3.1 or above [Note] If you're a heavy user of notary, avoid using v2.2.0, v2.2.1, v2.2.2 and v2.3.0, and use the fixed version for instead. Signed-off-by: Wang Yan <wangyan@vmware.com>

Fixes goharbor#14932 Harbor recompiles the notary v0.6.1 with go 1.15 from v2.2.0, which introduces an break change that leads to notary key not found after migration. [Root cause] Notary v0.6.1 consumed an old version dvsekhvalnov/jose2, which is not compatible with go 1.15. [References] dvsekhvalnov/jose2go#26 golang/go#41089 [Resolve] To resolve this issue, we have to roll back go vesrion to v1.14 for notary v0.6.1 binary and keep it until upstream have a patch release to support go 1.15 or above. [Break change] If you pushed and signed image using Harbor v2.2.0 ~ v2.2.2 and created new repository key in notary, you will encouter the same issue after migrate to v2.2.3(or above) or v2.3.1(or above) because of the go version downgrade. We will have a FAQ to help you to resovle this particular scenario. The influence path of the particular case: Harbor v2.1.0(or lower) --> [v2.2.0 ~ v2.2.2] --> v2.2.3(or above) Harbor v2.1.0(or lower) --> v2.3.0 --> v2.3.1(or above) The non influence path of the paticular case: Harbor v2.1.0(or lower) --> v2.2.3(or above) Harbor v2.1.0(or lower) --> v2.3.1(or above) [Fix in Version] Harbor v2.2.3 or above Harbor v2.3.1 or above [Note] If you're a heavy user of notary, avoid using v2.2.0, v2.2.1, v2.2.2 and v2.3.0, and use the fixed version for instead. Signed-off-by: Wang Yan <wangyan@vmware.com>

Fixes #14932 Harbor recompiles the notary v0.6.1 with go 1.15 from v2.2.0, which introduces an break change that leads to notary key not found after migration. [Root cause] Notary v0.6.1 consumed an old version dvsekhvalnov/jose2, which is not compatible with go 1.15. [References] dvsekhvalnov/jose2go#26 golang/go#41089 [Resolve] To resolve this issue, we have to roll back go vesrion to v1.14 for notary v0.6.1 binary and keep it until upstream have a patch release to support go 1.15 or above. [Break change] If you pushed and signed image using Harbor v2.2.0 ~ v2.2.2 and created new repository key in notary, you will encouter the same issue after migrate to v2.2.3(or above) or v2.3.1(or above) because of the go version downgrade. We will have a FAQ to help you to resovle this particular scenario. The influence path of the particular case: Harbor v2.1.0(or lower) --> [v2.2.0 ~ v2.2.2] --> v2.2.3(or above) Harbor v2.1.0(or lower) --> v2.3.0 --> v2.3.1(or above) The non influence path of the paticular case: Harbor v2.1.0(or lower) --> v2.2.3(or above) Harbor v2.1.0(or lower) --> v2.3.1(or above) [Fix in Version] Harbor v2.2.3 or above Harbor v2.3.1 or above [Note] If you're a heavy user of notary, avoid using v2.2.0, v2.2.1, v2.2.2 and v2.3.0, and use the fixed version for instead. Signed-off-by: Wang Yan <wangyan@vmware.com>

Fixes goharbor#14932 Harbor recompiles the notary v0.6.1 with go 1.15 from v2.2.0, which introduces an break change that leads to notary key not found after migration. [Root cause] Notary v0.6.1 consumed an old version dvsekhvalnov/jose2, which is not compatible with go 1.15. [References] dvsekhvalnov/jose2go#26 golang/go#41089 [Resolve] To resolve this issue, we have to roll back go vesrion to v1.14 for notary v0.6.1 binary and keep it until upstream have a patch release to support go 1.15 or above. [Break change] If you pushed and signed image using Harbor v2.2.0 ~ v2.2.2 and created new repository key in notary, you will encouter the same issue after migrate to v2.2.3(or above) or v2.3.1(or above) because of the go version downgrade. We will have a FAQ to help you to resovle this particular scenario. The influence path of the particular case: Harbor v2.1.0(or lower) --> [v2.2.0 ~ v2.2.2] --> v2.2.3(or above) Harbor v2.1.0(or lower) --> v2.3.0 --> v2.3.1(or above) The non influence path of the paticular case: Harbor v2.1.0(or lower) --> v2.2.3(or above) Harbor v2.1.0(or lower) --> v2.3.1(or above) [Fix in Version] Harbor v2.2.3 or above Harbor v2.3.1 or above [Note] If you're a heavy user of notary, avoid using v2.2.0, v2.2.1, v2.2.2 and v2.3.0, and use the fixed version for instead. Signed-off-by: Wang Yan <wangyan@vmware.com>

ALTree changed the title ~~"crypto/hmac" on 1.15 is no longer compatible with previous verisons?~~ crypto/hmac: 1.15 change in behaviour of the Reset method Aug 28, 2020

ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 28, 2020

gopherbot mentioned this issue Aug 28, 2020

crypto/hmac: 1.15 change in behaviour of the Reset method [1.15 backport] #41097

Closed

FiloSottile added Documentation NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 28, 2020

FiloSottile added this to the Go1.16 milestone Aug 28, 2020

FiloSottile changed the title ~~crypto/hmac: 1.15 change in behaviour of the Reset method~~ crypto/hmac: detect reuse of hash.Hash value Aug 28, 2020

dgryski added a commit to dgryski/semgrep-go that referenced this issue Aug 28, 2020

add a check for bad funcs passed to hmac.New()

9ac05b6

golang/go#41089

katiehockman self-assigned this Sep 22, 2020

gopherbot closed this as completed in 2a206c7 Oct 19, 2020

anthraxx mentioned this issue Mar 1, 2021

dep: bump keyring to v1.1.6 to fix Go 1.16 hmac API usage issues arafato/alicloud-vault#6

Merged

ericlagergren mentioned this issue Mar 26, 2021

crypto/hmac: panics #45252

Closed

wy65701436 mentioned this issue May 14, 2021

[WIP] upgrade golang to 1.16 goharbor/harbor#14872

Closed

reasonerjt mentioned this issue Jun 23, 2021

Harbor Version Upgrade from 2 to 2.2 Causing Key Issue goharbor/harbor#14932

Closed

wy65701436 mentioned this issue Jun 29, 2021

roll back go for notary binary goharbor/harbor#15223

Merged

wy65701436 mentioned this issue Jun 29, 2021

[cherry pick]roll back go for notary binary goharbor/harbor#15224

Merged

wy65701436 mentioned this issue Jun 29, 2021

[cherry pick]roll back go for notary binary goharbor/harbor#15225

Merged

reasonerjt mentioned this issue Jun 29, 2021

Error to decode a key that was persisted after using go1.15 to compile Notary notaryproject/notary#1602

Closed

golang locked and limited conversation to collaborators Oct 19, 2021

gopherbot added the FrozenDueToAge label Oct 19, 2021

rsc unassigned katiehockman Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto/hmac: detect reuse of hash.Hash value #41089

crypto/hmac: detect reuse of hash.Hash value #41089

dvsekhvalnov commented Aug 28, 2020 •

edited

FiloSottile commented Aug 28, 2020

gopherbot commented Aug 28, 2020

FiloSottile commented Aug 28, 2020 •

edited

FiloSottile commented Aug 28, 2020 •

edited

dvsekhvalnov commented Aug 28, 2020

katiehockman commented Sep 22, 2020

gopherbot commented Oct 13, 2020

magical commented Oct 13, 2020

katiehockman commented Oct 15, 2020

crypto/hmac: detect reuse of hash.Hash value #41089

crypto/hmac: detect reuse of hash.Hash value #41089

Comments

dvsekhvalnov commented Aug 28, 2020 • edited

FiloSottile commented Aug 28, 2020

gopherbot commented Aug 28, 2020

FiloSottile commented Aug 28, 2020 • edited

FiloSottile commented Aug 28, 2020 • edited

dvsekhvalnov commented Aug 28, 2020

katiehockman commented Sep 22, 2020

gopherbot commented Oct 13, 2020

magical commented Oct 13, 2020

katiehockman commented Oct 15, 2020

dvsekhvalnov commented Aug 28, 2020 •

edited

FiloSottile commented Aug 28, 2020 •

edited

FiloSottile commented Aug 28, 2020 •

edited