Skip to content

Commit

Permalink
Merge commit from fork
Browse files Browse the repository at this point in the history
[release-1.37] fix TOCTOU error when bind and cache mounts use "src" values
  • Loading branch information
nalind authored Jan 20, 2025
2 parents 55ee4ec + 31db4f8 commit 419d6fb
Show file tree
Hide file tree
Showing 28 changed files with 1,251 additions and 252 deletions.
37 changes: 33 additions & 4 deletions cmd/buildah/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@ import (
"strings"

"github.com/containers/buildah"
"github.com/containers/buildah/internal/tmpdir"
"github.com/containers/buildah/internal/volumes"
buildahcli "github.com/containers/buildah/pkg/cli"
"github.com/containers/buildah/pkg/overlay"
"github.com/containers/buildah/pkg/parse"
"github.com/containers/buildah/util"
"github.com/containers/storage/pkg/mount"
"github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -108,6 +111,16 @@ func runCmd(c *cobra.Command, args []string, iopts runInputOptions) error {
return errors.New("command must be specified")
}

tmpDir, err := os.MkdirTemp(tmpdir.GetTempDir(), "buildahvolume")
if err != nil {
return fmt.Errorf("creating temporary directory: %w", err)
}
defer func() {
if err := os.Remove(tmpDir); err != nil {
logrus.Debugf("removing should-be-empty temporary directory %q: %v", tmpDir, err)
}
}()

