From c445eea7749d8081d258b8b3b54474da67d348a7 Mon Sep 17 00:00:00 2001 From: llink5 Date: Tue, 31 Mar 2026 12:38:33 +0000 Subject: [PATCH] runtime: harden Docker 26+ networking fix - Replace sandbox ID denylist with positive regex (^[0-9a-f]{64}$) - Rollback partially-added endpoints on scan failure in scanEndpointsInNs Signed-off-by: llink5 --- src/runtime/virtcontainers/network_linux.go | 40 +++++++++--- .../virtcontainers/network_linux_test.go | 10 +-- src/runtime/virtcontainers/sandbox.go | 15 +++-- src/runtime/virtcontainers/utils/utils.go | 22 ++++--- .../virtcontainers/utils/utils_test.go | 64 +++++++++++++++---- 5 files changed, 111 insertions(+), 40 deletions(-) diff --git a/src/runtime/virtcontainers/network_linux.go b/src/runtime/virtcontainers/network_linux.go index f3a74d9885..93dafc28fb 100644 --- a/src/runtime/virtcontainers/network_linux.go +++ b/src/runtime/virtcontainers/network_linux.go @@ -46,6 +46,11 @@ type LinuxNetwork struct { interworkingModel NetInterworkingModel netNSCreated bool danConfigPath string + // placeholderNetNS holds the path to a placeholder network namespace + // that we created but later abandoned in favour of the hypervisor's + // netns. If best-effort deletion in addAllEndpoints fails, teardown + // retries the cleanup via RemoveEndpoints. + placeholderNetNS string } // NewNetwork creates a new Linux Network from a NetworkConfig. @@ -69,11 +74,11 @@ func NewNetwork(configs ...*NetworkConfig) (Network, error) { } return &LinuxNetwork{ - config.NetworkID, - []Endpoint{}, - config.InterworkingModel, - config.NetworkCreated, - config.DanConfigPath, + netNSPath: config.NetworkID, + eps: []Endpoint{}, + interworkingModel: config.InterworkingModel, + netNSCreated: config.NetworkCreated, + danConfigPath: config.DanConfigPath, }, nil } @@ -350,8 +355,6 @@ func (n *LinuxNetwork) addAllEndpoints(ctx context.Context, s *Sandbox, hotplug origPath := n.netNSPath origCreated := n.netNSCreated n.netNSPath = hypervisorNs - // The hypervisor's namespace was not created by us. - n.netNSCreated = false _, err = n.scanEndpointsInNs(ctx, s, n.netNSPath, hotplug) if err != nil { @@ -362,11 +365,16 @@ func (n *LinuxNetwork) addAllEndpoints(ctx context.Context, s *Sandbox, hotplug // Clean up the placeholder namespace we created — we're now // using the hypervisor's namespace and the placeholder is empty. + // Only clear netNSCreated once deletion succeeds; on failure, + // stash the path so RemoveEndpoints can retry during teardown. if origCreated { if delErr := deleteNetNS(origPath); delErr != nil { - networkLogger().WithField("netns", origPath).WithError(delErr).Warn("failed to delete placeholder netns") + networkLogger().WithField("netns", origPath).WithError(delErr).Warn("failed to delete placeholder netns, will retry during teardown") + n.placeholderNetNS = origPath } } + // The hypervisor's namespace was not created by us. + n.netNSCreated = false } } @@ -400,7 +408,9 @@ func (n *LinuxNetwork) scanEndpointsInNs(ctx context.Context, s *Sandbox, nsPath return nil, err } + epsBefore := len(n.eps) var added []Endpoint + for _, link := range linkList { netInfo, err := networkInfoFromLink(netlinkHandle, link) if err != nil { @@ -433,6 +443,9 @@ func (n *LinuxNetwork) scanEndpointsInNs(ctx context.Context, s *Sandbox, nsPath } return addErr }); err != nil { + // Rollback: remove any endpoints added during this scan + // so that a failed scan does not leave partial side effects. + n.eps = n.eps[:epsBefore] return nil, err } } @@ -666,6 +679,17 @@ func (n *LinuxNetwork) RemoveEndpoints(ctx context.Context, s *Sandbox, endpoint return deleteNetNS(n.netNSPath) } + // Retry cleanup of a placeholder namespace whose earlier deletion + // failed in addAllEndpoints. + if n.placeholderNetNS != "" && endpoints == nil { + if delErr := deleteNetNS(n.placeholderNetNS); delErr != nil { + networkLogger().WithField("netns", n.placeholderNetNS).WithError(delErr).Warn("failed to delete placeholder netns during teardown") + } else { + networkLogger().Infof("Placeholder network namespace %q deleted", n.placeholderNetNS) + n.placeholderNetNS = "" + } + } + return nil } diff --git a/src/runtime/virtcontainers/network_linux_test.go b/src/runtime/virtcontainers/network_linux_test.go index 4ab36e04e4..3e93c2356c 100644 --- a/src/runtime/virtcontainers/network_linux_test.go +++ b/src/runtime/virtcontainers/network_linux_test.go @@ -363,11 +363,11 @@ func TestConvertDanDeviceToNetworkInfo(t *testing.T) { func TestAddEndpoints_Dan(t *testing.T) { network := &LinuxNetwork{ - "net-123", - []Endpoint{}, - NetXConnectDefaultModel, - true, - "testdata/dan-config.json", + netNSPath: "net-123", + eps: []Endpoint{}, + interworkingModel: NetXConnectDefaultModel, + netNSCreated: true, + danConfigPath: "testdata/dan-config.json", } ctx := context.TODO() diff --git a/src/runtime/virtcontainers/sandbox.go b/src/runtime/virtcontainers/sandbox.go index f4aabda4ac..21fff0c720 100644 --- a/src/runtime/virtcontainers/sandbox.go +++ b/src/runtime/virtcontainers/sandbox.go @@ -352,11 +352,14 @@ func (s *Sandbox) RescanNetwork(ctx context.Context) error { const maxWait = 5 * time.Second const pollInterval = 50 * time.Millisecond - deadline := time.Now().Add(maxWait) + deadline := time.NewTimer(maxWait) + defer deadline.Stop() + ticker := time.NewTicker(pollInterval) + defer ticker.Stop() s.Logger().Info("waiting for network interfaces in namespace") - for time.Now().Before(deadline) { + for { if _, err := s.network.AddEndpoints(ctx, s, nil, true); err != nil { return err } @@ -367,12 +370,12 @@ func (s *Sandbox) RescanNetwork(ctx context.Context) error { select { case <-ctx.Done(): return ctx.Err() - case <-time.After(pollInterval): + case <-deadline.C: + s.Logger().Warn("no network interfaces found after timeout") + return nil + case <-ticker.C: } } - - s.Logger().Warn("no network interfaces found after timeout") - return nil } // configureGuestNetwork informs the guest agent about discovered network diff --git a/src/runtime/virtcontainers/utils/utils.go b/src/runtime/virtcontainers/utils/utils.go index 0e57339594..e2cb4b3baf 100644 --- a/src/runtime/virtcontainers/utils/utils.go +++ b/src/runtime/virtcontainers/utils/utils.go @@ -12,6 +12,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strings" "syscall" "time" @@ -503,6 +504,9 @@ const ( // daemon bind-mounts container network namespaces. var dockerNetnsPrefixes = []string{"/var/run/docker/netns/", "/run/docker/netns/"} +// validSandboxID matches Docker sandbox IDs: exactly 64 lowercase hex characters. +var validSandboxID = regexp.MustCompile(`^[0-9a-f]{64}$`) + // IsDockerContainer returns if the container is managed by docker // This is done by checking the prestart and createRuntime hooks for // `libnetwork` arguments. Docker 26+ may use CreateRuntime hooks @@ -551,21 +555,19 @@ func DockerNetnsPath(spec *specs.Spec) string { for i, arg := range hook.Args { if arg == dockerLibnetworkSetkey && i+1 < len(hook.Args) { sandboxID := hook.Args[i+1] - // Validate sandbox ID to prevent path traversal. - // Docker sandbox IDs are always hex strings. - if sandboxID == "" || - strings.Contains(sandboxID, "/") || - strings.Contains(sandboxID, "\\") || - strings.Contains(sandboxID, "..") || - strings.ContainsRune(sandboxID, 0) { + // Docker sandbox IDs are exactly 64 lowercase hex + // characters. Reject anything else to prevent path + // traversal and unexpected input. + if !validSandboxID.MatchString(sandboxID) { continue } // Docker stores netns under well-known paths. - // Use Lstat to reject symlinks that could point - // outside the Docker netns directory. + // Use Lstat to reject symlinks (which could point + // outside the Docker netns directory) and non-regular + // files such as directories. for _, prefix := range dockerNetnsPrefixes { nsPath := prefix + sandboxID - if fi, err := os.Lstat(nsPath); err == nil && fi.Mode().Type()&os.ModeSymlink == 0 { + if fi, err := os.Lstat(nsPath); err == nil && fi.Mode().IsRegular() { return nsPath } } diff --git a/src/runtime/virtcontainers/utils/utils_test.go b/src/runtime/virtcontainers/utils/utils_test.go index 44463c009c..1d1c85f113 100644 --- a/src/runtime/virtcontainers/utils/utils_test.go +++ b/src/runtime/virtcontainers/utils/utils_test.go @@ -628,6 +628,12 @@ func TestIsDockerContainer(t *testing.T) { func TestDockerNetnsPath(t *testing.T) { assert := assert.New(t) + // Valid 64-char hex sandbox IDs for test cases. + validID := strings.Repeat("ab", 32) // 64 hex chars + validID2 := strings.Repeat("cd", 32) // another 64 hex chars + invalidShortID := "abc123" // too short + invalidUpperID := strings.Repeat("AB", 32) // uppercase rejected + // nil spec assert.Equal("", DockerNetnsPath(nil)) @@ -644,43 +650,79 @@ func TestDockerNetnsPath(t *testing.T) { } assert.Equal("", DockerNetnsPath(spec)) - // Prestart hook with libnetwork-setkey but netns file doesn't exist + // Prestart hook with libnetwork-setkey but sandbox ID too short (rejected by regex) spec = &specs.Spec{ Hooks: &specs.Hooks{ Prestart: []specs.Hook{ //nolint:all - {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", "nonexistent999", "ctrl"}}, + {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", invalidShortID, "ctrl"}}, }, }, } assert.Equal("", DockerNetnsPath(spec)) - // Prestart hook with libnetwork-setkey and existing netns file + // Prestart hook with libnetwork-setkey but uppercase hex (rejected by regex) + spec = &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ //nolint:all + {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", invalidUpperID, "ctrl"}}, + }, + }, + } + assert.Equal("", DockerNetnsPath(spec)) + + // Prestart hook with valid sandbox ID but netns file doesn't exist on disk + spec = &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ //nolint:all + {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", validID, "ctrl"}}, + }, + }, + } + assert.Equal("", DockerNetnsPath(spec)) + + // Prestart hook with libnetwork-setkey and existing netns file — success path tmpDir := t.TempDir() - // Simulate /var/run/docker/netns/ by creating the file fakeNsDir := filepath.Join(tmpDir, "netns") err := os.MkdirAll(fakeNsDir, 0755) assert.NoError(err) - fakeNsFile := filepath.Join(fakeNsDir, "abc123def456") + fakeNsFile := filepath.Join(fakeNsDir, validID) err = os.WriteFile(fakeNsFile, []byte{}, 0644) assert.NoError(err) - // DockerNetnsPath checks hardcoded paths (/var/run/docker/netns/, /run/docker/netns/) - // so with a temp dir it won't find it. Verify that non-existent paths return "". + // Temporarily override dockerNetnsPrefixes so DockerNetnsPath can find + // the netns file we created under the temp directory. + origPrefixes := dockerNetnsPrefixes + dockerNetnsPrefixes = []string{fakeNsDir + "/"} + defer func() { dockerNetnsPrefixes = origPrefixes }() + spec = &specs.Spec{ Hooks: &specs.Hooks{ Prestart: []specs.Hook{ //nolint:all - {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", "abc123def456", "ctrl"}}, + {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", validID, "ctrl"}}, + }, + }, + } + assert.Equal(fakeNsFile, DockerNetnsPath(spec)) + + // Sandbox ID that is a directory rather than a regular file — must be rejected + dirID := validID2 + err = os.MkdirAll(filepath.Join(fakeNsDir, dirID), 0755) + assert.NoError(err) + spec = &specs.Spec{ + Hooks: &specs.Hooks{ + Prestart: []specs.Hook{ //nolint:all + {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", dirID, "ctrl"}}, }, }, } - // The sandbox ID "abc123def456" won't exist under /var/run/docker/netns/ assert.Equal("", DockerNetnsPath(spec)) - // CreateRuntime hook with libnetwork-setkey — same behavior, file doesn't exist + // CreateRuntime hook with valid sandbox ID — file doesn't exist + validID3 := strings.Repeat("ef", 32) spec = &specs.Spec{ Hooks: &specs.Hooks{ CreateRuntime: []specs.Hook{ - {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", "xyz789", "ctrl"}}, + {Args: []string{"/usr/bin/proxy", "libnetwork-setkey", validID3, "ctrl"}}, }, }, }