-
Notifications
You must be signed in to change notification settings - Fork 18.8k
Pass root to chroot to for chroot Tar/Untar (CVE-2018-15664) #39292
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,18 +27,34 @@ func NewArchiver(idMapping *idtools.IdentityMapping) *archive.Archiver { | |
// The archive may be compressed with one of the following algorithms: | ||
// identity (uncompressed), gzip, bzip2, xz. | ||
func Untar(tarArchive io.Reader, dest string, options *archive.TarOptions) error { | ||
return untarHandler(tarArchive, dest, options, true) | ||
return untarHandler(tarArchive, dest, options, true, dest) | ||
} | ||
|
||
// UntarWithRoot is the same as `Untar`, but allows you to pass in a root directory | ||
// The root directory is the directory that will be chrooted to. | ||
// `dest` must be a path within `root`, if it is not an error will be returned. | ||
// | ||
// `root` should set to a directory which is not controlled by any potentially | ||
// malicious process. | ||
// | ||
// This should be used to prevent a potential attacker from manipulating `dest` | ||
// such that it would provide access to files outside of `dest` through things | ||
// like symlinks. Normally `ResolveSymlinksInScope` would handle this, however | ||
// sanitizing symlinks in this manner is inherrently racey: | ||
// ref: CVE-2018-15664 | ||
func UntarWithRoot(tarArchive io.Reader, dest string, options *archive.TarOptions, root string) error { | ||
return untarHandler(tarArchive, dest, options, true, root) | ||
} | ||
|
||
// UntarUncompressed reads a stream of bytes from `archive`, parses it as a tar archive, | ||
// and unpacks it into the directory at `dest`. | ||
// The archive must be an uncompressed stream. | ||
func UntarUncompressed(tarArchive io.Reader, dest string, options *archive.TarOptions) error { | ||
return untarHandler(tarArchive, dest, options, false) | ||
return untarHandler(tarArchive, dest, options, false, dest) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the 5th parameter is the same with 2nd parameter , both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just didn't want to make a bunch of changes to code that isn't affected by this. |
||
} | ||
|
||
// Handler for teasing out the automatic decompression | ||
func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool) error { | ||
func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool, root string) error { | ||
if tarArchive == nil { | ||
return fmt.Errorf("Empty archive") | ||
} | ||
|
@@ -69,5 +85,13 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions | |
r = decompressedArchive | ||
} | ||
|
||
return invokeUnpack(r, dest, options) | ||
return invokeUnpack(r, dest, options, root) | ||
} | ||
|
||
// Tar tars the requested path while chrooted to the specified root. | ||
func Tar(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) { | ||
if options == nil { | ||
options = &archive.TarOptions{} | ||
} | ||
return invokePack(srcPath, options, root) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it perhaps make sense to add
root
toarchive.TarOptions
instead of as a new argument?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, those are specific to tar format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; I was a bit in doubt; had a quick look at other options, and wasn't sure if all the existing ones were really Tar-specific, or just a mechanism to pass options for our specific purpose