store, err := getStore(c)
if err != nil {
return err
Expand Down Expand Up @@ -179,14 +192,30 @@ func runCmd(c *cobra.Command, args []string, iopts runInputOptions) error {
if err != nil {
return fmt.Errorf("building system context: %w", err)
}
mounts, mountedImages, targetLocks, err := volumes.GetVolumes(systemContext, store, iopts.volumes, iopts.mounts, iopts.contextDir, iopts.workingDir)
mounts, mountedImages, intermediateMounts, _, targetLocks, err := volumes.GetVolumes(systemContext, store, builder.MountLabel, iopts.volumes, iopts.mounts, iopts.contextDir, iopts.workingDir, tmpDir)
if err != nil {
return err
}
defer volumes.UnlockLockArray(targetLocks)
defer func() {
if err := overlay.CleanupContent(tmpDir); err != nil {
logrus.Debugf("unmounting overlay mounts under %q: %v", tmpDir, err)
}
for _, intermediateMount := range intermediateMounts {
if err := mount.Unmount(intermediateMount); err != nil {
logrus.Debugf("unmounting mount %q: %v", intermediateMount, err)
}
if err := os.Remove(intermediateMount); err != nil {
logrus.Debugf("removing should-be-empty mount directory %q: %v", intermediateMount, err)
}
}
for _, mountedImage := range mountedImages {
if _, err := store.UnmountImage(mountedImage, false); err != nil {
logrus.Debugf("unmounting image %q: %v", mountedImage, err)
}
}
volumes.UnlockLockArray(targetLocks)
}()
options.Mounts = mounts
// Run() will automatically clean them up.
options.ExternalImageMounts = mountedImages
options.CgroupManager = globalFlagResults.CgroupManager

runerr := builder.Run(args, options)
Expand Down
40 changes: 23 additions & 17 deletions define/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,13 @@ type SBOMScanOptions struct {
MergeStrategy SBOMMergeStrategy // how to merge the outputs of multiple scans
}

// TempDirForURL checks if the passed-in string looks like a URL or -. If it is,
// TempDirForURL creates a temporary directory, arranges for its contents to be
// the contents of that URL, and returns the temporary directory's path, along
// with the name of a subdirectory which should be used as the build context
// (which may be empty or "."). Removal of the temporary directory is the
// responsibility of the caller. If the string doesn't look like a URL,
// TempDirForURL returns empty strings and a nil error code.
// TempDirForURL checks if the passed-in string looks like a URL or "-". If it
// is, TempDirForURL creates a temporary directory, arranges for its contents
// to be the contents of that URL, and returns the temporary directory's path,
// along with the relative name of a subdirectory which should be used as the
// build context (which may be empty or "."). Removal of the temporary
// directory is the responsibility of the caller. If the string doesn't look
// like a URL or "-", TempDirForURL returns empty strings and a nil error code.
func TempDirForURL(dir, prefix, url string) (name string, subdir string, err error) {
if !strings.HasPrefix(url, "http://") &&
!strings.HasPrefix(url, "https://") &&
Expand All @@ -188,19 +188,24 @@ func TempDirForURL(dir, prefix, url string) (name string, subdir string, err err
if err != nil {
return "", "", fmt.Errorf("creating temporary directory for %q: %w", url, err)
}
downloadDir := filepath.Join(name, "download")
if err = os.MkdirAll(downloadDir, 0o700); err != nil {
return "", "", fmt.Errorf("creating directory %q for %q: %w", downloadDir, url, err)
}
urlParsed, err := urlpkg.Parse(url)
if err != nil {
return "", "", fmt.Errorf("parsing url %q: %w", url, err)
}
if strings.HasPrefix(url, "git://") || strings.HasSuffix(urlParsed.Path, ".git") {
combinedOutput, gitSubDir, err := cloneToDirectory(url, name)
combinedOutput, gitSubDir, err := cloneToDirectory(url, downloadDir)
if err != nil {
if err2 := os.RemoveAll(name); err2 != nil {
logrus.Debugf("error removing temporary directory %q: %v", name, err2)
}
return "", "", fmt.Errorf("cloning %q to %q:\n%s: %w", url, name, string(combinedOutput), err)
}
return name, gitSubDir, nil
logrus.Debugf("Build context is at %q", filepath.Join(downloadDir, gitSubDir))
return name, filepath.Join(filepath.Base(downloadDir), gitSubDir), nil
}
if strings.HasPrefix(url, "github.com/") {
ghurl := url
Expand All @@ -209,28 +214,29 @@ func TempDirForURL(dir, prefix, url string) (name string, subdir string, err err
subdir = path.Base(ghurl) + "-master"
}
if strings.HasPrefix(url, "http://") || strings.HasPrefix(url, "https://") {
err = downloadToDirectory(url, name)
err = downloadToDirectory(url, downloadDir)
if err != nil {
if err2 := os.RemoveAll(name); err2 != nil {
logrus.Debugf("error removing temporary directory %q: %v", name, err2)
}
return "", subdir, err
return "", "", err
}
return name, subdir, nil
logrus.Debugf("Build context is at %q", filepath.Join(downloadDir, subdir))
return name, filepath.Join(filepath.Base(downloadDir), subdir), nil
}
if url == "-" {
err = stdinToDirectory(name)
err = stdinToDirectory(downloadDir)
if err != nil {
if err2 := os.RemoveAll(name); err2 != nil {
logrus.Debugf("error removing temporary directory %q: %v", name, err2)
}
return "", subdir, err
return "", "", err
}
logrus.Debugf("Build context is at %q", name)
return name, subdir, nil
logrus.Debugf("Build context is at %q", filepath.Join(downloadDir, subdir))
return name, filepath.Join(filepath.Base(downloadDir), subdir), nil
}
logrus.Debugf("don't know how to retrieve %q", url)
if err2 := os.Remove(name); err2 != nil {
if err2 := os.RemoveAll(name); err2 != nil {
logrus.Debugf("error removing temporary directory %q: %v", name, err2)
}
return "", "", errors.New("unreachable code reached")
Expand Down
14 changes: 8 additions & 6 deletions docs/buildah-run.1.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ BUILDAH\_ISOLATION environment variable. `export BUILDAH_ISOLATION=oci`

Attach a filesystem mount to the container

Current supported mount TYPES are bind, cache, secret and tmpfs.
Current supported mount TYPES are bind, cache, secret and tmpfs. Writes to `bind` and `tmpfs` mounts are discarded after the command finishes, while changes to `cache` mounts persist across uses.

e.g.

Expand All @@ -130,19 +130,19 @@ Current supported mount TYPES are bind, cache, secret and tmpfs.

Common Options:

· src, source: mount source spec for bind and volume. Mandatory for bind. If `from` is specified, `src` is the subpath in the `from` field.
· src, source: mount source spec for bind and cache. Mandatory for bind. If `from` is specified, `src` is the subpath in the `from` field.

· dst, destination, target: mount destination spec.
· dst, destination, target: location where the command being run should see the content being mounted.

· ro, read-only: true or false (default).
· ro, read-only: (default true for `type=bind`, false for `type=tmpfs`, `type=cache`).

Options specific to bind:

· bind-propagation: shared, slave, private, rshared, rslave, or rprivate(default). See also mount(2). <sup>[[1]](#Footnote1)</sup>

. bind-nonrecursive: do not setup a recursive bind mount. By default it is recursive.

· from: stage or image name for the root of the source. Defaults to the build context.
· from: image name for the root of the source. Defaults to **--contextdir**, mandatory if **--contextdir** was not specified.

· z: Set shared SELinux label on mounted destination. Use if SELinux is enabled on host machine.

Expand All @@ -162,7 +162,7 @@ Current supported mount TYPES are bind, cache, secret and tmpfs.

Options specific to cache:

· id: Create a separate cache directory for a particular id.
· id: Distinguish this cache from other caches using this ID rather than the target mount path.

· mode: File mode for new cache directory in octal. Default 0755.

Expand All @@ -174,6 +174,8 @@ Current supported mount TYPES are bind, cache, secret and tmpfs.

· from: stage name for the root of the source. Defaults to host cache directory.

· sharing: Whether other users of this cache need to wait for this command to complete (`sharing=locked`) or not (`sharing=shared`, which is the default).

· z: Set shared SELinux label on mounted destination. Enabled by default if SELinux is enabled on the host machine.

· Z: Set private SELinux label on mounted destination. Use if SELinux is enabled on host machine.
Expand Down
25 changes: 21 additions & 4 deletions imagebuildah/stage_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,12 @@ func (s *StageExecutor) runStageMountPoints(mountList []string) (map[string]inte
// to `mountPoint` replaced from additional
// build-context. Reason: Parser will use this
// `from` to refer from stageMountPoints map later.
stageMountPoints[from] = internal.StageMountDetails{IsStage: false, DidExecute: true, MountPoint: mountPoint}
stageMountPoints[from] = internal.StageMountDetails{
IsAdditionalBuildContext: true,
IsImage: true,
DidExecute: true,
MountPoint: mountPoint,
}
break
} else {
// Most likely this points to path on filesystem
Expand Down Expand Up @@ -667,7 +672,11 @@ func (s *StageExecutor) runStageMountPoints(mountList []string) (map[string]inte
mountPoint = additionalBuildContext.DownloadedCache
}
}
stageMountPoints[from] = internal.StageMountDetails{IsStage: true, DidExecute: true, MountPoint: mountPoint}
stageMountPoints[from] = internal.StageMountDetails{
IsAdditionalBuildContext: true,
DidExecute: true,
MountPoint: mountPoint,
}
break
}
}
Expand All @@ -678,14 +687,22 @@ func (s *StageExecutor) runStageMountPoints(mountList []string) (map[string]inte
return nil, err
}
if otherStage, ok := s.executor.stages[from]; ok && otherStage.index < s.index {
stageMountPoints[from] = internal.StageMountDetails{IsStage: true, DidExecute: otherStage.didExecute, MountPoint: otherStage.mountPoint}
stageMountPoints[from] = internal.StageMountDetails{
IsStage: true,
DidExecute: otherStage.didExecute,
MountPoint: otherStage.mountPoint,
}
break
} else {
mountPoint, err := s.getImageRootfs(s.ctx, from)
if err != nil {
return nil, fmt.Errorf("%s from=%s: no stage or image found with that name", flag, from)
}
stageMountPoints[from] = internal.StageMountDetails{IsStage: false, DidExecute: true, MountPoint: mountPoint}
stageMountPoints[from] = internal.StageMountDetails{
IsImage: true,
DidExecute: true,
MountPoint: mountPoint,
}
break
}
default:
Expand Down
39 changes: 39 additions & 0 deletions internal/open/open.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package open

import (
"errors"
"fmt"
"syscall"
)

// InChroot opens the file at `path` after chrooting to `root` and then
// changing its working directory to `wd`. Both `wd` and `path` are evaluated
// in the chroot.
// Returns a file handle, an Errno value if there was an error and the
// underlying error was a standard library error code, and a non-empty error if
// one was detected.
func InChroot(root, wd, path string, mode int, perm uint32) (fd int, errno syscall.Errno, err error) {
requests := requests{
Root: root,
Wd: wd,
Open: []request{
{
Path: path,
Mode: mode,
Perms: perm,
},
},
}
results := inChroot(requests)
if len(results.Open) != 1 {
return -1, 0, fmt.Errorf("got %d results back instead of 1", len(results.Open))
}
if results.Open[0].Err != "" {
if results.Open[0].Errno != 0 {
err = fmt.Errorf("%s: %w", results.Open[0].Err, results.Open[0].Errno)
} else {
err = errors.New(results.Open[0].Err)
}
}
return int(results.Open[0].Fd), results.Open[0].Errno, err
}
88 changes: 88 additions & 0 deletions internal/open/open_linux.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package open

import (
"bytes"
"encoding/json"
"errors"
"fmt"
"os"
"strings"

"github.com/containers/storage/pkg/reexec"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
)

const (
bindFdToPathCommand = "buildah-bind-fd-to-path"
)

func init() {
reexec.Register(bindFdToPathCommand, bindFdToPathMain)
}

// BindFdToPath creates a bind mount from the open file (which is actually a
// directory) to the specified location. If it succeeds, the caller will need
// to unmount the targetPath when it's finished using it. Regardless, it
// closes the passed-in descriptor.
func BindFdToPath(fd uintptr, targetPath string) error {
f := os.NewFile(fd, "passed-in directory descriptor")
defer func() {
if err := f.Close(); err != nil {
logrus.Debugf("closing descriptor %d after attempting to bind to %q: %v", fd, targetPath, err)
}
}()
pipeReader, pipeWriter, err := os.Pipe()
if err != nil {
return err
}
cmd := reexec.Command(bindFdToPathCommand)
cmd.Stdin = pipeReader
var stdout bytes.Buffer
var stderr bytes.Buffer
cmd.Stdout, cmd.Stderr = &stdout, &stderr
cmd.ExtraFiles = append(cmd.ExtraFiles, f)

err = cmd.Start()
pipeReader.Close()
if err != nil {
pipeWriter.Close()
return fmt.Errorf("starting child: %w", err)
}

encoder := json.NewEncoder(pipeWriter)
if err := encoder.Encode(&targetPath); err != nil {
return fmt.Errorf("sending target path to child: %w", err)
}
pipeWriter.Close()
err = cmd.Wait()
trimmedOutput := strings.TrimSpace(stdout.String()) + strings.TrimSpace(stderr.String())
if err != nil {
if len(trimmedOutput) > 0 {
err = fmt.Errorf("%s: %w", trimmedOutput, err)
}
} else {
if len(trimmedOutput) > 0 {
err = errors.New(trimmedOutput)
}
}
return err
}

func bindFdToPathMain() {
var targetPath string
decoder := json.NewDecoder(os.Stdin)
if err := decoder.Decode(&targetPath); err != nil {
fmt.Fprintf(os.Stderr, "error decoding target path")
os.Exit(1)
}
if err := unix.Fchdir(3); err != nil {
fmt.Fprintf(os.Stderr, "fchdir(): %v", err)
os.Exit(1)
}
if err := unix.Mount(".", targetPath, "bind", unix.MS_BIND, ""); err != nil {
fmt.Fprintf(os.Stderr, "bind-mounting passed-in directory to %q: %v", targetPath, err)
os.Exit(1)
}
os.Exit(0)
}
Loading

0 comments on commit 419d6fb

Please sign in to comment.