Merge pull request #53377 from vvoland/checkpoint-id-sanitize

daemon: Prevent checkpoint ID path traversal

api/docs/v1.24.md

@@ -1095,6 +1095,7 @@ Start the container `id`
 
 -   **204** – no error
 -   **304** – container already started
+-   **400** – bad parameter, including an invalid checkpoint ID
 -   **404** – no such container
 -   **500** – server error
 

api/docs/v1.25.yaml

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

api/docs/v1.26.yaml

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

api/docs/v1.27.yaml

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

api/docs/v1.28.yaml

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

api/docs/v1.29.yaml

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

api/docs/v1.30.yaml

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

api/docs/v1.31.yaml

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

api/docs/v1.32.yaml

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

api/docs/v1.33.yaml

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

api/docs/v1.34.yaml

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

api/docs/v1.35.yaml

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

api/docs/v1.36.yaml

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

api/docs/v1.37.yaml

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

api/docs/v1.38.yaml

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

api/docs/v1.39.yaml

@@ -6737,6 +6737,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:

api/docs/v1.40.yaml

@@ -7052,6 +7052,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:

api/docs/v1.41.yaml

@@ -7341,6 +7341,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:

api/docs/v1.42.yaml

@@ -7556,6 +7556,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:

api/docs/v1.43.yaml

@@ -7574,6 +7574,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:

api/docs/v1.44.yaml

@@ -7714,6 +7714,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:

api/docs/v1.45.yaml

@@ -7700,6 +7700,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:

api/docs/v1.46.yaml

@@ -7821,6 +7821,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:

api/docs/v1.47.yaml

@@ -7957,6 +7957,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:

api/docs/v1.48.yaml

@@ -8513,6 +8513,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:

api/docs/v1.49.yaml

@@ -8513,6 +8513,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:

api/docs/v1.50.yaml

@@ -8343,6 +8343,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:

api/docs/v1.51.yaml

@@ -8364,6 +8364,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:

api/docs/v1.52.yaml

@@ -8653,6 +8653,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:

api/docs/v1.53.yaml

@@ -8901,6 +8901,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:

api/docs/v1.54.yaml

@@ -8908,6 +8908,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:

api/docs/v1.55.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:

api/swagger.yaml

@@ -8938,6 +8938,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")