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.

Sign up for GitHub

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

Jump to bottom

x/crypto/sha3: bytepad function is not safe #69169

Closed
paocalvi opened this issue Aug 30, 2024 · 5 comments
Closed

x/crypto/sha3: bytepad function is not safe #69169

paocalvi opened this issue Aug 30, 2024 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@paocalvi
Copy link

paocalvi commented Aug 30, 2024 •
edited by seankhliao
Loading

Go version

1.23 (all)

Output of go env in your module/workspace:

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\...\AppData\Local\go-build
set GOENV=C:\Users\...\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\calvip\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\...\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.22.3
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\.....\AppData\Local\Temp\go-build939558769=/tmp/go-build -gno-record-gcc-switches

What did you do?

Static analysis of the x/crypto/sha3/shake.go function

What did you see happen?

The code

func bytepad(input []byte, w int) []byte {
	// leftEncode always returns max 9 bytes
	buf := make([]byte, 0, 9+len(input)+w)
	buf = append(buf, leftEncode(uint64(w))...)
	buf = append(buf, input...)
	padlen := w - (len(buf) % w)
	return append(buf, make([]byte, padlen)...)
}

and in particular

padlen := w - (len(buf) % w)

is not working properly when len(buf) % w == 0, causing an undesired padding.

The function appear to work only if the length of the input + length of the letfencoding is not a multiple of w already.

What did you expect to see?

leftEncode([]uint8("12345678"),10) // leftencode should be 2 bytelong that summed with 8 is 10
shoud return a 10 bytes result, not a 20 bytes result.
@gopherbot gopherbot added this to the Unreleased milestone Aug 30, 2024
@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@paocalvi paocalvi changed the title x/crypto : bytepad functionnot safe x/crypto : bytepad function is not safe Aug 30, 2024
@ianlancetaylor
Copy link
Member

CC @golang/security

@seankhliao seankhliao changed the title x/crypto : bytepad function is not safe x/crypto/sha3 : bytepad function is not safe Sep 2, 2024
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 4, 2024
@dmitshur dmitshur changed the title x/crypto/sha3 : bytepad function is not safe x/crypto/sha3: bytepad function is not safe Sep 4, 2024
@magical
Copy link
Contributor

magical commented Sep 6, 2024

@paocalvi's analysis seems right to me.

Here's a reproducer for cSHAKE-128:

	s := []byte(strings.Repeat("x", 168-7))
	fmt.Printf("S=%s\n", s)
	cs := sha3.NewCShake128(nil, s)
	h := cs.Sum(nil)
	fmt.Printf("%x\n", h)
	// Output: 430d3ebae1528304465f3b6f2ed34a7b931af804afe97d0e2a2796abf5725281
	// Expected output: 2cf20c4b26c9ee7751eaa273368e616c868e7275178634e1ecdbac80d4cab5f4

https://go.dev/play/p/RQRAsLybuBg

The 161-byte customization string ensures that the length of the bytepadded initial block will have a length equal to cSHAKE-128's rate (168 bytes).

Expected output taken from this random js implementation. Someone should probably verify it against the reference implementation, though.

@FiloSottile
Copy link
Contributor

Very unfortunate that neither our tests nor Wycheproof caught this.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616576 mentions this issue: sha3: fix padding for long cSHAKE parameters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@magical @FiloSottile @dmitshur @ianlancetaylor @gopherbot @paocalvi @gabyhelp