diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index 9cef20c07c..5062486e9c 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -80,6 +80,23 @@ pub struct AgentService { sandbox: Arc>, } +// A container ID must match this regex: +// +// ^[a-zA-Z0-9][a-zA-Z0-9_.-]+$ +// +fn verify_cid(id: &str) -> Result<()> { + let valid = id.len() > 1 + && id.chars().next().unwrap().is_alphanumeric() + && id + .chars() + .all(|c| (c.is_alphanumeric() || ['.', '-', '_'].contains(&c))); + + match valid { + true => Ok(()), + false => Err(anyhow!("invalid container ID: {:?}", id)), + } +} + impl AgentService { async fn do_create_container( &self, @@ -87,6 +104,8 @@ impl AgentService { ) -> Result<()> { let cid = req.container_id.clone(); + let _ = verify_cid(&cid)?; + let mut oci_spec = req.OCI.clone(); let use_sandbox_pidns = req.get_sandbox_pidns(); @@ -1778,4 +1797,231 @@ mod tests { assert!(result.is_err(), "expected add arp neighbors to fail"); } + + #[tokio::test] + async fn test_verify_cid() { + #[derive(Debug)] + struct TestData<'a> { + id: &'a str, + expect_error: bool, + } + + let tests = &[ + TestData { + // Cannot be blank + id: "", + expect_error: true, + }, + TestData { + // Cannot be a space + id: " ", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: ".", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: "-", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: "_", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: " a", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: ".a", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: "-a", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: "_a", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: "..", + expect_error: true, + }, + TestData { + // Too short + id: "a", + expect_error: true, + }, + TestData { + // Too short + id: "z", + expect_error: true, + }, + TestData { + // Too short + id: "A", + expect_error: true, + }, + TestData { + // Too short + id: "Z", + expect_error: true, + }, + TestData { + // Too short + id: "0", + expect_error: true, + }, + TestData { + // Too short + id: "9", + expect_error: true, + }, + TestData { + // Must start with an alphanumeric + id: "-1", + expect_error: true, + }, + TestData { + id: "/", + expect_error: true, + }, + TestData { + id: "a/", + expect_error: true, + }, + TestData { + id: "a/../", + expect_error: true, + }, + TestData { + id: "../a", + expect_error: true, + }, + TestData { + id: "../../a", + expect_error: true, + }, + TestData { + id: "../../../a", + expect_error: true, + }, + TestData { + id: "foo/../bar", + expect_error: true, + }, + TestData { + id: "foo bar", + expect_error: true, + }, + TestData { + id: "a.", + expect_error: false, + }, + TestData { + id: "a..", + expect_error: false, + }, + TestData { + id: "aa", + expect_error: false, + }, + TestData { + id: "aa.", + expect_error: false, + }, + TestData { + id: "hello..world", + expect_error: false, + }, + TestData { + id: "hello/../world", + expect_error: true, + }, + TestData { + id: "aa1245124sadfasdfgasdga.", + expect_error: false, + }, + TestData { + id: "aAzZ0123456789_.-", + expect_error: false, + }, + TestData { + id: "abcdefghijklmnopqrstuvwxyz0123456789.-_", + expect_error: false, + }, + TestData { + id: "0123456789abcdefghijklmnopqrstuvwxyz.-_", + expect_error: false, + }, + TestData { + id: " abcdefghijklmnopqrstuvwxyz0123456789.-_", + expect_error: true, + }, + TestData { + id: ".abcdefghijklmnopqrstuvwxyz0123456789.-_", + expect_error: true, + }, + TestData { + id: "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_", + expect_error: false, + }, + TestData { + id: "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ.-_", + expect_error: false, + }, + TestData { + id: " ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_", + expect_error: true, + }, + TestData { + id: ".ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_", + expect_error: true, + }, + TestData { + id: "/a/b/c", + expect_error: true, + }, + TestData { + id: "a/b/c", + expect_error: true, + }, + TestData { + id: "foo/../../../etc/passwd", + expect_error: true, + }, + TestData { + id: "../../../../../../etc/motd", + expect_error: true, + }, + TestData { + id: "/etc/passwd", + expect_error: true, + }, + ]; + + for (i, d) in tests.iter().enumerate() { + let msg = format!("test[{}]: {:?}", i, d); + + let result = verify_cid(d.id); + + let msg = format!("{}, result: {:?}", msg, result); + + if result.is_ok() { + assert!(!d.expect_error, msg); + } else { + assert!(d.expect_error, msg); + } + } + } } diff --git a/src/runtime/cli/kata-exec.go b/src/runtime/cli/kata-exec.go index 83a314aa72..6877071589 100644 --- a/src/runtime/cli/kata-exec.go +++ b/src/runtime/cli/kata-exec.go @@ -87,9 +87,11 @@ var kataExecCLICommand = cli.Command{ span.SetAttributes(label.Key("port").Uint64(port)) sandboxID := context.Args().Get(0) - if sandboxID == "" { - return fmt.Errorf("SandboxID not found") + + if err := katautils.VerifyContainerID(sandboxID); err != nil { + return err } + span.SetAttributes(label.Key("sandbox").String(sandboxID)) conn, err := getConn(namespace, sandboxID, port) diff --git a/src/runtime/containerd-shim-v2/service.go b/src/runtime/containerd-shim-v2/service.go index bcecebe3a2..329c1982b0 100644 --- a/src/runtime/containerd-shim-v2/service.go +++ b/src/runtime/containerd-shim-v2/service.go @@ -368,6 +368,10 @@ func (s *service) Create(ctx context.Context, r *taskAPI.CreateTaskRequest) (_ * s.mu.Lock() defer s.mu.Unlock() + if err := katautils.VerifyContainerID(r.ID); err != nil { + return nil, err + } + type Result struct { container *container err error diff --git a/src/runtime/containerd-shim-v2/service_test.go b/src/runtime/containerd-shim-v2/service_test.go new file mode 100644 index 0000000000..843df7b8d0 --- /dev/null +++ b/src/runtime/containerd-shim-v2/service_test.go @@ -0,0 +1,80 @@ +// Copyright (c) 2021 Intel Corporation +// +// SPDX-License-Identifier: Apache-2.0 +// + +package containerdshim + +import ( + "context" + "fmt" + "io/ioutil" + "os" + "path/filepath" + "strings" + "testing" + + taskAPI "github.com/containerd/containerd/runtime/v2/task" + ktu "github.com/kata-containers/kata-containers/src/runtime/pkg/katatestutils" + "github.com/stretchr/testify/assert" +) + +func NewService(id string) (service, error) { + ctx := context.Background() + + ctx, cancel := context.WithCancel(ctx) + + s := service{ + id: id, + pid: uint32(os.Getpid()), + ctx: ctx, + containers: make(map[string]*container), + events: make(chan interface{}, chSize), + ec: make(chan exit, bufferSize), + cancel: cancel, + } + + return s, nil +} + +func TestServiceCreate(t *testing.T) { + const badCIDErrorPrefix = "invalid container/sandbox ID" + const blankCIDError = "ID cannot be blank" + + assert := assert.New(t) + + tmpdir, err := ioutil.TempDir("", "") + defer os.RemoveAll(tmpdir) + + bundleDir := filepath.Join(tmpdir, "bundle") + err = makeOCIBundle(bundleDir) + assert.NoError(err) + + ctx := context.Background() + + s, err := NewService("foo") + assert.NoError(err) + + for i, d := range ktu.ContainerIDTestData { + msg := fmt.Sprintf("test[%d]: %+v", i, d) + + // Only consider error scenarios as we are only testing invalid CIDs here. + if d.Valid { + continue + } + + task := taskAPI.CreateTaskRequest{ + ID: d.ID, + Bundle: bundleDir, + } + + _, err = s.Create(ctx, &task) + assert.Error(err, msg) + + if d.ID == "" { + assert.Equal(err.Error(), blankCIDError, msg) + } else { + assert.True(strings.HasPrefix(err.Error(), badCIDErrorPrefix), msg) + } + } +} diff --git a/src/runtime/pkg/katatestutils/utils.go b/src/runtime/pkg/katatestutils/utils.go index f328b19f64..ad0c99916b 100644 --- a/src/runtime/pkg/katatestutils/utils.go +++ b/src/runtime/pkg/katatestutils/utils.go @@ -47,6 +47,62 @@ type RuntimeConfigOptions struct { JaegerPassword string } +// ContainerIDTestDataType is a type used to test Container and Sandbox ID's. +type ContainerIDTestDataType struct { + ID string + Valid bool +} + +// Set of test data that lists valid and invalid Container IDs +var ContainerIDTestData = []ContainerIDTestDataType{ + {"", false}, // Cannot be blank + {" ", false}, // Cannot be a space + {".", false}, // Must start with an alphanumeric + {"-", false}, // Must start with an alphanumeric + {"_", false}, // Must start with an alphanumeric + {" a", false}, // Must start with an alphanumeric + {".a", false}, // Must start with an alphanumeric + {"-a", false}, // Must start with an alphanumeric + {"_a", false}, // Must start with an alphanumeric + {"..", false}, // Must start with an alphanumeric + {"a", false}, // Too short + {"z", false}, // Too short + {"A", false}, // Too short + {"Z", false}, // Too short + {"0", false}, // Too short + {"9", false}, // Too short + {"-1", false}, // Must start with an alphanumeric + {"/", false}, + {"a/", false}, + {"a/../", false}, + {"../a", false}, + {"../../a", false}, + {"../../../a", false}, + {"foo/../bar", false}, + {"foo bar", false}, + {"a.", true}, + {"a..", true}, + {"aa", true}, + {"aa.", true}, + {"hello..world", true}, + {"hello/../world", false}, + {"aa1245124sadfasdfgasdga.", true}, + {"aAzZ0123456789_.-", true}, + {"abcdefghijklmnopqrstuvwxyz0123456789.-_", true}, + {"0123456789abcdefghijklmnopqrstuvwxyz.-_", true}, + {" abcdefghijklmnopqrstuvwxyz0123456789.-_", false}, + {".abcdefghijklmnopqrstuvwxyz0123456789.-_", false}, + {"ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_", true}, + {"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ.-_", true}, + {" ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_", false}, + {".ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_", false}, + {"/a/b/c", false}, + {"a/b/c", false}, + {"foo/../../../etc/passwd", false}, + {"../../../../../../etc/motd", false}, + {"/etc/passwd", false}, +} + func MakeRuntimeConfigFileData(config RuntimeConfigOptions) string { return ` # Runtime configuration file diff --git a/src/runtime/pkg/katautils/utils.go b/src/runtime/pkg/katautils/utils.go index 02c2ea568f..454bb5cf6d 100644 --- a/src/runtime/pkg/katautils/utils.go +++ b/src/runtime/pkg/katautils/utils.go @@ -7,15 +7,21 @@ package katautils import ( + "errors" "fmt" "io/ioutil" "os" "path/filepath" + "regexp" "syscall" "golang.org/x/sys/unix" ) +// validCIDRegex is a regular expression used to determine +// if a container ID (or sandbox ID) is valid. +const validCIDRegex = `^[a-zA-Z0-9][a-zA-Z0-9_.-]+$` + // FileExists test is a file exiting or not func FileExists(path string) bool { if _, err := os.Stat(path); os.IsNotExist(err) { @@ -109,3 +115,22 @@ func GetFileContents(file string) (string, error) { return string(bytes), nil } + +// VerifyContainerID checks if the specified container ID +// (or sandbox ID) is valid. +func VerifyContainerID(id string) error { + if id == "" { + return errors.New("ID cannot be blank") + } + + // Note: no length check. + validPattern := regexp.MustCompile(validCIDRegex) + + matches := validPattern.MatchString(id) + + if !matches { + return fmt.Errorf("invalid container/sandbox ID (should match %q)", validCIDRegex) + } + + return nil +} diff --git a/src/runtime/pkg/katautils/utils_test.go b/src/runtime/pkg/katautils/utils_test.go index 984efc3483..0beedc2ff9 100644 --- a/src/runtime/pkg/katautils/utils_test.go +++ b/src/runtime/pkg/katautils/utils_test.go @@ -18,6 +18,7 @@ import ( "syscall" "testing" + ktu "github.com/kata-containers/kata-containers/src/runtime/pkg/katatestutils" "github.com/kata-containers/kata-containers/src/runtime/pkg/utils" "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/compatoci" "github.com/stretchr/testify/assert" @@ -366,3 +367,18 @@ func TestGetFileContents(t *testing.T) { assert.Equal(t, contents, d.contents) } } + +func TestVerifyContainerID(t *testing.T) { + assert := assert.New(t) + + for i, d := range ktu.ContainerIDTestData { + msg := fmt.Sprintf("test[%d]: %+v", i, d) + + err := VerifyContainerID(d.ID) + if d.Valid { + assert.NoError(err, msg) + } else { + assert.Error(err, msg) + } + } +} diff --git a/src/runtime/virtcontainers/pkg/vcmock/sandbox.go b/src/runtime/virtcontainers/pkg/vcmock/sandbox.go index ae67558b60..168bb51e33 100644 --- a/src/runtime/virtcontainers/pkg/vcmock/sandbox.go +++ b/src/runtime/virtcontainers/pkg/vcmock/sandbox.go @@ -255,3 +255,7 @@ func (s *Sandbox) GetAgentURL() (string, error) { } return "", nil } + +func (s *Sandbox) GetHypervisorPid() (int, error) { + return 0, nil +}