From 021de2e1bc8d5d4ab66d4f4f5c560dc585ae3ae0 Mon Sep 17 00:00:00 2001 From: Mike O'Driscoll Date: Tue, 10 Mar 2026 15:19:15 -0400 Subject: [PATCH] util/linuxfw: fix nil pointer panic in connmark rules without IPv6 (#18946) When IPv6 is unavailable on a system, AddConnmarkSaveRule() and DelConnmarkSaveRule() would panic with a nil pointer dereference. Both methods directly iterated over []iptablesInterface{i.ipt4, i.ipt6} without checking if ipt6 was nil. Use `getTables()` instead to properly retrieve the available tables on a given system Fixes #3310 Signed-off-by: Mike O'Driscoll --- util/linuxfw/fake.go | 18 ++-- util/linuxfw/iptables_runner.go | 8 +- util/linuxfw/iptables_runner_test.go | 140 +++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 12 deletions(-) diff --git a/util/linuxfw/fake.go b/util/linuxfw/fake.go index 166d80401..b902b93c1 100644 --- a/util/linuxfw/fake.go +++ b/util/linuxfw/fake.go @@ -26,13 +26,15 @@ type fakeRule struct { func newFakeIPTables() *fakeIPTables { return &fakeIPTables{ n: map[string][]string{ - "filter/INPUT": nil, - "filter/OUTPUT": nil, - "filter/FORWARD": nil, - "nat/PREROUTING": nil, - "nat/OUTPUT": nil, - "nat/POSTROUTING": nil, - "mangle/FORWARD": nil, + "filter/INPUT": nil, + "filter/OUTPUT": nil, + "filter/FORWARD": nil, + "nat/PREROUTING": nil, + "nat/OUTPUT": nil, + "nat/POSTROUTING": nil, + "mangle/FORWARD": nil, + "mangle/PREROUTING": nil, + "mangle/OUTPUT": nil, }, } } @@ -80,7 +82,7 @@ func (n *fakeIPTables) Delete(table, chain string, args ...string) error { return nil } } - return fmt.Errorf("delete of unknown rule %q from %s", strings.Join(args, " "), k) + return errors.New("exitcode:1") } else { return fmt.Errorf("unknown table/chain %s", k) } diff --git a/util/linuxfw/iptables_runner.go b/util/linuxfw/iptables_runner.go index b8eb39f21..0d50bdd61 100644 --- a/util/linuxfw/iptables_runner.go +++ b/util/linuxfw/iptables_runner.go @@ -533,7 +533,7 @@ func (i *iptablesRunner) DelStatefulRule(tunname string) error { // proper routing table lookups for exit nodes and subnet routers. func (i *iptablesRunner) AddConnmarkSaveRule() error { // Check if rules already exist (idempotency) - for _, ipt := range []iptablesInterface{i.ipt4, i.ipt6} { + for _, ipt := range i.getTables() { rules, err := ipt.List("mangle", "PREROUTING") if err != nil { continue @@ -551,7 +551,7 @@ func (i *iptablesRunner) AddConnmarkSaveRule() error { // mangle/PREROUTING: Restore mark from conntrack for ESTABLISHED/RELATED connections // This runs BEFORE routing decision and rp_filter check - for _, ipt := range []iptablesInterface{i.ipt4, i.ipt6} { + for _, ipt := range i.getTables() { args := []string{ "-m", "conntrack", "--ctstate", "ESTABLISHED,RELATED", @@ -566,7 +566,7 @@ func (i *iptablesRunner) AddConnmarkSaveRule() error { } // mangle/OUTPUT: Save mark to conntrack for NEW connections with non-zero marks - for _, ipt := range []iptablesInterface{i.ipt4, i.ipt6} { + for _, ipt := range i.getTables() { args := []string{ "-m", "conntrack", "--ctstate", "NEW", @@ -587,7 +587,7 @@ func (i *iptablesRunner) AddConnmarkSaveRule() error { // DelConnmarkSaveRule removes conntrack marking rules added by AddConnmarkSaveRule. func (i *iptablesRunner) DelConnmarkSaveRule() error { - for _, ipt := range []iptablesInterface{i.ipt4, i.ipt6} { + for _, ipt := range i.getTables() { // Delete PREROUTING rule args := []string{ "-m", "conntrack", diff --git a/util/linuxfw/iptables_runner_test.go b/util/linuxfw/iptables_runner_test.go index 0dcade351..77c753004 100644 --- a/util/linuxfw/iptables_runner_test.go +++ b/util/linuxfw/iptables_runner_test.go @@ -364,3 +364,143 @@ func checkSNATRuleCount(t *testing.T, iptr *iptablesRunner, ip netip.Addr, wants t.Fatalf("wants %d rules, got %d", wantsRules, len(rules)) } } + +func TestAddAndDelConnmarkSaveRule(t *testing.T) { + preroutingArgs := []string{ + "-m", "conntrack", + "--ctstate", "ESTABLISHED,RELATED", + "-j", "CONNMARK", + "--restore-mark", + "--nfmask", "0xff0000", + "--ctmask", "0xff0000", + } + + outputArgs := []string{ + "-m", "conntrack", + "--ctstate", "NEW", + "-m", "mark", + "!", "--mark", "0x0/0xff0000", + "-j", "CONNMARK", + "--save-mark", + "--nfmask", "0xff0000", + "--ctmask", "0xff0000", + } + + t.Run("with_ipv6", func(t *testing.T) { + iptr := newFakeIPTablesRunner() + + // Add connmark rules + if err := iptr.AddConnmarkSaveRule(); err != nil { + t.Fatalf("AddConnmarkSaveRule failed: %v", err) + } + + // Verify rules exist in both IPv4 and IPv6 + for _, proto := range []iptablesInterface{iptr.ipt4, iptr.ipt6} { + if exists, err := proto.Exists("mangle", "PREROUTING", preroutingArgs...); err != nil { + t.Fatalf("error checking PREROUTING rule: %v", err) + } else if !exists { + t.Errorf("PREROUTING connmark rule doesn't exist") + } + + if exists, err := proto.Exists("mangle", "OUTPUT", outputArgs...); err != nil { + t.Fatalf("error checking OUTPUT rule: %v", err) + } else if !exists { + t.Errorf("OUTPUT connmark rule doesn't exist") + } + } + + // Test idempotency - calling AddConnmarkSaveRule again should not fail or duplicate + if err := iptr.AddConnmarkSaveRule(); err != nil { + t.Fatalf("AddConnmarkSaveRule (second call) failed: %v", err) + } + + // Verify rules still exist and weren't duplicated + for _, proto := range []iptablesInterface{iptr.ipt4, iptr.ipt6} { + preroutingRules, err := proto.List("mangle", "PREROUTING") + if err != nil { + t.Fatalf("error listing PREROUTING rules: %v", err) + } + connmarkCount := 0 + for _, rule := range preroutingRules { + if strings.Contains(rule, "CONNMARK") && strings.Contains(rule, "restore-mark") { + connmarkCount++ + } + } + if connmarkCount != 1 { + t.Errorf("expected 1 PREROUTING connmark rule, got %d", connmarkCount) + } + } + + // Delete connmark rules + if err := iptr.DelConnmarkSaveRule(); err != nil { + t.Fatalf("DelConnmarkSaveRule failed: %v", err) + } + + // Verify rules are deleted + for _, proto := range []iptablesInterface{iptr.ipt4, iptr.ipt6} { + if exists, err := proto.Exists("mangle", "PREROUTING", preroutingArgs...); err != nil { + t.Fatalf("error checking PREROUTING rule: %v", err) + } else if exists { + t.Errorf("PREROUTING connmark rule still exists after deletion") + } + + if exists, err := proto.Exists("mangle", "OUTPUT", outputArgs...); err != nil { + t.Fatalf("error checking OUTPUT rule: %v", err) + } else if exists { + t.Errorf("OUTPUT connmark rule still exists after deletion") + } + } + + // Test idempotency of deletion + if err := iptr.DelConnmarkSaveRule(); err != nil { + t.Fatalf("DelConnmarkSaveRule (second call) failed: %v", err) + } + }) + + t.Run("without_ipv6", func(t *testing.T) { + // Create an iptables runner with only IPv4 (simulating system without IPv6) + iptr := &iptablesRunner{ + ipt4: newFakeIPTables(), + ipt6: nil, // IPv6 not available + v6Available: false, + v6NATAvailable: false, + v6FilterAvailable: false, + } + + // Add connmark rules should NOT panic with nil ipt6 + if err := iptr.AddConnmarkSaveRule(); err != nil { + t.Fatalf("AddConnmarkSaveRule failed with IPv6 disabled: %v", err) + } + + // Verify rules exist ONLY in IPv4 + if exists, err := iptr.ipt4.Exists("mangle", "PREROUTING", preroutingArgs...); err != nil { + t.Fatalf("error checking IPv4 PREROUTING rule: %v", err) + } else if !exists { + t.Errorf("IPv4 PREROUTING connmark rule doesn't exist") + } + + if exists, err := iptr.ipt4.Exists("mangle", "OUTPUT", outputArgs...); err != nil { + t.Fatalf("error checking IPv4 OUTPUT rule: %v", err) + } else if !exists { + t.Errorf("IPv4 OUTPUT connmark rule doesn't exist") + } + + // Delete connmark rules should NOT panic with nil ipt6 + if err := iptr.DelConnmarkSaveRule(); err != nil { + t.Fatalf("DelConnmarkSaveRule failed with IPv6 disabled: %v", err) + } + + // Verify rules are deleted from IPv4 + if exists, err := iptr.ipt4.Exists("mangle", "PREROUTING", preroutingArgs...); err != nil { + t.Fatalf("error checking IPv4 PREROUTING rule: %v", err) + } else if exists { + t.Errorf("IPv4 PREROUTING connmark rule still exists after deletion") + } + + if exists, err := iptr.ipt4.Exists("mangle", "OUTPUT", outputArgs...); err != nil { + t.Fatalf("error checking IPv4 OUTPUT rule: %v", err) + } else if exists { + t.Errorf("IPv4 OUTPUT connmark rule still exists after deletion") + } + }) +}