diff --git a/client/client_test.go b/client/client_test.go index 1bfef0fbc..c2dae0dda 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -271,6 +271,7 @@ func TestClientGatewayIntegration(t *testing.T) { // gateway_container_mount_test.go testClientGatewayContainerMounts, + testClientGatewayContainerReadFileSpecial, testClientGatewayContainerPlatformPATH, testClientGatewayContainerSecretEnv, diff --git a/client/gateway_container_mount_test.go b/client/gateway_container_mount_test.go index 080b8a507..c820edde7 100644 --- a/client/gateway_container_mount_test.go +++ b/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) +} diff --git a/frontend/gateway/container/container.go b/frontend/gateway/container/container.go index 458e3d445..f18238ec0 100644 --- a/frontend/gateway/container/container.go +++ b/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 } diff --git a/util/openfile/openfile.go b/util/openfile/openfile.go index 8856b656d..ea562ebc2 100644 --- a/util/openfile/openfile.go +++ b/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 { diff --git a/util/openfile/openfile_linux.go b/util/openfile/openfile_linux.go index fe6b13c26..b45a94039 100644 --- a/util/openfile/openfile_linux.go +++ b/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} +} diff --git a/util/openfile/openfile_linux_test.go b/util/openfile/openfile_linux_test.go index 7cb24da80..efc9a03fa 100644 --- a/util/openfile/openfile_linux_test.go +++ b/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") +} diff --git a/util/openfile/openfile_nonlinux.go b/util/openfile/openfile_nonlinux.go index df3404cdd..72238ab26 100644 --- a/util/openfile/openfile_nonlinux.go +++ b/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) +}