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.

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

tar: asm: store padding in chunks to avoid memory exhaustion #42

Merged
merged 3 commits into from Nov 7, 2017

Conversation

Projects
None yet
2 participants
@cyphar
Contributor

cyphar commented Nov 7, 2017

Previously, we would read the entire padding in a given archive into
memory in order to store it in the packer. This would cause memory
exhaustion if a malicious archive was crafted with very large amounts of
padding. Since a given SegmentType is reconstructed losslessly, we can
simply chunk up any padding into large segments to avoid this problem.
Use a reasonable default of 1MiB to avoid changing the tar-split.json of
existing archives that are not malformed.

Fixes: CVE-2017-14992
Signed-off-by: Aleksa Sarai asarai@suse.de

Fixes #41

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

/me will also add a test for this.
/cc @vbatts

Contributor

cyphar commented Nov 7, 2017

/me will also add a test for this.
/cc @vbatts

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

Fixed and added a test. I've verified that the test crashes without the fix, so it looks like this is the right place to fix it.

Contributor

cyphar commented Nov 7, 2017

Fixed and added a test. I've verified that the test crashes without the fix, so it looks like this is the right place to fix it.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

hrm ...

Owner

vbatts commented Nov 7, 2017

hrm ...

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

Test failure looks like go vet doesn't like the top-level tar_benchmark_test.go (which seems like a bad decision IMO). Do you want me to move it to archive/tar/?

Contributor

cyphar commented Nov 7, 2017

Test failure looks like go vet doesn't like the top-level tar_benchmark_test.go (which seems like a bad decision IMO). Do you want me to move it to archive/tar/?

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

master does not fail with this though

[vbatts@getdown] {master} ~/src/github.com/vbatts/tar-split/tar$ gt ./...
=== RUN   TestTarStreamMangledGetterPutter
--- PASS: TestTarStreamMangledGetterPutter (0.00s)
=== RUN   TestTarStream
--- PASS: TestTarStream (0.18s)
PASS
ok      github.com/vbatts/tar-split/tar/asm     0.180s
=== RUN   TestEntries
--- PASS: TestEntries (0.00s)
=== RUN   TestFile
--- PASS: TestFile (0.00s)
=== RUN   TestFileRaw
--- PASS: TestFileRaw (0.00s)
=== RUN   TestGetter
--- PASS: TestGetter (0.00s)
=== RUN   TestPutter
--- PASS: TestPutter (0.00s)
=== RUN   TestDuplicateFail
--- PASS: TestDuplicateFail (0.00s)
=== RUN   TestJSONPackerUnpacker
--- PASS: TestJSONPackerUnpacker (0.00s)
        packer_test.go:94: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x68, 0x6f, 0x77}, Position:0}
        packer_test.go:94: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x79, 0x27, 0x61, 0x6c, 0x6c}, Position:1}
        packer_test.go:94: got &storage.Entry{Type:1, Name:"./hurr.txt", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x64, 0x65, 0x61, 0x64, 0x62, 0x65, 0x65, 0x66}, Position:2}
        packer_test.go:94: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x64, 0x6f, 0x69, 0x6e}, Position:3}
=== RUN   TestGzip
--- PASS: TestGzip (0.00s)
        packer_test.go:158: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x68, 0x6f, 0x77}, Position:0}
        packer_test.go:158: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x79, 0x27, 0x61, 0x6c, 0x6c}, Position:1}
        packer_test.go:158: got &storage.Entry{Type:1, Name:"./hurr.txt", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x64, 0x65, 0x61, 0x64, 0x62, 0x65, 0x65, 0x66}, Position:2}
        packer_test.go:158: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x64, 0x6f, 0x69, 0x6e}, Position:3}
PASS
ok      github.com/vbatts/tar-split/tar/storage 0.002s
Owner

vbatts commented Nov 7, 2017

master does not fail with this though

[vbatts@getdown] {master} ~/src/github.com/vbatts/tar-split/tar$ gt ./...
=== RUN   TestTarStreamMangledGetterPutter
--- PASS: TestTarStreamMangledGetterPutter (0.00s)
=== RUN   TestTarStream
--- PASS: TestTarStream (0.18s)
PASS
ok      github.com/vbatts/tar-split/tar/asm     0.180s
=== RUN   TestEntries
--- PASS: TestEntries (0.00s)
=== RUN   TestFile
--- PASS: TestFile (0.00s)
=== RUN   TestFileRaw
--- PASS: TestFileRaw (0.00s)
=== RUN   TestGetter
--- PASS: TestGetter (0.00s)
=== RUN   TestPutter
--- PASS: TestPutter (0.00s)
=== RUN   TestDuplicateFail
--- PASS: TestDuplicateFail (0.00s)
=== RUN   TestJSONPackerUnpacker
--- PASS: TestJSONPackerUnpacker (0.00s)
        packer_test.go:94: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x68, 0x6f, 0x77}, Position:0}
        packer_test.go:94: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x79, 0x27, 0x61, 0x6c, 0x6c}, Position:1}
        packer_test.go:94: got &storage.Entry{Type:1, Name:"./hurr.txt", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x64, 0x65, 0x61, 0x64, 0x62, 0x65, 0x65, 0x66}, Position:2}
        packer_test.go:94: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x64, 0x6f, 0x69, 0x6e}, Position:3}
=== RUN   TestGzip
--- PASS: TestGzip (0.00s)
        packer_test.go:158: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x68, 0x6f, 0x77}, Position:0}
        packer_test.go:158: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x79, 0x27, 0x61, 0x6c, 0x6c}, Position:1}
        packer_test.go:158: got &storage.Entry{Type:1, Name:"./hurr.txt", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x64, 0x65, 0x61, 0x64, 0x62, 0x65, 0x65, 0x66}, Position:2}
        packer_test.go:158: got &storage.Entry{Type:2, Name:"", NameRaw:[]uint8(nil), Size:0, Payload:[]uint8{0x64, 0x6f, 0x69, 0x6e}, Position:3}
