From f781267af1acb688e94740e1fdc22c1bf587d7fd Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 19 Dec 2023 20:23:10 -0800 Subject: [PATCH] exec: add extra validation for submount sources While submount paths were already validated there are some cases where the parent mount may not be immutable while the submount is created. Signed-off-by: Tonis Tiigi (cherry picked from commit 2529ec4121bcd8c35bcd96218083da175c2e5b77) (cherry picked from commit cbc233b3b695918d92fd5b1407b829296c53db70) --- executor/oci/spec.go | 30 ++++++++++-------- executor/oci/spec_freebsd.go | 15 +++++++++ executor/oci/spec_linux.go | 57 +++++++++++++++++++++++++++++++++++ executor/oci/spec_windows.go | 11 +++++++ snapshot/localmounter.go | 35 +++++++++++++++------ snapshot/localmounter_unix.go | 45 +++++++++++++++++++-------- 6 files changed, 159 insertions(+), 34 deletions(-) create mode 100644 executor/oci/spec_freebsd.go create mode 100644 executor/oci/spec_linux.go diff --git a/executor/oci/spec.go b/executor/oci/spec.go index 8842f362a3ef..544c68d9a96a 100644 --- a/executor/oci/spec.go +++ b/executor/oci/spec.go @@ -12,7 +12,6 @@ import ( "github.com/containerd/containerd/namespaces" "github.com/containerd/containerd/oci" "github.com/containerd/containerd/pkg/userns" - "github.com/containerd/continuity/fs" "github.com/docker/docker/pkg/idtools" "github.com/mitchellh/hashstructure/v2" "github.com/moby/buildkit/executor" @@ -215,6 +214,7 @@ func GenerateSpec(ctx context.Context, meta executor.Meta, mounts []executor.Mou type mountRef struct { mount mount.Mount unmount func() error + subRefs map[string]mountRef } type submounts struct { @@ -233,10 +233,17 @@ func (s *submounts) subMount(m mount.Mount, subPath string) (mount.Mount, error) return mount.Mount{}, err } if mr, ok := s.m[h]; ok { - sm, err := sub(mr.mount, subPath) + if sm, ok := mr.subRefs[subPath]; ok { + return sm.mount, nil + } + sm, unmount, err := sub(mr.mount, subPath) if err != nil { return mount.Mount{}, err } + mr.subRefs[subPath] = mountRef{ + mount: sm, + unmount: unmount, + } return sm, nil } @@ -261,12 +268,17 @@ func (s *submounts) subMount(m mount.Mount, subPath string) (mount.Mount, error) Options: opts, }, unmount: lm.Unmount, + subRefs: map[string]mountRef{}, } - sm, err := sub(s.m[h].mount, subPath) + sm, unmount, err := sub(s.m[h].mount, subPath) if err != nil { return mount.Mount{}, err } + s.m[h].subRefs[subPath] = mountRef{ + mount: sm, + unmount: unmount, + } return sm, nil } @@ -276,6 +288,9 @@ func (s *submounts) cleanup() { for _, m := range s.m { func(m mountRef) { go func() { + for _, sm := range m.subRefs { + sm.unmount() + } m.unmount() wg.Done() }() @@ -284,15 +299,6 @@ func (s *submounts) cleanup() { wg.Wait() } -func sub(m mount.Mount, subPath string) (mount.Mount, error) { - src, err := fs.RootPath(m.Source, subPath) - if err != nil { - return mount.Mount{}, err - } - m.Source = src - return m, nil -} - func specMapping(s []idtools.IDMap) []specs.LinuxIDMapping { var ids []specs.LinuxIDMapping for _, item := range s { diff --git a/executor/oci/spec_freebsd.go b/executor/oci/spec_freebsd.go new file mode 100644 index 000000000000..0810bc428867 --- /dev/null +++ b/executor/oci/spec_freebsd.go @@ -0,0 +1,15 @@ +package oci + +import ( + "github.com/containerd/containerd/mount" + "github.com/containerd/continuity/fs" +) + +func sub(m mount.Mount, subPath string) (mount.Mount, func() error, error) { + src, err := fs.RootPath(m.Source, subPath) + if err != nil { + return mount.Mount{}, nil, err + } + m.Source = src + return m, func() error { return nil }, nil +} diff --git a/executor/oci/spec_linux.go b/executor/oci/spec_linux.go new file mode 100644 index 000000000000..abbf0879d87a --- /dev/null +++ b/executor/oci/spec_linux.go @@ -0,0 +1,57 @@ +//go:build linux +// +build linux + +package oci + +import ( + "os" + "strconv" + + "github.com/containerd/containerd/mount" + "github.com/containerd/continuity/fs" + "github.com/moby/buildkit/snapshot" + "github.com/pkg/errors" + "golang.org/x/sys/unix" +) + +func sub(m mount.Mount, subPath string) (mount.Mount, func() error, error) { + var retries = 10 + root := m.Source + for { + src, err := fs.RootPath(root, subPath) + if err != nil { + return mount.Mount{}, nil, err + } + // similar to runc.WithProcfd + fh, err := os.OpenFile(src, unix.O_PATH|unix.O_CLOEXEC, 0) + if err != nil { + return mount.Mount{}, nil, err + } + + fdPath := "/proc/self/fd/" + strconv.Itoa(int(fh.Fd())) + if resolved, err := os.Readlink(fdPath); err != nil { + fh.Close() + return mount.Mount{}, nil, err + } else if resolved != src { + retries-- + if retries <= 0 { + fh.Close() + return mount.Mount{}, nil, errors.Errorf("unable to safely resolve subpath %s", subPath) + } + fh.Close() + continue + } + + m.Source = fdPath + lm := snapshot.LocalMounterWithMounts([]mount.Mount{m}, snapshot.ForceRemount()) + mp, err := lm.Mount() + if err != nil { + fh.Close() + return mount.Mount{}, nil, err + } + m.Source = mp + fh.Close() // release the fd, we don't need it anymore + + return m, lm.Unmount, nil + } +} diff --git a/executor/oci/spec_windows.go b/executor/oci/spec_windows.go index ef7c67363ea6..261bbb593084 100644 --- a/executor/oci/spec_windows.go +++ b/executor/oci/spec_windows.go @@ -7,7 +7,9 @@ import ( "fmt" "path/filepath" + "github.com/containerd/containerd/mount" "github.com/containerd/containerd/oci" + "github.com/containerd/continuity/fs" "github.com/docker/docker/pkg/idtools" "github.com/moby/buildkit/solver/pb" specs "github.com/opencontainers/runtime-spec/specs-go" @@ -67,3 +69,12 @@ func getTracingSocket() string { func cgroupV2NamespaceSupported() bool { return false } + +func sub(m mount.Mount, subPath string) (mount.Mount, func() error, error) { + src, err := fs.RootPath(m.Source, subPath) + if err != nil { + return mount.Mount{}, nil, err + } + m.Source = src + return m, func() error { return nil }, nil +} diff --git a/snapshot/localmounter.go b/snapshot/localmounter.go index 9ddb7c1af642..304eebc9e02d 100644 --- a/snapshot/localmounter.go +++ b/snapshot/localmounter.go @@ -11,22 +11,39 @@ type Mounter interface { Unmount() error } +type LocalMounterOpt func(*localMounter) + // LocalMounter is a helper for mounting mountfactory to temporary path. In // addition it can mount binds without privileges -func LocalMounter(mountable Mountable) Mounter { - return &localMounter{mountable: mountable} +func LocalMounter(mountable Mountable, opts ...LocalMounterOpt) Mounter { + lm := &localMounter{mountable: mountable} + for _, opt := range opts { + opt(lm) + } + return lm } // LocalMounterWithMounts is a helper for mounting to temporary path. In // addition it can mount binds without privileges -func LocalMounterWithMounts(mounts []mount.Mount) Mounter { - return &localMounter{mounts: mounts} +func LocalMounterWithMounts(mounts []mount.Mount, opts ...LocalMounterOpt) Mounter { + lm := &localMounter{mounts: mounts} + for _, opt := range opts { + opt(lm) + } + return lm } type localMounter struct { - mu sync.Mutex - mounts []mount.Mount - mountable Mountable - target string - release func() error + mu sync.Mutex + mounts []mount.Mount + mountable Mountable + target string + release func() error + forceRemount bool +} + +func ForceRemount() LocalMounterOpt { + return func(lm *localMounter) { + lm.forceRemount = true + } } diff --git a/snapshot/localmounter_unix.go b/snapshot/localmounter_unix.go index a4b7b1a9e409..0e1f40f298c4 100644 --- a/snapshot/localmounter_unix.go +++ b/snapshot/localmounter_unix.go @@ -5,6 +5,7 @@ package snapshot import ( "os" + "path/filepath" "syscall" "github.com/containerd/containerd/mount" @@ -34,30 +35,48 @@ func (lm *localMounter) Mount() (string, error) { } } + var isFile bool if len(lm.mounts) == 1 && (lm.mounts[0].Type == "bind" || lm.mounts[0].Type == "rbind") { - ro := false - for _, opt := range lm.mounts[0].Options { - if opt == "ro" { - ro = true - break + if !lm.forceRemount { + ro := false + for _, opt := range lm.mounts[0].Options { + if opt == "ro" { + ro = true + break + } } + if !ro { + return lm.mounts[0].Source, nil + } + } + fi, err := os.Stat(lm.mounts[0].Source) + if err != nil { + return "", err } - if !ro { - return lm.mounts[0].Source, nil + if !fi.IsDir() { + isFile = true } } - dir, err := os.MkdirTemp("", "buildkit-mount") + dest, err := os.MkdirTemp("", "buildkit-mount") if err != nil { return "", errors.Wrap(err, "failed to create temp dir") } - if err := mount.All(lm.mounts, dir); err != nil { - os.RemoveAll(dir) - return "", errors.Wrapf(err, "failed to mount %s: %+v", dir, lm.mounts) + if isFile { + dest = filepath.Join(dest, "file") + if err := os.WriteFile(dest, []byte{}, 0644); err != nil { + os.RemoveAll(dest) + return "", errors.Wrap(err, "failed to create temp file") + } + } + + if err := mount.All(lm.mounts, dest); err != nil { + os.RemoveAll(dest) + return "", errors.Wrapf(err, "failed to mount %s: %+v", dest, lm.mounts) } - lm.target = dir - return dir, nil + lm.target = dest + return dest, nil } func (lm *localMounter) Unmount() error {