daemon: Prevent checkpoint ID path traversal

Checkpoint restore joined the caller-controlled checkpoint ID to the
target container's checkpoint directory.

Require named checkpoint IDs to be one local path component before any
path construction or container lookup.

Invalid IDs now return HTTP 400.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

api/swagger.yaml

@@ -8927,6 +8927,10 @@ paths:
           description: "no error"
         304:
           description: "container already started"
+        400:
+          description: "bad parameter, including an invalid checkpoint ID"
+          schema:
+            $ref: "#/definitions/ErrorResponse"
         404:
           description: "no such container"
           schema:

daemon/checkpoint.go

@@ -2,30 +2,37 @@ package daemon
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/moby/moby/api/types/checkpoint"
 	"github.com/moby/moby/api/types/events"
 	"github.com/moby/moby/v2/daemon/names"
 	"github.com/moby/moby/v2/daemon/server/backend"
+	"github.com/moby/moby/v2/errdefs"
 )
 
-var (
-	validCheckpointNameChars   = names.RestrictedNameChars
-	validCheckpointNamePattern = names.RestrictedNamePattern
-)
-
-// getCheckpointDir verifies checkpoint directory for create,remove, list options and checks if checkpoint already exists
-func getCheckpointDir(checkDir, checkpointID, ctrName, ctrID, ctrCheckpointDir string, create bool) (string, error) {
-	var checkpointDir string
-	var err2 error
-	if checkDir != "" {
-		checkpointDir = checkDir
-	} else {
-		checkpointDir = ctrCheckpointDir
+func validateCheckpointID(checkpointID string) error {
+	if !names.RestrictedNamePattern.MatchString(checkpointID) || strings.HasSuffix(checkpointID, ".") {
+		return errdefs.InvalidParameter(fmt.Errorf("invalid checkpoint ID %q, only %s are allowed", checkpointID, names.RestrictedNameChars))
 	}
+	return nil
+}
+
+func checkpointRoot(checkDir, ctrCheckpointDir string) string {
+	if checkDir != "" {
+		return checkDir
+	}
+	return ctrCheckpointDir
+}
+
+// getCheckpointDir resolves and verifies a named checkpoint directory.
+func getCheckpointDir(checkDir, checkpointID, ctrName, ctrID, ctrCheckpointDir string, create bool) (string, error) {
+	checkpointDir := checkpointRoot(checkDir, ctrCheckpointDir)
+	var err2 error
 	checkpointAbsDir := filepath.Join(checkpointDir, checkpointID)
 	stat, err := os.Stat(checkpointAbsDir)
 	if create {
@@ -52,8 +59,32 @@ func getCheckpointDir(checkDir, checkpointID, ctrName, ctrID, ctrCheckpointDir s
 	return checkpointAbsDir, err2
 }
 
+// getCheckpointRoot resolves the checkpoint root used by CheckpointList.
+func getCheckpointRoot(checkDir, ctrName, ctrCheckpointDir string) (string, error) {
+	checkpointDir := checkpointRoot(checkDir, ctrCheckpointDir)
+	// The API intentionally supports caller-selected checkpoint roots.
+	stat, err := os.Stat(checkpointDir)
+	if err != nil {
+		if checkDir != "" && errors.Is(err, os.ErrNotExist) {
+			return checkpointDir, errdefs.NotFound(fmt.Errorf("checkpoint directory %q does not exist for container %s: %w", checkpointDir, ctrName, err))
+		}
+		return checkpointDir, fmt.Errorf("failed to stat checkpoint directory %q for container %s: %w", checkpointDir, ctrName, err)
+	}
+	if !stat.IsDir() {
+		if checkDir != "" {
+			return checkpointDir, errdefs.InvalidParameter(fmt.Errorf("checkpoint directory %q exists but is not a directory", checkpointDir))
+		}
+		return checkpointDir, fmt.Errorf("%s exists and is not a directory", checkpointDir)
+	}
+	return checkpointDir, nil
+}
+
 // CheckpointCreate checkpoints the process running in a container with CRIU
 func (daemon *Daemon) CheckpointCreate(name string, config checkpoint.CreateRequest) error {
+	if err := validateCheckpointID(config.CheckpointID); err != nil {
+		return err
+	}
+
 	container, err := daemon.GetContainer(name)
 	if err != nil {
 		return err
@@ -66,13 +97,9 @@ func (daemon *Daemon) CheckpointCreate(name string, config checkpoint.CreateRequ
 		return err
 	}
 
-	if !validCheckpointNamePattern.MatchString(config.CheckpointID) {
-		return fmt.Errorf("Invalid checkpoint ID (%s), only %s are allowed", config.CheckpointID, validCheckpointNameChars)
-	}
-
 	checkpointDir, err := getCheckpointDir(config.CheckpointDir, config.CheckpointID, name, container.ID, container.CheckpointDir(), true)
 	if err != nil {
-		return fmt.Errorf("cannot checkpoint container %s: %s", name, err)
+		return fmt.Errorf("cannot checkpoint container %s: %w", name, err)
 	}
 
 	err = tsk.CreateCheckpoint(context.Background(), checkpointDir, config.Exit)
@@ -88,6 +115,10 @@ func (daemon *Daemon) CheckpointCreate(name string, config checkpoint.CreateRequ
 
 // CheckpointDelete deletes the specified checkpoint
 func (daemon *Daemon) CheckpointDelete(name string, config backend.CheckpointDeleteOptions) error {
+	if err := validateCheckpointID(config.CheckpointID); err != nil {
+		return err
+	}
+
 	container, err := daemon.GetContainer(name)
 	if err != nil {
 		return err
@@ -108,17 +139,22 @@ func (daemon *Daemon) CheckpointList(name string, config backend.CheckpointListO
 		return nil, err
 	}
 
-	checkpointDir, err := getCheckpointDir(config.CheckpointDir, "", name, container.ID, container.CheckpointDir(), false)
+	checkpointDir, err := getCheckpointRoot(config.CheckpointDir, name, container.CheckpointDir())
 	if err != nil {
-		return nil, err
-	}
-
-	if err := os.MkdirAll(checkpointDir, 0o755); err != nil {
+		if config.CheckpointDir == "" && errors.Is(err, os.ErrNotExist) {
+			return out, nil
+		}
 		return nil, err
 	}
 
 	dirs, err := os.ReadDir(checkpointDir)
 	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			if config.CheckpointDir == "" {
+				return out, nil
+			}
+			return nil, errdefs.NotFound(fmt.Errorf("checkpoint directory %q does not exist for container %s: %w", checkpointDir, name, err))
+		}
 		return nil, err
 	}
 

daemon/checkpoint_test.go

@@ -0,0 +1,106 @@
+package daemon
+
+import (
+	"context"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	cerrdefs "github.com/containerd/errdefs"
+	"github.com/moby/moby/api/types/checkpoint"
+	"github.com/moby/moby/v2/daemon/container"
+	"github.com/moby/moby/v2/daemon/server/backend"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func TestValidateCheckpointID(t *testing.T) {
+	type testCase struct {
+		name         string
+		checkpointID string
+		valid        bool
+	}
+	tests := []testCase{
+		{name: "restricted name", checkpointID: "a1", valid: true},
+		{name: "space", checkpointID: "space name"},
+		{name: "unicode", checkpointID: "unicode-世界"},
+		{name: "dots", checkpointID: "name.with-dots", valid: true},
+		{name: "empty", checkpointID: ""},
+		{name: "dot", checkpointID: "."},
+		{name: "dot-dot", checkpointID: ".."},
+		{name: "traversal", checkpointID: "../outside"},
+		{name: "nested traversal", checkpointID: "foo/../outside"},
+		{name: "separator", checkpointID: "foo/bar"},
+		{name: "windows separator", checkpointID: `foo\bar`},
+		{name: "NUL", checkpointID: "contains\x00nul"},
+		{name: "absolute path", checkpointID: filepath.Join(string(filepath.Separator), "absolute")},
+	}
+	if runtime.GOOS == "windows" {
+		tests = append(tests,
+			testCase{name: "dot trailing space", checkpointID: ". "},
+			testCase{name: "dot-dot trailing space", checkpointID: ".. "},
+			testCase{name: "trailing dot", checkpointID: "name."},
+			testCase{name: "trailing space", checkpointID: "name "},
+		)
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateCheckpointID(tc.checkpointID)
+			if tc.valid {
+				assert.NilError(t, err)
+				return
+			}
+			assert.Check(t, is.ErrorType(err, cerrdefs.IsInvalidArgument))
+		})
+	}
+}
+
+func TestCheckpointOperationsValidateBeforeContainerLookup(t *testing.T) {
+	daemon := &Daemon{}
+	tests := []struct {
+		name      string
+		operation func() error
+	}{
+		{
+			name: "create",
+			operation: func() error {
+				return daemon.CheckpointCreate("missing", checkpoint.CreateRequest{CheckpointID: "../outside"})
+			},
+		},
+		{
+			name: "delete",
+			operation: func() error {
+				return daemon.CheckpointDelete("missing", backend.CheckpointDeleteOptions{CheckpointID: "../outside"})
+			},
+		},
+		{
+			name: "restore",
+			operation: func() error {
+				return daemon.ContainerStart(context.Background(), "missing", "../outside", "")
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.operation()
+			assert.Check(t, is.ErrorType(err, cerrdefs.IsInvalidArgument))
+			assert.Check(t, is.ErrorContains(err, `invalid checkpoint ID "../outside"`))
+		})
+	}
+}
+
+func TestCheckpointListMissingDefaultDirectory(t *testing.T) {
+	t.Parallel()
+
+	root := t.TempDir()
+	ctr := container.NewBaseContainer("container-id", root)
+	store := container.NewMemoryStore()
+	store.Add(ctr.ID, ctr)
+	daemon := &Daemon{containers: store}
+
+	checkpoints, err := daemon.CheckpointList(ctr.ID, backend.CheckpointListOptions{})
+	assert.NilError(t, err)
+	assert.Assert(t, is.Len(checkpoints, 0))
+}

daemon/start.go

@@ -47,6 +47,12 @@ func validateState(ctr *container.Container) error {
 
 // ContainerStart starts a container.
 func (daemon *Daemon) ContainerStart(ctx context.Context, name string, checkpoint string, checkpointDir string) error {
+	if checkpoint != "" {
+		if err := validateCheckpointID(checkpoint); err != nil {
+			return err
+		}
+	}
+
 	daemonCfg := daemon.config()
 	if checkpoint != "" && !daemonCfg.Experimental {
 		return errdefs.InvalidParameter(errors.New("checkpoint is only supported in experimental mode"))
@@ -84,6 +90,11 @@ func (daemon *Daemon) containerStart(ctx context.Context, daemonCfg *configStore
 		span.End()
 	}()
 
+	if checkpointDir != "" {
+		// TODO(mlaventure): how would we support that?
+		return errdefs.Forbidden(errors.New("custom checkpointdir is not supported"))
+	}
+
 	start := time.Now()
 	container.Lock()
 	defer container.Unlock()
@@ -96,11 +107,6 @@ func (daemon *Daemon) containerStart(ctx context.Context, daemonCfg *configStore
 		return errdefs.Conflict(errors.New("container is marked for removal and cannot be started"))
 	}
 
-	if checkpointDir != "" {
-		// TODO(mlaventure): how would we support that?
-		return errdefs.Forbidden(errors.New("custom checkpointdir is not supported"))
-	}
-
 	// if we encounter an error during start we need to ensure that any other
 	// setup has been cleaned up properly
 	defer func() {

integration/container/checkpoint_test.go

@@ -1,11 +1,16 @@
 package container
 
 import (
+	"encoding/json"
+	"net/http"
+	"net/url"
 	"os/exec"
 	"regexp"
 	"sort"
 	"testing"
 
+	"github.com/moby/moby/api/types/common"
+
 	containertypes "github.com/moby/moby/api/types/container"
 	mounttypes "github.com/moby/moby/api/types/mount"
 	"github.com/moby/moby/client"
@@ -17,6 +22,19 @@ import (
 	"gotest.tools/v3/skip"
 )
 
+func TestCheckpointInvalidID(t *testing.T) {
+	ctx := setupTest(t)
+	query := url.Values{"checkpoint": {"../outside"}}
+	res, body, err := request.Post(ctx, "/containers/missing/start?"+query.Encode())
+	assert.NilError(t, err)
+	defer body.Close()
+	assert.Equal(t, res.StatusCode, http.StatusBadRequest)
+
+	var errorResponse common.ErrorResponse
+	assert.NilError(t, json.NewDecoder(body).Decode(&errorResponse))
+	assert.Check(t, is.ErrorContains(errorResponse, `invalid checkpoint ID "../outside"`))
+}
+
 func TestCheckpoint(t *testing.T) {
 	t.Skip("TestCheckpoint is broken; see https://github.com/moby/moby/issues/38963")
 	skip.If(t, testEnv.DaemonInfo.OSType == "windows")