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

vendor: update to github.com/vbatts/tar-split@v0.10.2 #35424

Merged
merged 3 commits into from
Nov 7, 2017
Merged

vendor: update to github.com/vbatts/tar-split@v0.10.2 #35424

merged 3 commits into from
Nov 7, 2017

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Nov 7, 2017

Update to the latest version of tar-split, which includes a change to
fix a memory exhaustion issue where a malformed image could cause the
Docker daemon to crash.

  • tar: asm: store padding in chunks to avoid memory exhaustion

axolotls2 by kori monster

axolotls2 by kori monster

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

@cyphar
vendor: update to github.com/vbatts/tar-split@v0.10.2
e0ff7cc 
Update to the latest version of tar-split, which includes a change to
fix a memory exhaustion issue where a malformed image could cause the
Docker daemon to crash.

  * tar: asm: store padding in chunks to avoid memory exhaustion

Fixes: CVE-2017-14992
Signed-off-by: Aleksa Sarai <asarai@suse.de>
@cyphar
Copy link
Contributor Author

cyphar commented Nov 7, 2017

/cc @n4ss

AkihiroSuda
AkihiroSuda approved these changes Nov 7, 2017
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, LGTM

@cyphar
Copy link
Contributor Author

cyphar commented Nov 7, 2017

CVE-2017-14992 was assigned for this bug.

vdemeester
vdemeester approved these changes Nov 7, 2017
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐮
/cc @thaJeztah @vieux

thaJeztah
thaJeztah approved these changes Nov 7, 2017
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

do we need a test in this repository as well, or is it sufficient that it's tested upstream?

@cyphar
Copy link
Contributor Author

cyphar commented Nov 7, 2017

@thaJeztah I can work up an integration test if you like.

@cyphar cyphar requested a review from dnephin as a code owner November 7, 2017 16:28
@cyphar
internal: testutil: add DevZero helper
2f8d3e1 
This helper acts like /dev/zero (outputs \x00 indefinitely) in an
OS-independent fashion. This ensures we don't need to special-case
around Windows in tests that want to open /dev/zero.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
@thaJeztah
Copy link
Member

Thanks for adding the test! I restarted PowerPC and Z CI (Jenkins seemed to have some issues)

@cyphar
Copy link
Contributor Author

cyphar commented Nov 7, 2017

Hmm, looks like 20GB is too large for this test. I'll try reducing it...

@thaJeztah
Copy link
Member

oh, can you squash that commit with the previous one?

@cyphar
Copy link
Contributor Author

cyphar commented Nov 7, 2017 •
edited
Loading

I'll squash as soon as I've got a working size. 😉

EDIT: Done, 8GB looks like it's big enough to test the issue but only takes ~2min to "upload".

@cyphar
image: add import test for CVE-2017-14992
0a13f82 
To ensure that we don't revert CVE-2017-14992, add a test that is quite
similar to that upstream tar-split test (create an empty archive with
lots of junk and make sure the daemon doesn't crash).

Signed-off-by: Aleksa Sarai <asarai@suse.de>
@cyphar
Copy link
Contributor Author

cyphar commented Nov 7, 2017 •
edited
Loading

All tests pass (ppc appears to be stalling in the last bit of clean-up but the actual tests have passed).

🥗

@andrewhsu
Copy link
Member

The ppc64le tests passed even though the status is not green:

20:33:27 OK: 1620 passed, 107 skipped
20:33:27 PASS

We've been having connectivity issues with the ppc64le slaves so I'd say don't wait on a perfectly green run from that arch.

@thaJeztah
Copy link
Member

SGTM, let's go ahead and merge

@thaJeztah thaJeztah merged commit bd8ed57 into moby:master Nov 7, 2017
@andrewhsu andrewhsu mentioned this pull request Nov 7, 2017
Merged
This was referenced Nov 7, 2017
Closed
Merged
@cyphar cyphar deleted the tar-split-cve_2017_14992 branch November 7, 2017 22:38
@cyphar cyphar mentioned this pull request Nov 8, 2017
Merged
@thaJeztah thaJeztah mentioned this pull request Nov 23, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@dnephin dnephin Awaiting requested review from dnephin

Assignees
No one assigned
Labels
impact/changelog status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tar-split does not read block by block nor validate tar headers
6 participants
@cyphar @thaJeztah @andrewhsu @vdemeester @AkihiroSuda @GordonTheTurtle