Skip to content

x/crypto/chacha20: panic with valid counter value #37157

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
Closed
rod-hynes opened this issue Feb 10, 2020 · 4 comments
Closed

x/crypto/chacha20: panic with valid counter value #37157

rod-hynes opened this issue Feb 10, 2020 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@rod-hynes
Copy link

What version of Go are you using (go version)?

Go 1.13.7

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

darwin/amd64

What did you do?

Ran the following test, which reproduces the panic.

Note that this test uses the pending CL golang/crypto#108, which adds the SetCounter method to x/crypto/chacha20. While the panic on the last valid counter value, 2^32-1, is present in the current x/crypto/chacha20, it's unlikley to be hit in typical use as the counter value cannot be set and always starts at 0.

func TestIETFQUICHeaderProtection(t *testing.T) {
	var key [32]byte
	var nonce [12]byte
	var block [64]byte
	c, _ := NewUnauthenticatedCipher(key[:], nonce[:])
	c.SetCounter(0xffffffff)
	c.XORKeyStream(block[:], block[:])
}

Background:

IETF ChaCha20 allows 2^32 blocks: https://tools.ietf.org/html/rfc7539#section-2.3.2.

IETF QUIC ChaCha20-based header protection, https://tools.ietf.org/html/draft-ietf-quic-tls-24#section-5.4.4, initializes the ChaCha20 counter with "4 bytes of [...] sampled ciphertext", so it's possible to have any counter value in [0, 2^32-1] and attempt to encrypt one block.

We hit this panic in production while using https://github.com/lucas-clemente/quic-go, which uses a recent fork of x/crypto/internal/chacha20 that exposes a function to set the counter value.

Related issue: quic-go/quic-go#2326.

What did you expect to see?

Successful test run.

What did you see instead?

=== RUN   TestIETFQUICHeaderProtection
--- FAIL: TestIETFQUICHeaderProtection (0.00s)
panic: chacha20: counter overflow [recovered]
	panic: chacha20: counter overflow

goroutine 7 [running]:
testing.tRunner.func1(0xc000094100)
	/usr/local/go/src/testing/testing.go:874 +0x3a3
panic(0x1117dc0, 0x1187e60)
	/usr/local/go/src/runtime/panic.go:679 +0x1b2
golang.org/x/crypto/chacha20.(*Cipher).XORKeyStream(0xc000034eb0, 0xc000034e70, 0x40, 0x40, 0xc000034e70, 0x40, 0x40)
	/Users/Work/Code/gopath/src/github.com/lukechampine/crypto/chacha20/chacha_generic.go:206 +0x393
golang.org/x/crypto/chacha20.TestIETFQUICHeaderProtection(0xc000094100)
	/Users/Work/Code/gopath/src/github.com/lukechampine/crypto/chacha20/chacha_test.go:32 +0x161
testing.tRunner(0xc000094100, 0x1169418)
	/usr/local/go/src/testing/testing.go:909 +0xc9
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:960 +0x350
exit status 2
FAIL	golang.org/x/crypto/chacha20	0.322s

Here is a fix implemented by @lukechampine: lukechampine/crypto@d3e589b (briefly discussed here: golang/crypto#108 (comment)).
@gopherbot gopherbot added this to the Unreleased milestone Feb 10, 2020
@MaxSem
Copy link

MaxSem commented Feb 10, 2020

CC @FiloSottile

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 14, 2020
@rod-hynes rod-hynes mentioned this issue Feb 24, 2020
Closed
@lukechampine lukechampine mentioned this issue Feb 24, 2020
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/220591 mentions this issue: chacha20: Prevent panic when crypting final block

@FiloSottile FiloSottile changed the title x/crypto: Panic in chacha20 with valid counter value x/crypto/chacha20: panic with valid counter value Feb 24, 2020
@FiloSottile FiloSottile added 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 Mar 9, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/224279 mentions this issue: chacha20: don't panic encrypting the final blocks

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/229119 mentions this issue: chacha20: don't panic encrypting the final blocks

@FiloSottile FiloSottile reopened this Apr 20, 2020
@golang golang locked and limited conversation to collaborators Apr 23, 2021
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@FiloSottile
chacha20: don't panic encrypting the final blocks
e93bdac 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Change-Id: Iba52edb1ba36af391b8fe4ee615c5c41d7e64f48
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@FiloSottile
chacha20: don't panic encrypting the final blocks
4eceb83 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Relanding of CL 224279, which was broken on multi-block buffers.
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

Change-Id: Ia382c6f62ae49ffe257b67f7b794e8d7124d981e
(cherry picked from commit 1c2c788)
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/229119
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
chacha20: don't panic encrypting the final blocks
5a19b1e 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Change-Id: Iba52edb1ba36af391b8fe4ee615c5c41d7e64f48
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
chacha20: don't panic encrypting the final blocks
f258727 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Relanding of CL 224279, which was broken on multi-block buffers.
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

Change-Id: Ia382c6f62ae49ffe257b67f7b794e8d7124d981e
(cherry picked from commit 1c2c788)
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/229119
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
chacha20: don't panic encrypting the final blocks
3ecf467 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Change-Id: Iba52edb1ba36af391b8fe4ee615c5c41d7e64f48
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
chacha20: don't panic encrypting the final blocks
7f8abfb 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Relanding of CL 224279, which was broken on multi-block buffers.
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

Change-Id: Ia382c6f62ae49ffe257b67f7b794e8d7124d981e
(cherry picked from commit 1c2c788)
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/229119
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@FiloSottile
chacha20: don't panic encrypting the final blocks
0b689e6 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Change-Id: Iba52edb1ba36af391b8fe4ee615c5c41d7e64f48
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@FiloSottile
chacha20: don't panic encrypting the final blocks
cfed815 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Relanding of CL 224279, which was broken on multi-block buffers.
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

Change-Id: Ia382c6f62ae49ffe257b67f7b794e8d7124d981e
(cherry picked from commit 0b689e6)
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/229119
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@FiloSottile
chacha20: don't panic encrypting the final blocks
d043b6f 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Change-Id: Iba52edb1ba36af391b8fe4ee615c5c41d7e64f48
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@FiloSottile
chacha20: don't panic encrypting the final blocks
d4aae45 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Relanding of CL 224279, which was broken on multi-block buffers.
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

Change-Id: Ia382c6f62ae49ffe257b67f7b794e8d7124d981e
(cherry picked from commit 1c2c788)
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/229119
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
desdeel2d0m added a commit to desdeel2d0m/crypto that referenced this issue Jul 1, 2024
@desdeel2d0m @FiloSottile
chacha20: don't panic encrypting the final blocks
78fe84a 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Change-Id: Iba52edb1ba36af391b8fe4ee615c5c41d7e64f48
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
desdeel2d0m added a commit to desdeel2d0m/crypto that referenced this issue Jul 1, 2024
@desdeel2d0m @FiloSottile
chacha20: don't panic encrypting the final blocks
826c68c 
Certain operations with counter values close to overflowing were causing
an unnecessary panic, which was reachable due to the SetCounter API and
actually observed in QUIC.

Tests by lukechampine <luke.champine@gmail.com> from CL 220591.

Fixes golang/go#37157

Relanding of CL 224279, which was broken on multi-block buffers.
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/224279
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>

Change-Id: Ia382c6f62ae49ffe257b67f7b794e8d7124d981e
(cherry picked from commit 78fe84a)
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/229119
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

chacha20: Prevent panic when crypting final block lukechampine/crypto
5 participants
@rod-hynes @FiloSottile @MaxSem @dmitshur @gopherbot