Enforce ID validation

agent/service.go

@@ -156,7 +156,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
 
 	err = bundleDir.MountRootfs(drive.Path(), "ext4", nil)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to mount rootfs")
+		return nil, errors.Wrapf(err, "failed to mount rootfs %q", drive.Path())
 	}
 
 	req.Bundle = bundleDir.RootPath()

firecracker-control/local.go

@@ -116,8 +116,14 @@ func (s *local) CreateVM(requestCtx context.Context, req *proto.CreateVMRequest)
 	// If we're here, there is no pre-existing shim for this VMID, so we spawn a new one
 	defer shimSocket.Close()
 
-	shimDir := vm.ShimDir(ns, id)
-	err = shimDir.Create()
+	shimDir, err := vm.ShimDir(ns, id)
+	if err != nil {
+		err = errors.Wrapf(err, "failed to build shim path")
+		s.logger.WithError(err).Error()
+		return nil, err
+	}
+
+	err = shimDir.Mkdir()
 	if err != nil {
 		err = errors.Wrapf(err, "failed to create VM dir %q", shimDir.RootPath())
 		s.logger.WithError(err).Error()
@@ -266,7 +272,14 @@ func (s *local) newShim(ns, vmID, containerdAddress string, shimSocket *net.Unix
 
 	cmd := exec.Command(internal.ShimBinaryName, args...)
 
-	cmd.Dir = vm.ShimDir(ns, vmID).RootPath()
+	shimDir, err := vm.ShimDir(ns, vmID)
+	if err != nil {
+		err = errors.Wrap(err, "failed to create shim dir")
+		logger.WithError(err).Error()
+		return nil, err
+	}
+
+	cmd.Dir = shimDir.RootPath()
 
 	shimSocketFile, err := shimSocket.File()
 	if err != nil {

internal/vm/dir.go

@@ -19,6 +19,7 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/containerd/containerd/identifiers"
 	"github.com/containerd/containerd/runtime/v2/shim"
 	"github.com/containerd/fifo"
 	"github.com/firecracker-microvm/firecracker-containerd/internal"
@@ -33,8 +34,16 @@ const (
 
 // ShimDir holds files, sockets and FIFOs scoped to a single shim managing the
 // VM with the given VMID. It is unique per-VM and containerd namespace.
-func ShimDir(namespace, vmID string) Dir {
-	return Dir(filepath.Join(varRunDir, namespace, vmID))
+func ShimDir(namespace, vmID string) (Dir, error) {
+	if err := identifiers.Validate(namespace); err != nil {
+		return "", errors.Wrap(err, "invalid namespace")
+	}
+
+	if err := identifiers.Validate(vmID); err != nil {
+		return "", errors.Wrap(err, "invalid vm id")
+	}
+
+	return Dir(filepath.Join(varRunDir, namespace, vmID)), nil
 }
 
 // Dir represents the root of a firecracker-containerd VM directory, which
@@ -46,9 +55,9 @@ func (d Dir) RootPath() string {
 	return string(d)
 }
 
-// Create will mkdir the RootPath with correct permissions, or no-op if it
+// Mkdir will mkdir the RootPath with correct permissions, or no-op if it
 // already exists
-func (d Dir) Create() error {
+func (d Dir) Mkdir() error {
 	return os.MkdirAll(d.RootPath(), 0700)
 }
 
@@ -88,26 +97,45 @@ func (d Dir) FirecrackerMetricsFifoPath() string {
 
 // BundleLink returns the path to the symlink to the bundle dir for a given container running
 // inside the VM of this vm dir.
-func (d Dir) BundleLink(containerID string) bundle.Dir {
-	return bundle.Dir(filepath.Join(d.RootPath(), containerID))
+func (d Dir) BundleLink(containerID string) (bundle.Dir, error) {
+	if err := identifiers.Validate(containerID); err != nil {
+		return "", errors.Wrapf(err, "invalid container id %q", containerID)
+	}
+
+	return bundle.Dir(filepath.Join(d.RootPath(), containerID)), nil
 }
 
 // CreateBundleLink creates the BundleLink by symlinking to the provided bundle dir
 func (d Dir) CreateBundleLink(containerID string, bundleDir bundle.Dir) error {
-	return createSymlink(bundleDir.RootPath(), d.BundleLink(containerID).RootPath(), "bundle")
+	path, err := d.BundleLink(containerID)
+	if err != nil {
+		return err
+	}
+
+	return createSymlink(bundleDir.RootPath(), path.RootPath(), "bundle")
 }
 
 // CreateAddressLink creates a symlink from the VM dir to the bundle dir for the shim address file.
 // This symlink is read by containerd.
 // CreateAddressLink assumes that CreateBundleLink has been called.
 func (d Dir) CreateAddressLink(containerID string) error {
-	return createSymlink(d.AddrFilePath(), d.BundleLink(containerID).AddrFilePath(), "shim address file")
+	path, err := d.BundleLink(containerID)
+	if err != nil {
+		return err
+	}
+
+	return createSymlink(d.AddrFilePath(), path.AddrFilePath(), "shim address file")
 }
 
 // CreateShimLogFifoLink creates a symlink from the bundleDir of the provided container.
 // CreateAddressLink assumes that CreateBundleLink has been called.
 func (d Dir) CreateShimLogFifoLink(containerID string) error {
-	return createSymlink(d.BundleLink(containerID).LogFifoPath(), d.LogFifoPath(), "shim log fifo")
+	path, err := d.BundleLink(containerID)
+	if err != nil {
+		return err
+	}
+
+	return createSymlink(path.LogFifoPath(), d.LogFifoPath(), "shim log fifo")
 }
 
 // WriteAddress will write the actual address file in the VM dir.

internal/vm/dir_test.go

@@ -0,0 +1,86 @@
+// Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//	http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package vm
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+var invalidContainerIDs = []string{"", "id?", "*", "id/1", "id\\"}
+
+func TestShimDir(t *testing.T) {
+	tests := []struct {
+		name   string
+		ns     string
+		id     string
+		outDir string
+		outErr string
+	}{
+		{name: "empty ns", outErr: "invalid namespace: identifier must not be empty"},
+		{name: "ns with /", ns: "/", outErr: `invalid namespace: identifier "/" must match`},
+		{name: "ns with ?", ns: "?", outErr: `invalid namespace: identifier "?" must match`},
+		{name: "ns with *", ns: "*", outErr: `invalid namespace: identifier "*" must match`},
+		{name: "ns with ,", ns: ",", outErr: `invalid namespace: identifier "," must match`},
+		{name: "empty id", ns: "test", outErr: "invalid vm id: identifier must not be empty"},
+		{name: "id with /", ns: "test", id: "/", outErr: `invalid vm id: identifier "/" must match`},
+		{name: "id with ?", ns: "test", id: "?", outErr: `invalid vm id: identifier "?" must match`},
+		{name: "id with *", ns: "test", id: "*", outErr: `invalid vm id: identifier "*" must match`},
+		{name: "id with ,", ns: "test", id: ",", outErr: `invalid vm id: identifier "," must match`},
+		{name: "valid", ns: "ns", id: "1", outDir: "/var/run/firecracker-containerd/ns/1"},
+		{name: "valid with dashes", ns: "test-123", id: "123-456", outDir: "/var/run/firecracker-containerd/test-123/123-456"},
+		{name: "valid with dots", ns: "test.123", id: "123.456", outDir: "/var/run/firecracker-containerd/test.123/123.456"},
+	}
+
+	for _, tc := range tests {
+		test := tc
+		t.Run(test.name, func(t *testing.T) {
+			dir, err := ShimDir(test.ns, test.id)
+
+			if test.outErr != "" {
+				assert.Error(t, err)
+				assert.True(t, strings.Contains(err.Error(), test.outErr), err.Error())
+			} else {
+				assert.NoError(t, err)
+				assert.EqualValues(t, dir, test.outDir)
+			}
+		})
+	}
+}
+
+func TestBundleLink(t *testing.T) {
+	var root Dir = "/root/"
+
+	dir, err := root.BundleLink("1")
+	assert.NoError(t, err)
+	assert.EqualValues(t, "/root/1", dir)
+
+	dir, err = root.BundleLink("test-1")
+	assert.NoError(t, err)
+	assert.EqualValues(t, "/root/test-1", dir)
+}
+
+func TestBundleLinkInvalidID(t *testing.T) {
+	var (
+		root Dir = "/root/"
+		err  error
+	)
+
+	for _, id := range invalidContainerIDs {
+		_, err = root.BundleLink(id)
+		assert.Error(t, err)
+	}
+}

runtime/service.go

@@ -113,7 +113,9 @@ type service struct {
 	shimCtx    context.Context
 	shimCancel func()
 
-	vmID   string
+	vmID    string
+	shimDir vm.Dir
+
 	config *Config
 
 	// vmReady is closed once CreateVM has been successfully called
@@ -161,10 +163,16 @@ func NewService(shimCtx context.Context, id string, remotePublisher shim.Publish
 		namespace = namespaces.Default
 	}
 
+	var shimDir vm.Dir
 	vmID := os.Getenv(internal.VMIDEnvVarKey)
 	logger := log.G(shimCtx)
 	if vmID != "" {
 		logger = logger.WithField("vmID", vmID)
+
+		shimDir, err = vm.ShimDir(namespace, vmID)
+		if err != nil {
+			return nil, errors.Wrap(err, "invalid shim directory")
+		}
 	}
 
 	s := &service{
@@ -176,13 +184,15 @@ func NewService(shimCtx context.Context, id string, remotePublisher shim.Publish
 		shimCtx:    shimCtx,
 		shimCancel: shimCancel,
 
-		vmID:   vmID,
+		vmID:    vmID,
+		shimDir: shimDir,
+
 		config: config,
 
 		vmReady: make(chan struct{}),
 	}
 
-	s.stubDriveHandler = newStubDriveHandler(s.shimDir().RootPath(), logger)
+	s.stubDriveHandler = newStubDriveHandler(s.shimDir.RootPath(), logger)
 	s.startEventForwarders(remotePublisher)
 
 	err = s.serveFCControl()
@@ -453,7 +463,7 @@ func (s *service) createVM(requestCtx context.Context, request *proto.CreateVMRe
 
 	cmd := firecracker.VMCommandBuilder{}.
 		WithBin(s.config.FirecrackerBinaryPath).
-		WithSocketPath(s.shimDir().FirecrackerSockPath()).
+		WithSocketPath(s.shimDir.FirecrackerSockPath()).
 		Build(s.shimCtx) // shimCtx so the VM process is only killed when the shim shuts down
 
 	// use shimCtx so the VM is killed when the shim shuts down
@@ -563,18 +573,14 @@ func (s *service) SetVMMetadata(requestCtx context.Context, request *proto.SetVM
 	return &empty.Empty{}, nil
 }
 
-func (s *service) shimDir() vm.Dir {
-	return vm.ShimDir(s.namespace, s.vmID)
-}
-
 func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker.Config, error) {
 	logger := s.logger.WithField("cid", s.machineCID)
 
 	cfg := firecracker.Config{
-		SocketPath:   s.shimDir().FirecrackerSockPath(),
+		SocketPath:   s.shimDir.FirecrackerSockPath(),
 		VsockDevices: []firecracker.VsockDevice{{Path: "root", CID: s.machineCID}},
-		LogFifo:      s.shimDir().FirecrackerLogFifoPath(),
-		MetricsFifo:  s.shimDir().FirecrackerMetricsFifoPath(),
+		LogFifo:      s.shimDir.FirecrackerLogFifoPath(),
+		MetricsFifo:  s.shimDir.FirecrackerMetricsFifoPath(),
 		MachineCfg:   machineConfigurationFromProto(s.config, req.MachineCfg),
 	}
 
@@ -662,14 +668,14 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask
 	}).Debug("creating task")
 
 	bundleDir := bundle.Dir(request.Bundle)
-	err = s.shimDir().CreateBundleLink(request.ID, bundleDir)
+	err = s.shimDir.CreateBundleLink(request.ID, bundleDir)
 	if err != nil {
 		err = errors.Wrap(err, "failed to create VM dir bundle link")
 		logger.WithError(err).Error()
 		return nil, err
 	}
 
-	err = s.shimDir().CreateAddressLink(request.ID)
+	err = s.shimDir.CreateAddressLink(request.ID)
 	if err != nil {
 		err = errors.Wrap(err, "failed to create shim address symlink")
 		logger.WithError(err).Error()
@@ -983,9 +989,9 @@ func (s *service) Shutdown(requestCtx context.Context, req *taskAPI.ShutdownRequ
 		shutdownErr = multierror.Append(shutdownErr, errors.Wrap(err, "failed to gracefully stop VM"))
 	}
 
-	err = os.RemoveAll(s.shimDir().RootPath())
+	err = os.RemoveAll(s.shimDir.RootPath())
 	if err != nil {
-		shutdownErr = multierror.Append(shutdownErr, errors.Wrapf(err, "failed to remove VM dir %q during shutdown", s.shimDir().RootPath()))
+		shutdownErr = multierror.Append(shutdownErr, errors.Wrapf(err, "failed to remove VM dir %q during shutdown", s.shimDir.RootPath()))
 	}
 
 	if shutdownErr != nil {

runtime/service_test.go

@@ -186,10 +186,10 @@ func TestBuildVMConfiguration(t *testing.T) {
 
 			svc.stubDriveHandler = newStubDriveHandler(tempDir, svc.logger)
 			// For values that remain constant between tests, they are written here
-			tc.expectedCfg.SocketPath = svc.shimDir().FirecrackerSockPath()
+			tc.expectedCfg.SocketPath = svc.shimDir.FirecrackerSockPath()
 			tc.expectedCfg.VsockDevices = []firecracker.VsockDevice{{Path: "root", CID: svc.machineCID}}
-			tc.expectedCfg.LogFifo = svc.shimDir().FirecrackerLogFifoPath()
-			tc.expectedCfg.MetricsFifo = svc.shimDir().FirecrackerMetricsFifoPath()
+			tc.expectedCfg.LogFifo = svc.shimDir.FirecrackerLogFifoPath()
+			tc.expectedCfg.MetricsFifo = svc.shimDir.FirecrackerMetricsFifoPath()
 			tc.expectedCfg.Drives[0].PathOnHost = firecracker.String(filepath.Join(tempDir, "stub0"))
 
 			actualCfg, err := svc.buildVMConfiguration(tc.request)