Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions daemon/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package daemon

import (
"errors"
"fmt"
"io"
"os"
"path/filepath"
Expand Down Expand Up @@ -152,7 +153,33 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
// container ID.
opts := archive.TarResourceRebaseOpts(resolvedPath, filepath.Base(absPath))

data, err := chrootarchive.Tar(resolvedPath, opts, filepath.Dir(absPath))
// Get the directory where the file lives in the container.
ctrDir := filepath.Join(container.BaseFS, filepath.Dir(absPath))

// Evaluate any symlink that occurs and point to the actual dir.
// EvalSymlinks will look for the target on the host and will fail
// if not found. Ignore errors and assume it's OK in the container.
resolvedCtrDir, _ := filepath.EvalSymlinks(ctrDir)

fileInfo, err := os.Lstat(ctrDir)
if err != nil {
return nil, nil, fmt.Errorf("error retrieving fileinfo for %q", ctrDir)
}
// If resolvedCtrDir was a symlink, it will have the resolved directory from
// the containers root. i.e. `ln -s /tmp /test` in the container and
// resolvedCtrDir would have `/tmp` in it at this point. Add it back
// to the container.Basefs to get the full container target directory
// on the host.
if fileInfo.Mode()&os.ModeSymlink != 0 {
resolvedCtrDir = filepath.Join(container.BaseFS, resolvedCtrDir)

Choose a reason for hiding this comment

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

should we ensure that after the Join the path is still under container.BaseFS?

Could happen that resolvedCtrDir has .. components, so it can potentially point outside of the basefs once we do the join?

Copy link
Author

Choose a reason for hiding this comment

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

I played with it tonight, and excellent spot. I think I've a solution for that, testing now.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I played more and add a symlink in the container like: ln -s /tmp ../../../../../../testy and that just resolved to the root directory of the container. However I did come up with a quick check that will make it completely safe, so I've added it.

}

// Make sure we didn't leak outside of container.
if !strings.HasPrefix(resolvedCtrDir, container.BaseFS ) {
return nil, nil, fmt.Errorf("error targeted directory is not within the container %s", resolvedCtrDir)
}

data, err := chrootarchive.Tar(resolvedPath, opts, resolvedCtrDir)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -271,7 +298,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path
}
}

if err := chrootarchive.UntarWithRoot(content, resolvedPath, options, absPath); err != nil {
if err := chrootarchive.UntarWithRoot(content, resolvedPath, options, resolvedPath); err != nil {
return err
}

Expand Down