Merged
Conversation
BINSTRING opcode follows 4-byte length of the data and the data itself. Upon seeing the BINSTRING len header we were preallocating destination buffer with len as optimization (see 14aaa14 "decoder: Preallocate .buf capacity when we know (approximate) size of output to it"). However this way it is easy for a malicious pickle to specify BINSTRING biglength (4GB) and no data, and then the goroutine that processes such input will allocate 4GB immediately, which in turn might cause out-of-memory DOS. The other places where we currently grow Decoder.buf all either have 1 or 2 byte length (thus limited to 64K), or the length of the data that was already read from input stream. The problem was found by rerunning fuzz tests: "T0000" fatal error: runtime: out of memory runtime stack: runtime.throw(0x514fff, 0x16) /tmp/go-fuzz-build515164548/goroot/src/runtime/panic.go:608 +0x72 runtime.sysMap(0xc004000000, 0x34000000, 0x5f83d8) /tmp/go-fuzz-build515164548/goroot/src/runtime/mem_linux.go:156 +0xc7 runtime.(*mheap).sysAlloc(0x5dfd40, 0x34000000, 0x43c0e3, 0x7ffc94fe56b8) /tmp/go-fuzz-build515164548/goroot/src/runtime/malloc.go:619 +0x1c7 runtime.(*mheap).grow(0x5dfd40, 0x18182, 0x0) /tmp/go-fuzz-build515164548/goroot/src/runtime/mheap.go:920 +0x42 runtime.(*mheap).allocSpanLocked(0x5dfd40, 0x18182, 0x5f83e8, 0x203000) /tmp/go-fuzz-build515164548/goroot/src/runtime/mheap.go:848 +0x337 runtime.(*mheap).alloc_m(0x5dfd40, 0x18182, 0x7ffc94fe0101, 0x40a87f) /tmp/go-fuzz-build515164548/goroot/src/runtime/mheap.go:692 +0x119 runtime.(*mheap).alloc.func1() /tmp/go-fuzz-build515164548/goroot/src/runtime/mheap.go:759 +0x4c runtime.(*mheap).alloc(0x5dfd40, 0x18182, 0x7ffc94010101, 0x4135f5) /tmp/go-fuzz-build515164548/goroot/src/runtime/mheap.go:758 +0x8a runtime.largeAlloc(0x30303030, 0x440101, 0xc00005e240) /tmp/go-fuzz-build515164548/goroot/src/runtime/malloc.go:1019 +0x97 runtime.mallocgc.func1() /tmp/go-fuzz-build515164548/goroot/src/runtime/malloc.go:914 +0x46 runtime.systemstack(0x44eea9) /tmp/go-fuzz-build515164548/goroot/src/runtime/asm_amd64.s:351 +0x66 runtime.mstart() /tmp/go-fuzz-build515164548/goroot/src/runtime/proc.go:1229 goroutine 1 [running]: runtime.systemstack_switch() /tmp/go-fuzz-build515164548/goroot/src/runtime/asm_amd64.s:311 fp=0xc000034310 sp=0xc000034308 pc=0x44efa0 runtime.mallocgc(0x30303030, 0x4edc00, 0x1, 0xc0000343e8) /tmp/go-fuzz-build515164548/goroot/src/runtime/malloc.go:913 +0x896 fp=0xc0000343b0 sp=0xc000034310 pc=0x40ae26 runtime.makeslice(0x4edc00, 0x30303030, 0x30303030, 0xc000018108, 0x4, 0x4) /tmp/go-fuzz-build515164548/goroot/src/runtime/slice.go:70 +0x77 fp=0xc0000343e0 sp=0xc0000343b0 pc=0x43b1d7 bytes.makeSlice(0x30303030, 0x0, 0x0, 0x0) /tmp/go-fuzz-build515164548/goroot/src/bytes/buffer.go:231 +0x9d fp=0xc000034420 sp=0xc0000343e0 pc=0x4b204d bytes.(*Buffer).grow(0xc000084030, 0x30303030, 0x0) /tmp/go-fuzz-build515164548/goroot/src/bytes/buffer.go:144 +0x2e4 fp=0xc000034470 sp=0xc000034420 pc=0x4b1604 bytes.(*Buffer).Grow(0xc000084030, 0x30303030) /tmp/go-fuzz-build515164548/goroot/src/bytes/buffer.go:163 +0x86 fp=0xc000034498 sp=0xc000034470 pc=0x4b18b6 github.com/kisielk/og-rek.(*Decoder).loadBinString(0xc000084000, 0x203054, 0x0) /tmp/go-fuzz-build515164548/gopath/src/github.com/kisielk/og-rek/ogorek.go:646 +0x143 fp=0xc000034520 sp=0xc000034498 pc=0x4d5103 github.com/kisielk/og-rek.(*Decoder).Decode(0xc000084000, 0xc000080000, 0xc000084000, 0xf7eb9fa, 0x586dc) /tmp/go-fuzz-build515164548/gopath/src/github.com/kisielk/og-rek/ogorek.go:227 +0xf94 fp=0xc0000346d8 sp=0xc000034520 pc=0x4d09b4 github.com/kisielk/og-rek.Fuzz(0x7fbccd400000, 0x5, 0x200000, 0xc000034748) /tmp/go-fuzz-build515164548/gopath/src/github.com/kisielk/og-rek/fuzz.go:12 +0xb0 fp=0xc000034710 sp=0xc0000346d8 pc=0x4cf640 go-fuzz-dep.Main(0x519cf0) /tmp/go-fuzz-build515164548/goroot/src/go-fuzz-dep/main.go:49 +0xad fp=0xc000034780 sp=0xc000034710 pc=0x4642fd main.main() /tmp/go-fuzz-build515164548/gopath/src/github.com/kisielk/og-rek/go.fuzz.main/main.go:10 +0x2d fp=0xc000034798 sp=0xc000034780 pc=0x4db1ad runtime.main() /tmp/go-fuzz-build515164548/goroot/src/runtime/proc.go:201 +0x207 fp=0xc0000347e0 sp=0xc000034798 pc=0x428ec7 runtime.goexit() /tmp/go-fuzz-build515164548/goroot/src/runtime/asm_amd64.s:1333 +0x1 fp=0xc0000347e8 sp=0xc0000347e0 pc=0x450f01 exit status 2
Add more files go-fuzz put to corpus while discovering the crash fixed in previous commit.
We can enhance our fuzz-testing coverage by hooking Encoder also into the loop: if input data is suceessfully decoded, we have an object that can be passed back to Encoder to generate a pickle. We can't tell that that pickle must be the same as original input data, since pickle machine allows multiple representations of the same data. However we can assert that when that pickle is decoded back it should be the same as encoded object. This catches several problems: - marker is currently returned as pickle result (see next patch). - text-based STRING and UNICODE are not properly decoded (no fix yet). - self-referencing data structures kill Encoder (no fix yet).
A pickle is considered as invalid if it tries to return MARK as the
result by both Python2 and Python3, e.g.:
In [2]: pickle.loads(b"(.")
---------------------------------------------------------------------------
UnpicklingError Traceback (most recent call last)
<ipython-input-2-0c142c82b126> in <module>()
----> 1 pickle.loads(b"(.")
UnpicklingError: unexpected MARK found
However until now, despite mark is unexported ogórek type, we were
allowing for it to be returned just ok.
The problem was caught by decode/encode roundtrip fuzz tests, e.g.
"(Q."
panic: protocol 1: decode·encode != identity:
have: ogórek.Ref{Pid:map[interface {}]interface {}{}}
want: ogórek.Ref{Pid:ogórek.mark{}}
goroutine 1 [running]:
github.com/kisielk/og-rek.Fuzz(0x7fbe6c15f000, 0x3, 0x200000, 0x3)
/tmp/go-fuzz-build697921479/gopath/src/github.com/kisielk/og-rek/fuzz.go:87 +0x604
go-fuzz-dep.Main(0x524d78)
/tmp/go-fuzz-build697921479/goroot/src/go-fuzz-dep/main.go:49 +0xad
main.main()
/tmp/go-fuzz-build697921479/gopath/src/github.com/kisielk/og-rek/go.fuzz.main/main.go:10 +0x2d
exit status 2
New test vectors caught by decode·encode idempotency fuzzing.
This was referenced Sep 20, 2018
Collaborator
Author
|
Thanks for merging. |
Owner
|
Great stuff as always |
Collaborator
Author
|
Thanks for kind words. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Not much for today: