internal/zstd: Bugs and crashes #63824

klauspost · 2023-10-30T16:17:21Z

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

Go tip, self compiled.

Does this issue reproduce with the latest release?

Yes.

What did you do?

Run some fuzz tests against the internal zstd package.

What did you expect to see?

No output.

What did you see instead?

A) Crash

With the following input, the internal/zstd decompressor can fail. To reproduce:

Add src/internal/zstd/testdata/fuzz/FuzzReader/e4f4dab13d374c2d with the following content:

go test fuzz v1
[]byte("(\xb5/\xfd00\xec\x00\x00&@\x05\x05A7002\x02\x00\x02\x00\x02\x0000000000000000")

This produces the following crash:

--- FAIL: FuzzReader (10.70s)
    --- FAIL: FuzzReader (0.00s)
        testing.go:1504: panic: runtime error: index out of range [2] with length 2
            goroutine 145795 [running]:
            runtime/debug.Stack()
                C:/go/src/runtime/debug/stack.go:24 +0x9b
            testing.tRunner.func1()
                C:/go/src/testing/testing.go:1504 +0x1ee
            panic({0x6952c0?, 0xc000130618?})
                C:/go/src/runtime/panic.go:914 +0x21f
            [github.com/klauspost/compress/z2.(*Reader).readLiteralsFourStreams(0xc001cb1e00](http://github.com/klauspost/compress/z2.(*Reader).readLiteralsFourStreams(0xc001cb1e00), {0xc000502d20, 0x1d, 0x20}, 0x9, 0xf, 0x2, {0x0, 0x0, 0x0})
                e:/gopath/src/[github.com/klauspost/compress/z2/literals.go:324](http://github.com/klauspost/compress/z2/literals.go:324) +0x1079
            [github.com/klauspost/compress/z2.(*Reader).readHuffLiterals(0xc001cb1e00](http://github.com/klauspost/compress/z2.(*Reader).readHuffLiterals(0xc001cb1e00), {0xc000502d20, 0x1d, 0x20}, 0x1, 0x26, {0x0, 0x0, 0x0})
                e:/gopath/src/[github.com/klauspost/compress/z2/literals.go:168](http://github.com/klauspost/compress/z2/literals.go:168) +0x11e7
            [github.com/klauspost/compress/z2.(*Reader).readLiterals(0xc001cb1e00](http://github.com/klauspost/compress/z2.(*Reader).readLiterals(0xc001cb1e00), {0xc000502d20, 0x1d, 0x20}, 0x0, {0x0, 0x0, 0x0})
                e:/gopath/src/[github.com/klauspost/compress/z2/literals.go:26](http://github.com/klauspost/compress/z2/literals.go:26) +0x30d
            [github.com/klauspost/compress/z2.(*Reader).compressedBlock(0xc001cb1e00](http://github.com/klauspost/compress/z2.(*Reader).compressedBlock(0xc001cb1e00), 0x1d)
                e:/gopath/src/[github.com/klauspost/compress/z2/block.go:35](http://github.com/klauspost/compress/z2/block.go:35) +0x44d
            [github.com/klauspost/compress/z2.(*Reader).readBlock(0xc001cb1e00)](http://github.com/klauspost/compress/z2.(*Reader).readBlock(0xc001cb1e00))
                e:/gopath/src/[github.com/klauspost/compress/z2/zstd.go:397](http://github.com/klauspost/compress/z2/zstd.go:397) +0xd10
            [github.com/klauspost/compress/z2.(*Reader).refill(0xc001cb1e00](http://github.com/klauspost/compress/z2.(*Reader).refill(0xc001cb1e00)?)
                e:/gopath/src/[github.com/klauspost/compress/z2/zstd.go:164](http://github.com/klauspost/compress/z2/zstd.go:164) +0xd4
            [github.com/klauspost/compress/z2.(*Reader).refillIfNeeded(0xc001cb1e00)](http://github.com/klauspost/compress/z2.(*Reader).refillIfNeeded(0xc001cb1e00))
                e:/gopath/src/[github.com/klauspost/compress/z2/zstd.go:149](http://github.com/klauspost/compress/z2/zstd.go:149) +0xa5
            [github.com/klauspost/compress/z2.(*Reader).Read(0xc001cb1e00](http://github.com/klauspost/compress/z2.(*Reader).Read(0xc001cb1e00), {0xc001d2a000, 0x2000, 0x0?})
                e:/gopath/src/[github.com/klauspost/compress/z2/zstd.go:128](http://github.com/klauspost/compress/z2/zstd.go:128) +0x45
            io.discard.ReadFrom({}, {0x6f0200, 0xc001cb1e00})
                C:/go/src/io/io.go:658 +0xad
            io.copyBuffer({0x6f02a0, 0x876a20}, {0x6f0200, 0xc001cb1e00}, {0x0, 0x0, 0x0})
                C:/go/src/io/io.go:416 +0x14f
            io.Copy(...)
                C:/go/src/io/io.go:389
            [github.com/klauspost/compress/z2.FuzzReader.func1(0x0](http://github.com/klauspost/compress/z2.FuzzReader.func1(0x0)?, {0xc00013a150, 0x26, 0x30})
                e:/gopath/src/[github.com/klauspost/compress/z2/fuzz_test.go:42](http://github.com/klauspost/compress/z2/fuzz_test.go:42) +0x10e
            reflect.Value.call({0x66dca0?, 0x6b9990?, 0x6a68e0?}, {0x6a7418, 0x4}, {0xc001c7b1a0, 0x2, 0x18?})
                C:/go/src/reflect/value.go:596 +0xce7
            reflect.Value.Call({0x66dca0?, 0x6b9990?, 0x8061a0?}, {0xc001c7b1a0?, 0x6a68e0?, 0x57c58d?})
                C:/go/src/reflect/value.go:380 +0xb9
            testing.(*F).Fuzz.func1.1(0x436800?)
                C:/go/src/testing/fuzz.go:335 +0x347
            testing.tRunner(0xc001ccb040, 0xc001c97170)
                C:/go/src/testing/testing.go:1595 +0xff
            created by testing.(*F).Fuzz.func1 in goroutine 19
                C:/go/src/testing/fuzz.go:322 +0x597

Checking that out1+out2+out3 are in bounds fixes the issue, but it seems a bit expensive.

B) Missing error reporting

The following inputs does not report expected problems:

zstd-internal-divergences.zip

Input and various outputs are reported as:

go: <nil>, klaus: output bigger than max block size (1024), zstd.exe: exit status 1: /*stdin*\ : Decoding error (36) : Data corruption detected

go: Error reported by internal/zstd.
klaus: Error reported by https://github.com/klauspost/compress/zstd
zstd.exe: Error reported from Zstandard CLI (64-bit) v1.5.4

Sorry for not having the time to dig into these right now, but I thought it would be the best to get them reported.

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

bcmills · 2023-10-30T17:31:45Z

Labeling as compiler/runtime since the only current importer of internal/zstd appears to be debug/elf.

Fixes: golang#63824

aimuz · 2023-11-06T08:22:15Z

I attempted to make a fix for this issue that: #63959

gopherbot · 2023-11-06T08:30:35Z

Change https://go.dev/cl/540055 mentions this issue: internal/zstd: fix regeneratedSize too small to cause panic

regeneratedSize that is too small will result in index out-of-bounds behaviour for expanded output slices. regeneratedSize should be greater than or equal to 6 Fixes: golang#63824

This commit adds an additional safety check to ensure that the `regeneratedSize` is not less than 6. If it's smaller, this would lead to an out-of-bounds error when trying to access the third or fourth stream within our slice during decoding." Fixes: golang#63824

This commit adds an additional safety check to ensure that the `regeneratedSize` is not less than 6. If it's smaller, this would lead to an out-of-bounds error when trying to access the third or fourth stream within our slice during decoding. Fixes: golang#63824

refactor literal decoding into decodeSymbol function. Added output buffer checking to avoid panic caused by too small output buffer. Benchmarking hasn't changed much goos: darwin goarch: amd64 pkg: internal/zstd cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ Large-12 7.823m ± 2% 7.854m ± 6% ~ (p=0.247 n=10) │ old.txt │ new.txt │ │ B/s │ B/s vs base │ Large-12 33.17Mi ± 2% 33.04Mi ± 6% ~ (p=0.217 n=10) │ old.txt │ new.txt │ │ B/op │ B/op vs base │ Large-12 73.62Ki ± 4% 74.32Ki ± 3% ~ (p=0.424 n=10) │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ Large-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹ ¹ all samples are equal Fixes: golang#63824

The decompressed size of each stream is equal to (Regenerated_Size+3)/4, except for the last stream, which may be up to 3 bytes smaller, to reach a total decompressed size as specified in Regenerated_Size. Fixes golang#63824

klauspost · 2023-11-09T15:00:33Z

@aimuz Will you be picking up the un-reported corruption as well?

aimuz · 2023-11-09T15:24:56Z

@aimuz Will you be picking up the un-reported corruption as well?

@klauspost If you suspect there's an underlying problem that hasn't been documented yet, please let me know, and I'll investigate it.

klauspost · 2023-11-09T15:32:14Z

@aimuz I was thinking of the divergence in output in the "B) Missing error reporting" section.

aimuz · 2023-11-09T15:41:08Z

@klauspost Can you give me more details?

klauspost · 2023-11-09T17:08:58Z

@aimuz See section B) of the issue above. There is a zip file with reproducers.

aimuz · 2023-11-10T01:47:33Z

I'm sorry I left the above information out, thank you, I'll look into it 😀

gopherbot · 2023-11-10T07:36:48Z

Change https://go.dev/cl/541220 mentions this issue: internal/zstd: fix seek offset bounds check in skipFrame

gopherbot · 2023-11-15T07:30:21Z

Change https://go.dev/cl/542417 mentions this issue: internal/zstd: enforce block size limit according to RFC 8878

aimuz · 2023-11-15T07:30:59Z

@klauspost All issues have been fixed

klauspost · 2023-11-15T09:44:56Z

@aimuz If you are up for it, here is another batch: batch2.zip

Probably only as few root issues. Seeing unreported: output bigger than max block size, unexpected EOF errors.

Seeing zstd decompression error at XX: invalid symbol compression mode on seemingly valid content.

aimuz · 2023-11-15T11:42:45Z

Seeing zstd decompression error at XX: invalid symbol compression mode on seemingly valid content.

It should be fine as described in the documentation here
https://datatracker.ietf.org/doc/html/rfc8878#name-symbol_compression_modes

I created an issue in the zstd repository: facebook/zstd#3821

This change enhances the zstd Reader's skipFrame function to validate the new offset when skipping frames in a seekable stream, preventing invalid offsets that could occur previously. A set of "bad" test strings has been added to fuzz_test.go to extend the robustness checks against potential decompression panics. Additionally, a new test named TestReaderBad is introduced in zstd_test.go to verify proper error handling with corrupted input strings. The BenchmarkLarge function has also been refactored for clarity, removing unnecessary timer stops and resets. Updates #63824 Change-Id: Iccd248756ad6348afa1395c7799350d07402868a GitHub-Last-Rev: 63055b9 GitHub-Pull-Request: #64056 Reviewed-on: https://go-review.googlesource.com/c/go/+/541220 Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Klaus Post <klauspost@gmail.com> Auto-Submit: Bryan Mills <bcmills@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

mauri870 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 30, 2023

bcmills added help wanted compiler/runtime Issues related to the Go compiler and/or runtime. labels Oct 30, 2023

cherrymui added this to the Backlog milestone Oct 30, 2023

aimuz added a commit to aimuz/go that referenced this issue Nov 6, 2023

internal/zstd: fix regeneratedSize too small to cause panic

c76e909

Fixes: golang#63824

aimuz mentioned this issue Nov 6, 2023

internal/zstd: avoid panic when the regenerated size is too small #63959

Closed

mknyszek assigned mdempsky and aimuz Nov 8, 2023

gopherbot closed this as completed in 291ffcb Nov 9, 2023

aimuz mentioned this issue Nov 10, 2023

internal/zstd: fix seek offset bounds check in skipFrame #64056

Closed

aimuz mentioned this issue Nov 15, 2023

internal/zstd: enforce block size limit according to RFC 8878 #64155

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/zstd: Bugs and crashes #63824

internal/zstd: Bugs and crashes #63824

klauspost commented Oct 30, 2023

bcmills commented Oct 30, 2023

aimuz commented Nov 6, 2023

gopherbot commented Nov 6, 2023

klauspost commented Nov 9, 2023

aimuz commented Nov 9, 2023

klauspost commented Nov 9, 2023

aimuz commented Nov 9, 2023

klauspost commented Nov 9, 2023

aimuz commented Nov 10, 2023

gopherbot commented Nov 10, 2023

gopherbot commented Nov 15, 2023

aimuz commented Nov 15, 2023

klauspost commented Nov 15, 2023

aimuz commented Nov 15, 2023 •

edited

internal/zstd: Bugs and crashes #63824

internal/zstd: Bugs and crashes #63824

Comments

klauspost commented Oct 30, 2023

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

Does this issue reproduce with the latest release?

What did you do?

What did you expect to see?

What did you see instead?

A) Crash

B) Missing error reporting

bcmills commented Oct 30, 2023

aimuz commented Nov 6, 2023

gopherbot commented Nov 6, 2023

klauspost commented Nov 9, 2023

aimuz commented Nov 9, 2023

klauspost commented Nov 9, 2023

aimuz commented Nov 9, 2023

klauspost commented Nov 9, 2023

aimuz commented Nov 10, 2023

gopherbot commented Nov 10, 2023

gopherbot commented Nov 15, 2023

aimuz commented Nov 15, 2023

klauspost commented Nov 15, 2023

aimuz commented Nov 15, 2023 • edited

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

aimuz commented Nov 15, 2023 •

edited