PASS
ok      github.com/vbatts/tar-split/tar/storage 0.002s
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

@vbatts Sorry, I deleted my old comment with the test failures. go test -v ./... passes for all Go versions now. The current failure is this:

$ go vet ./...
go build github.com/vbatts/tar-split: no non-test Go files in /home/travis/gopath/src/github.com/vbatts/tar-split
The command "go vet ./..." exited with 1.

And it only happens on tip, not for any released Go version.

Contributor

cyphar commented Nov 7, 2017

@vbatts Sorry, I deleted my old comment with the test failures. go test -v ./... passes for all Go versions now. The current failure is this:

$ go vet ./...
go build github.com/vbatts/tar-split: no non-test Go files in /home/travis/gopath/src/github.com/vbatts/tar-split
The command "go vet ./..." exited with 1.

And it only happens on tip, not for any released Go version.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

oh? you say it's for file placement?

Owner

vbatts commented Nov 7, 2017

oh? you say it's for file placement?

*: move tar_benchmark to cmd/tar-split/
This fixes a new go-vet(1) error which has surfaced in Go HEAD.

  $ go vet ./...
  go build github.com/vbatts/tar-split: no non-test Go files in
  /home/travis/gopath/src/github.com/vbatts/tar-split

Signed-off-by: Aleksa Sarai <asarai@suse.de>
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

Yup. I've moved the file in cyphar/tar-split@b9775006bf71ba57bd488d7faf097d18d9979b7b and the issue is no longer broken.

Contributor

cyphar commented Nov 7, 2017

Yup. I've moved the file in cyphar/tar-split@b9775006bf71ba57bd488d7faf097d18d9979b7b and the issue is no longer broken.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

also, you're using a 1Mb chunk, not 1K

Owner

vbatts commented Nov 7, 2017

also, you're using a 1Mb chunk, not 1K

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

Typo fixed.

Contributor

cyphar commented Nov 7, 2017

Typo fixed.

cyphar added some commits Nov 7, 2017

tar: asm: store padding in chunks to avoid memory exhaustion
Previously, we would read the entire padding in a given archive into
memory in order to store it in the packer. This would cause memory
exhaustion if a malicious archive was crafted with very large amounts of
padding. Since a given SegmentType is reconstructed losslessly, we can
simply chunk up any padding into large segments to avoid this problem.
Use a reasonable default of 1MiB to avoid changing the tar-split.json of
existing archives that are not malformed.

Fixes: CVE-2017-14992
Signed-off-by: Aleksa Sarai <asarai@suse.de>
tar: asm: add an excess padding test case
To ensure we don't have regressions in our padding fix, add a test case
that attempts to crash the test by creating 20GB of random junk padding.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

also also @cyphar , do you want to open the PR to moby once this is fixed?

Owner

vbatts commented Nov 7, 2017

also also @cyphar , do you want to open the PR to moby once this is fixed?

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

LGTM

Owner

vbatts commented Nov 7, 2017

LGTM

@vbatts vbatts merged commit 38ec4dd into vbatts:master Nov 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

Sure, I'll open the PR. As soon as you do a release. 😉

Contributor

cyphar commented Nov 7, 2017

Sure, I'll open the PR. As soon as you do a release. 😉

@cyphar cyphar deleted the cyphar:chunked-padding branch Nov 7, 2017

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

v0.10.2 is pushed now

Owner

vbatts commented Nov 7, 2017

v0.10.2 is pushed now

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 7, 2017

Contributor

Whoa that was fast. 😸 Working up the PR now.

Contributor

cyphar commented Nov 7, 2017

Whoa that was fast. 😸 Working up the PR now.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

woah
wrong tag!

Owner

vbatts commented Nov 7, 2017

woah
wrong tag!

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 7, 2017

Owner

oh weird. there were unpushed tags that got me concerned of an out of sequence tag. sorry.
v0.10.2 is pushed https://github.com/vbatts/tar-split/releases/tag/v0.10.2

Owner

vbatts commented Nov 7, 2017

oh weird. there were unpushed tags that got me concerned of an out of sequence tag. sorry.
v0.10.2 is pushed https://github.com/vbatts/tar-split/releases/tag/v0.10.2

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Nov 8, 2017

Contributor

Docker PR was merged: moby/moby#35424

Contributor

cyphar commented Nov 8, 2017

Docker PR was merged: moby/moby#35424

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 8, 2017

Owner
Owner

vbatts commented Nov 8, 2017

nalind added a commit to nalind/buildah-1 that referenced this pull request Nov 8, 2017

Bump github.com/vbatts/tar-split
Update github.com/vbatts/tar-split to v0.10.2 and pin that version
instead of master, to pick up vbatts/tar-split#42

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

rh-atomic-bot added a commit to containers/buildah that referenced this pull request Nov 8, 2017

Bump github.com/vbatts/tar-split
Update github.com/vbatts/tar-split to v0.10.2 and pin that version
instead of master, to pick up vbatts/tar-split#42

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Closes: #318
Approved by: rhatdan

nalind added a commit to nalind/cri-o that referenced this pull request Nov 8, 2017

Bump github.com/vbatts/tar-split
Update vendor/github.com/vbatts/tar-split to v0.10.2, to fix
CVE-2017-14992, per vbatts/tar-split#42.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind added a commit to nalind/cri-o that referenced this pull request Nov 9, 2017

Bump github.com/vbatts/tar-split
Update vendor/github.com/vbatts/tar-split to v0.10.2, to fix
CVE-2017-14992, per vbatts/tar-split#42.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment