From c610a661e5bc0a9acb4fe163c036809f9884aa3e Mon Sep 17 00:00:00 2001 From: Jeroen van Bemmel Date: Sun, 23 Aug 2026 11:25:27 -0500 Subject: [PATCH 1/2] Fix setuid iptables DOCKER-USER rules on legacy backends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit iptables ≥ 1.8.8 exits 111 when ruid != euid; run those CLIs with matching root UIDs in the child and strip LD_* so setuid clab_admins hosts can install/remove mgmt external-access rules. Co-authored-by: Cursor --- runtime/docker/firewall/iptables/client.go | 9 ++- runtime/docker/firewall/iptables/exec.go | 61 +++++++++++++++++++ runtime/docker/firewall/iptables/exec_test.go | 57 +++++++++++++++++ 3 files changed, 122 insertions(+), 5 deletions(-) create mode 100644 runtime/docker/firewall/iptables/exec.go create mode 100644 runtime/docker/firewall/iptables/exec_test.go diff --git a/runtime/docker/firewall/iptables/client.go b/runtime/docker/firewall/iptables/client.go index 23c3513130..ff368ce3f9 100644 --- a/runtime/docker/firewall/iptables/client.go +++ b/runtime/docker/firewall/iptables/client.go @@ -3,7 +3,6 @@ package iptables import ( "bytes" "fmt" - "os/exec" "strings" "github.com/charmbracelet/log" @@ -101,7 +100,7 @@ func (c *IpTablesClient) InstallForwardingRulesForAF( log.Debugf("Installing iptables (%s) rules for bridge %q", af, iface) - stdOutErr, err := exec.Command(iptCmd, cmd...).CombinedOutput() + stdOutErr, err := newIptablesCmd(iptCmd, cmd...).CombinedOutput() if err != nil { log.Warnf("Iptables install stdout/stderr result is: %s", stdOutErr) return fmt.Errorf("unable to install iptables rule using '%s' command: %w", cmd, err) @@ -137,7 +136,7 @@ func (c *IpTablesClient) DeleteForwardingRulesForAF( iface := rule.Interface // first check if a rule exists before trying to delete it - res, err := exec.Command(iptCmd, strings.Split(iptCheckArgs, " ")...).Output() + res, err := newIptablesCmd(iptCmd, strings.Split(iptCheckArgs, " ")...).Output() if err != nil { // non nil error typically means that DOCKER-USER chain doesn't exist // this happens with old docker installations (centos7 hello) from default repos @@ -170,7 +169,7 @@ func (c *IpTablesClient) DeleteForwardingRulesForAF( log.Debugf("removing clab iptables rules for bridge %q", iface) log.Debugf("trying to delete the forwarding rule with cmd: iptables %s", cmd) - stdOutErr, err := exec.Command(iptCmd, cmd...).CombinedOutput() + stdOutErr, err := newIptablesCmd(iptCmd, cmd...).CombinedOutput() if err != nil { log.Warnf("Iptables delete stdout/stderr result is: %s", stdOutErr) return fmt.Errorf("unable to delete iptables rules: %w", err) @@ -183,7 +182,7 @@ func (c *IpTablesClient) DeleteForwardingRulesForAF( func (c *IpTablesClient) ruleExists(af string, rule *definitions.FirewallRule) bool { iptCmd := iptablesCmd[af] - res, err := exec.Command(iptCmd, strings.Split(iptCheckArgs, " ")...).CombinedOutput() + res, err := newIptablesCmd(iptCmd, strings.Split(iptCheckArgs, " ")...).CombinedOutput() if err != nil { log.Warnf("iptables check error: %s. Output: %s", err, string(res)) // if we errored on check we don't want to try setting up the rule diff --git a/runtime/docker/firewall/iptables/exec.go b/runtime/docker/firewall/iptables/exec.go new file mode 100644 index 0000000000..4cfe2f71f1 --- /dev/null +++ b/runtime/docker/firewall/iptables/exec.go @@ -0,0 +1,61 @@ +package iptables + +import ( + "os" + "os/exec" + "strings" + "syscall" +) + +// newIptablesCmd builds an iptables/ip6tables command that is safe to run from a +// setuid-root containerlab binary (ruid=user, euid=0). +// +// iptables ≥ 1.8.8 exits 111 when getuid() != geteuid() because it loads match/ +// target shared libraries and therefore refuses to run under a setuid parent. +// We isolate the fix to the child process via Credential so the long-lived +// containerlab process keeps its real UID (important for $HOME file ownership). +// LD_* is stripped so a poisoned library path cannot follow into iptables. +func newIptablesCmd(name string, args ...string) *exec.Cmd { + cmd := exec.Command(name, args...) + cmd.Env = sanitizeIptablesEnv(os.Environ()) + + if attr := iptablesSysProcAttr(os.Getuid(), os.Geteuid(), os.Getegid()); attr != nil { + cmd.SysProcAttr = attr + } + + return cmd +} + +// iptablesSysProcAttr returns SysProcAttr that sets the child's real/effective +// UIDs to root when the parent is running setuid-root. Nil when no change is needed. +func iptablesSysProcAttr(ruid, euid, egid int) *syscall.SysProcAttr { + if euid != 0 || ruid == 0 { + return nil + } + + return &syscall.SysProcAttr{ + Credential: &syscall.Credential{ + Uid: 0, + Gid: uint32(egid), + NoSetGroups: true, + }, + } +} + +// sanitizeIptablesEnv drops LD_* (and similar) loader overrides from env. +func sanitizeIptablesEnv(env []string) []string { + out := make([]string, 0, len(env)) + for _, e := range env { + key, _, _ := strings.Cut(e, "=") + if isUnsafeIptablesEnvKey(key) { + continue + } + out = append(out, e) + } + return out +} + +func isUnsafeIptablesEnvKey(key string) bool { + // iptables loads xtables plugins via dlopen; LD_* would defeat the setuid check's intent. + return strings.HasPrefix(key, "LD_") +} diff --git a/runtime/docker/firewall/iptables/exec_test.go b/runtime/docker/firewall/iptables/exec_test.go new file mode 100644 index 0000000000..107e991de1 --- /dev/null +++ b/runtime/docker/firewall/iptables/exec_test.go @@ -0,0 +1,57 @@ +package iptables + +import ( + "slices" + "testing" +) + +func TestSanitizeIptablesEnv(t *testing.T) { + in := []string{ + "PATH=/usr/bin", + "HOME=/home/user", + "LD_PRELOAD=/tmp/evil.so", + "LD_LIBRARY_PATH=/tmp/bad", + "LD_AUDIT=x", + "FOO=bar", + "LD_=shouldstrip", + } + got := sanitizeIptablesEnv(in) + want := []string{ + "PATH=/usr/bin", + "HOME=/home/user", + "FOO=bar", + } + if !slices.Equal(got, want) { + t.Fatalf("sanitizeIptablesEnv() = %#v, want %#v", got, want) + } +} + +func TestIptablesSysProcAttr(t *testing.T) { + t.Run("setuid parent needs matching root uids in child", func(t *testing.T) { + attr := iptablesSysProcAttr(1000, 0, 1000) + if attr == nil || attr.Credential == nil { + t.Fatal("expected Credential for setuid-root parent") + } + if attr.Credential.Uid != 0 { + t.Fatalf("Uid = %d, want 0", attr.Credential.Uid) + } + if attr.Credential.Gid != 1000 { + t.Fatalf("Gid = %d, want 1000 (preserve egid)", attr.Credential.Gid) + } + if !attr.Credential.NoSetGroups { + t.Fatal("expected NoSetGroups to preserve supplementary groups") + } + }) + + t.Run("fully root parent needs no credential", func(t *testing.T) { + if attr := iptablesSysProcAttr(0, 0, 0); attr != nil { + t.Fatalf("expected nil SysProcAttr, got %#v", attr) + } + }) + + t.Run("non-root parent needs no credential", func(t *testing.T) { + if attr := iptablesSysProcAttr(1000, 1000, 1000); attr != nil { + t.Fatalf("expected nil SysProcAttr, got %#v", attr) + } + }) +} From 0a787bfa211e9664e11f6a7ab9c794b387d61a83 Mon Sep 17 00:00:00 2001 From: Jeroen van Bemmel Date: Sun, 23 Aug 2026 19:59:39 -0500 Subject: [PATCH 2/2] Simplify setuid iptables exec: clear env, keep helpers in client.go Use an empty child environment instead of filtering LD_*, and drop the separate exec.go so the iptables spawn path stays in one file. Co-authored-by: Cursor --- runtime/docker/firewall/iptables/client.go | 39 ++++++++++++ .../iptables/{exec_test.go => client_test.go} | 32 +++------- runtime/docker/firewall/iptables/exec.go | 61 ------------------- 3 files changed, 49 insertions(+), 83 deletions(-) rename runtime/docker/firewall/iptables/{exec_test.go => client_test.go} (70%) delete mode 100644 runtime/docker/firewall/iptables/exec.go diff --git a/runtime/docker/firewall/iptables/client.go b/runtime/docker/firewall/iptables/client.go index ff368ce3f9..74c95a831a 100644 --- a/runtime/docker/firewall/iptables/client.go +++ b/runtime/docker/firewall/iptables/client.go @@ -3,7 +3,10 @@ package iptables import ( "bytes" "fmt" + "os" + "os/exec" "strings" + "syscall" "github.com/charmbracelet/log" "github.com/google/shlex" @@ -214,3 +217,39 @@ func (c *IpTablesClient) ruleExists(af string, rule *definitions.FirewallRule) b return false } + +// newIptablesCmd builds an iptables/ip6tables command that is safe to run from a +// setuid-root containerlab binary (ruid=user, euid=0). +// +// iptables ≥ 1.8.8 exits 111 when getuid() != geteuid() because it loads match/ +// target shared libraries and therefore refuses to run under a setuid parent. +// We isolate the fix to the child process via Credential so the long-lived +// containerlab process keeps its real UID (important for $HOME file ownership). +// Env is cleared so a poisoned library path cannot follow into iptables. +func newIptablesCmd(name string, args ...string) *exec.Cmd { + cmd := exec.Command(name, args...) + // nil Env inherits the parent; empty means env -i. + cmd.Env = []string{} + + if attr := iptablesSysProcAttr(os.Getuid(), os.Geteuid(), os.Getegid()); attr != nil { + cmd.SysProcAttr = attr + } + + return cmd +} + +// iptablesSysProcAttr returns SysProcAttr that sets the child's real/effective +// UIDs to root when the parent is running setuid-root. Nil when no change is needed. +func iptablesSysProcAttr(ruid, euid, egid int) *syscall.SysProcAttr { + if euid != 0 || ruid == 0 { + return nil + } + + return &syscall.SysProcAttr{ + Credential: &syscall.Credential{ + Uid: 0, + Gid: uint32(egid), + NoSetGroups: true, + }, + } +} diff --git a/runtime/docker/firewall/iptables/exec_test.go b/runtime/docker/firewall/iptables/client_test.go similarity index 70% rename from runtime/docker/firewall/iptables/exec_test.go rename to runtime/docker/firewall/iptables/client_test.go index 107e991de1..f4802becf3 100644 --- a/runtime/docker/firewall/iptables/exec_test.go +++ b/runtime/docker/firewall/iptables/client_test.go @@ -1,31 +1,9 @@ package iptables import ( - "slices" "testing" ) -func TestSanitizeIptablesEnv(t *testing.T) { - in := []string{ - "PATH=/usr/bin", - "HOME=/home/user", - "LD_PRELOAD=/tmp/evil.so", - "LD_LIBRARY_PATH=/tmp/bad", - "LD_AUDIT=x", - "FOO=bar", - "LD_=shouldstrip", - } - got := sanitizeIptablesEnv(in) - want := []string{ - "PATH=/usr/bin", - "HOME=/home/user", - "FOO=bar", - } - if !slices.Equal(got, want) { - t.Fatalf("sanitizeIptablesEnv() = %#v, want %#v", got, want) - } -} - func TestIptablesSysProcAttr(t *testing.T) { t.Run("setuid parent needs matching root uids in child", func(t *testing.T) { attr := iptablesSysProcAttr(1000, 0, 1000) @@ -55,3 +33,13 @@ func TestIptablesSysProcAttr(t *testing.T) { } }) } + +func TestNewIptablesCmdClearsEnv(t *testing.T) { + cmd := newIptablesCmd("iptables", "-V") + if cmd.Env == nil { + t.Fatal("Env is nil (would inherit parent); want empty slice") + } + if len(cmd.Env) != 0 { + t.Fatalf("Env = %#v, want empty", cmd.Env) + } +} diff --git a/runtime/docker/firewall/iptables/exec.go b/runtime/docker/firewall/iptables/exec.go deleted file mode 100644 index 4cfe2f71f1..0000000000 --- a/runtime/docker/firewall/iptables/exec.go +++ /dev/null @@ -1,61 +0,0 @@ -package iptables - -import ( - "os" - "os/exec" - "strings" - "syscall" -) - -// newIptablesCmd builds an iptables/ip6tables command that is safe to run from a -// setuid-root containerlab binary (ruid=user, euid=0). -// -// iptables ≥ 1.8.8 exits 111 when getuid() != geteuid() because it loads match/ -// target shared libraries and therefore refuses to run under a setuid parent. -// We isolate the fix to the child process via Credential so the long-lived -// containerlab process keeps its real UID (important for $HOME file ownership). -// LD_* is stripped so a poisoned library path cannot follow into iptables. -func newIptablesCmd(name string, args ...string) *exec.Cmd { - cmd := exec.Command(name, args...) - cmd.Env = sanitizeIptablesEnv(os.Environ()) - - if attr := iptablesSysProcAttr(os.Getuid(), os.Geteuid(), os.Getegid()); attr != nil { - cmd.SysProcAttr = attr - } - - return cmd -} - -// iptablesSysProcAttr returns SysProcAttr that sets the child's real/effective -// UIDs to root when the parent is running setuid-root. Nil when no change is needed. -func iptablesSysProcAttr(ruid, euid, egid int) *syscall.SysProcAttr { - if euid != 0 || ruid == 0 { - return nil - } - - return &syscall.SysProcAttr{ - Credential: &syscall.Credential{ - Uid: 0, - Gid: uint32(egid), - NoSetGroups: true, - }, - } -} - -// sanitizeIptablesEnv drops LD_* (and similar) loader overrides from env. -func sanitizeIptablesEnv(env []string) []string { - out := make([]string, 0, len(env)) - for _, e := range env { - key, _, _ := strings.Cut(e, "=") - if isUnsafeIptablesEnvKey(key) { - continue - } - out = append(out, e) - } - return out -} - -func isUnsafeIptablesEnvKey(key string) bool { - // iptables loads xtables plugins via dlopen; LD_* would defeat the setuid check's intent. - return strings.HasPrefix(key, "LD_") -}