gateway: resolve container reads inside the mount

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
(cherry picked from commit 64137a91962a5fdbe1d6c49d9915ad2501190600)
(cherry picked from commit 929ad12778e82215bffbdb593dccdf124912ac2b)

client/client_test.go

@@ -271,6 +271,7 @@ func TestClientGatewayIntegration(t *testing.T) {
 
 		// gateway_container_mount_test.go
 		testClientGatewayContainerMounts,
+		testClientGatewayContainerReadFileSpecial,
 		testClientGatewayContainerPlatformPATH,
 		testClientGatewayContainerSecretEnv,
 

client/gateway_container_mount_test.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/moby/buildkit/client/llb"
 	"github.com/moby/buildkit/frontend/gateway/client"
@@ -384,3 +385,105 @@ func testClientGatewayContainerSecretEnv(t *testing.T, sb integration.Sandbox) {
 
 	checkAllReleasable(t, c, sb, true)
 }
+
+// testClientGatewayContainerReadFileSpecial checks that ReadFile only reads
+// regular files. A container can mknod a device node or mkfifo with the default
+// capability set; reading the former daemon-side would resolve its rdev against
+// the host device table, and the latter would block the daemon in open(2).
+func testClientGatewayContainerReadFileSpecial(t *testing.T, sb integration.Sandbox) {
+	requiresLinux(t)
+
+	ctx := sb.Context()
+	c, err := New(ctx, sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	product := "buildkit_test"
+
+	// a user namespace cannot mknod a device node, so the device case is only
+	// reachable, and only testable, on a rootful worker
+	withDevice := !sb.Rootless()
+
+	b := func(ctx context.Context, c client.Client) (*client.Result, error) {
+		setup := `mkfifo /fifo && printf 0123456789 > /regular`
+		if withDevice {
+			// 5,0 is /dev/tty, which fails open with ENXIO when the daemon has
+			// no controlling terminal. Rejecting it as a non-regular file
+			// therefore shows the driver was never entered, which a device
+			// like /dev/null could not distinguish.
+			setup = `mknod /dev-node c 5 0 && ` + setup
+		}
+		st := llb.Image("busybox:latest").Run(
+			llb.Shlexf(`sh -c %q`, setup),
+		).Root()
+
+		def, err := st.Marshal(ctx)
+		if err != nil {
+			return nil, err
+		}
+		r, err := c.Solve(ctx, client.SolveRequest{Definition: def.ToPB()})
+		if err != nil {
+			return nil, err
+		}
+
+		ctr, err := c.NewContainer(ctx, client.NewContainerRequest{
+			Mounts: []client.Mount{{
+				Dest:      "/",
+				MountType: pb.MountType_BIND,
+				Ref:       r.Ref,
+			}},
+		})
+		if err != nil {
+			return nil, err
+		}
+		defer ctr.Release(context.WithoutCancel(ctx))
+
+		read := func(name string, rng *client.FileRange) ([]byte, error) {
+			return ctr.ReadFile(ctx, client.ReadContainerRequest{
+				ReadRequest: client.ReadRequest{Filename: name, Range: rng},
+				MountIndex:  0,
+			})
+		}
+
+		if withDevice {
+			_, err = read("/dev-node", nil)
+			require.ErrorContains(t, err, "not a regular file")
+		}
+
+		// Bound this one. Against vulnerable code the daemon blocks in open(2)
+		// on a fifo with no writer and never answers, so an unbounded call would
+		// hang the suite instead of failing it.
+		fifoCtx, cancel := context.WithTimeoutCause(ctx, 30*time.Second,
+			errors.New("fifo read did not return"))
+		defer cancel()
+		_, err = ctr.ReadFile(fifoCtx, client.ReadContainerRequest{
+			ReadRequest: client.ReadRequest{Filename: "/fifo"},
+			MountIndex:  0,
+		})
+		require.ErrorContains(t, err, "not a regular file")
+
+		dt, err := read("/regular", nil)
+		require.NoError(t, err)
+		require.Equal(t, []byte("0123456789"), dt)
+
+		dt, err = read("/regular", &client.FileRange{Offset: 2, Length: 3})
+		require.NoError(t, err)
+		require.Equal(t, []byte("234"), dt)
+
+		// a negative range must not reach ReadAt, whose error names the
+		// daemon-side path
+		for _, rng := range []client.FileRange{{Offset: -1, Length: 3}, {Offset: 0, Length: -1}} {
+			_, err = read("/regular", &rng)
+			require.ErrorContains(t, err, "invalid range")
+		}
+
+		_, err = read("/missing", nil)
+		require.ErrorContains(t, err, "/missing")
+		require.NotContains(t, err.Error(), "/tmp/", "daemon-side path leaked")
+
+		return client.NewResult(), nil
+	}
+
+	_, err = c.Build(ctx, SolveOpt{}, product, b, nil)
+	require.NoError(t, err)
+}

frontend/gateway/container/container.go

@@ -4,6 +4,7 @@ import (
 	"cmp"
 	"context"
 	"fmt"
+	"io"
 	"io/fs"
 	"os"
 	"path"
@@ -16,6 +17,7 @@ import (
 
 	"github.com/moby/buildkit/session/secrets"
 	"github.com/moby/buildkit/util/bklog"
+	"github.com/moby/buildkit/util/openfile"
 	"github.com/moby/buildkit/util/system"
 
 	"github.com/moby/buildkit/cache"
@@ -449,8 +451,8 @@ func (gwCtr *gatewayContainer) Release(ctx context.Context) error {
 	return stack.Enable(err2)
 }
 
-func (gwCtr *gatewayContainer) ReadFile(ctx context.Context, req client.ReadContainerRequest) ([]byte, error) {
-	fsys, err := gwCtr.mount(ctx, req.MountIndex)
+func (gwCtr *gatewayContainer) ReadFile(ctx context.Context, req client.ReadContainerRequest) (_ []byte, retErr error) {
+	m, err := gwCtr.localMount(ctx, req.MountIndex)
 	if err != nil {
 		return nil, err
 	}
@@ -459,7 +461,37 @@ func (gwCtr *gatewayContainer) ReadFile(ctx context.Context, req client.ReadCont
 	if err != nil {
 		return nil, err
 	}
-	return fs.ReadFile(fsys, fpath)
+
+	// os.Root names paths relative to the mount, while reads on the open file
+	// name the daemon-side path. Report what the caller asked for in both cases.
+	defer func() {
+		var pathErr *os.PathError
+		if errors.As(retErr, &pathErr) {
+			pathErr.Path = req.Filename
+		}
+	}()
+
+	// The mount is mutable while the container runs, so resolution and open
+	// have to be one operation. Anything but a regular file is refused without
+	// its driver's open method running.
+	f, err := openfile.RegularInRoot(m.Root.Name(), fpath)
+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()
+
+	var rdr io.Reader = f
+	if req.Range != nil {
+		if req.Range.Offset < 0 || req.Range.Length < 0 {
+			return nil, errors.Errorf("invalid range for %s", req.Filename)
+		}
+		rdr = io.NewSectionReader(f, int64(req.Range.Offset), int64(req.Range.Length))
+	}
+	dt, err := io.ReadAll(rdr)
+	if err != nil {
+		return nil, errors.WithStack(err)
+	}
+	return dt, nil
 }
 
 func (gwCtr *gatewayContainer) ReadDir(ctx context.Context, req client.ReadDirContainerRequest) ([]*fstypes.Stat, error) {
@@ -525,6 +557,14 @@ func (gwCtr *gatewayContainer) StatFile(ctx context.Context, req client.StatCont
 }
 
 func (gwCtr *gatewayContainer) mount(ctx context.Context, index int) (fs.FS, error) {
+	m, err := gwCtr.localMount(ctx, index)
+	if err != nil {
+		return nil, err
+	}
+	return m.FS, nil
+}
+
+func (gwCtr *gatewayContainer) localMount(ctx context.Context, index int) (*gatewayContainerMount, error) {
 	// No lock needed for this because the number of mounts does
 	// not change.
 	if index < 0 || index >= len(gwCtr.localMounts) {
@@ -538,7 +578,7 @@ func (gwCtr *gatewayContainer) mount(ctx context.Context, index int) (fs.FS, err
 
 	// Already mounted?
 	if mount.FS != nil {
-		return mount.FS, nil
+		return &gwCtr.localMounts[index], nil
 	}
 
 	// Defensively check that this mount really exists.
@@ -572,9 +612,9 @@ func (gwCtr *gatewayContainer) mount(ctx context.Context, index int) (fs.FS, err
 		return root.Close()
 	})
 
-	f := root.FS()
-	gwCtr.localMounts[index].FS = f
-	return f, nil
+	gwCtr.localMounts[index].Root = root
+	gwCtr.localMounts[index].FS = root.FS()
+	return &gwCtr.localMounts[index], nil
 }
 
 type gatewayContainerProcess struct {
@@ -735,6 +775,7 @@ func relpath(p string) (string, error) {
 }
 
 type gatewayContainerMount struct {
-	Src executor.Mountable
-	FS  fs.FS
+	Src  executor.Mountable
+	Root *os.Root
+	FS   fs.FS
 }

util/openfile/openfile.go

@@ -23,6 +23,16 @@ func Regular(p string) (*os.File, error) {
 	return openRegular(p)
 }
 
+// RegularInRoot opens name inside root for reading and fails unless it is a
+// regular file. Unlike Regular it resolves name itself, confined to root, and
+// does so as a single operation: a process running against the same mount
+// cannot swap a path component between the resolution and the open. Use it
+// when the mount may be mutated while it is read, as a gateway container mount
+// can be.
+func RegularInRoot(root, name string) (*os.File, error) {
+	return openRegularInRoot(root, name)
+}
+
 func checkRegular(f *os.File, name string) error {
 	fi, err := f.Stat()
 	if err != nil {

util/openfile/openfile_linux.go

@@ -3,7 +3,9 @@ package openfile
 import (
 	"os"
 	"strconv"
+	"syscall"
 
+	pathrs "github.com/cyphar/filepath-securejoin/pathrs-lite"
 	"github.com/pkg/errors"
 	"golang.org/x/sys/unix"
 )
@@ -35,3 +37,42 @@ func openRegular(p string) (*os.File, error) {
 	}
 	return f, nil
 }
+
+// openRegularInRoot resolves name within root and pins the result in one
+// operation. pathrs hands back an O_PATH descriptor, so the resolved inode is
+// never opened for real until it has been shown to be a regular file, and
+// Reopen upgrades that exact inode rather than resolving the path again.
+func openRegularInRoot(root, name string) (*os.File, error) {
+	pinned, err := pathrs.OpenInRoot(root, name)
+	if err != nil {
+		return nil, errors.WithStack(cleanPathError(err, name))
+	}
+	defer pinned.Close()
+
+	if err := checkRegular(pinned, name); err != nil {
+		return nil, err
+	}
+
+	f, err := pathrs.Reopen(pinned, os.O_RDONLY)
+	if err != nil {
+		return nil, errors.WithStack(err)
+	}
+	return f, nil
+}
+
+// cleanPathError restates err as an *os.PathError naming name. Resolution
+// errors from pathrs quote the daemon-side path they failed on, which must not
+// reach the client; the errno is preserved so errors.Is still identifies it.
+func cleanPathError(err error, name string) error {
+	var errno syscall.Errno
+	if errors.As(err, &errno) {
+		return &os.PathError{Op: "open", Path: name, Err: errno}
+	}
+	// pathrs' pre-openat2 fallback reports breakout detections that are not
+	// errnos, and whose diagnostics can quote the path they resolved. Keep the
+	// EXDEV signal those carry and drop everything else.
+	if errors.Is(err, unix.EXDEV) {
+		return &os.PathError{Op: "open", Path: name, Err: unix.EXDEV}
+	}
+	return &os.PathError{Op: "open", Path: name, Err: os.ErrInvalid}
+}

util/openfile/openfile_linux_test.go

@@ -1,9 +1,12 @@
 package openfile
 
 import (
+	"os"
 	"path/filepath"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"golang.org/x/sys/unix"
 )
 
@@ -16,3 +19,25 @@ func TestRejectsDeviceNode(t *testing.T) {
 	}
 	requireRejected(t, p)
 }
+
+// A device whose open observably fails tells us whether the driver was entered
+// at all: opening /dev/tty without a controlling terminal returns ENXIO. If the
+// helper reports ErrNotRegular instead, the inode was never opened.
+func TestRegularInRootDoesNotEnterDriver(t *testing.T) {
+	dir := t.TempDir()
+	p := filepath.Join(dir, "tty")
+	if err := unix.Mknod(p, unix.S_IFCHR|0600, int(unix.Mkdev(5, 0))); err != nil {
+		t.Skipf("cannot create device node: %v", err)
+	}
+
+	f, err := os.OpenFile(p, os.O_RDONLY|unix.O_NONBLOCK, 0)
+	if err == nil {
+		f.Close()
+		t.Skip("this host opens /dev/tty successfully, the check would not be conclusive")
+	}
+	require.ErrorIs(t, err, unix.ENXIO, "expected a plain open to enter the driver")
+
+	_, err = RegularInRoot(dir, "tty")
+	require.ErrorIs(t, err, ErrNotRegular)
+	require.NotErrorIs(t, err, unix.ENXIO, "the driver was entered")
+}

util/openfile/openfile_nonlinux.go

@@ -5,6 +5,8 @@ package openfile
 import (
 	"os"
 
+	"github.com/containerd/continuity/fs"
+
 	"github.com/pkg/errors"
 )
 
@@ -34,3 +36,14 @@ func openRegular(p string) (*os.File, error) {
 	}
 	return f, nil
 }
+
+// openRegularInRoot has no race-safe equivalent off Linux: resolution and open
+// are separate steps, so a mount mutated in between can still redirect the
+// open. The inode type is still checked before and after opening.
+func openRegularInRoot(root, name string) (*os.File, error) {
+	p, err := fs.RootPath(root, name)
+	if err != nil {
+		return nil, errors.WithStack(err)
+	}
+	return openRegular(p)
+}