diff --git a/internal/firewall/firewall.go b/internal/firewall/firewall.go index f730afe..43faf6d 100644 --- a/internal/firewall/firewall.go +++ b/internal/firewall/firewall.go @@ -419,11 +419,61 @@ func Enable(ctx context.Context, run Runner) error { // Disable retires ufw without flushing it: its configuration stays on disk, and the container // runtime's rules are not its to remove. +// +// **Nor is the forward policy ufw's to open.** Measured on a lab machine running the container +// runtime with a published port (testdata/ufw-disable-iptables-before.txt and -after.txt): +// `ufw disable` sets every built-in chain's policy to accept, the forward chain's among them. The +// runtime had set that one to drop when it turned forwarding on, and it does not set it again while +// forwarding stays on — not even on a restart. Left so, a retired ufw turns the machine into a +// router for anyone who can reach it. So each family's forward policy is read before, and one that +// was drop is put back and read back. func Disable(ctx context.Context, run Runner) error { + type family struct{ tool, policy string } + var before []family + for _, tool := range []string{"iptables", "ip6tables"} { + if policy, ok := forwardPolicy(ctx, run, tool); ok { + before = append(before, family{tool, policy}) + } + } if _, err := run(ctx, "ufw", "disable"); err != nil { return fmt.Errorf("disabling ufw: %w", err) } - return expectActive(ctx, run, false) + if err := expectActive(ctx, run, false); err != nil { + return err + } + for _, f := range before { + if f.policy != "DROP" { + continue + } + if now, ok := forwardPolicy(ctx, run, f.tool); ok && now == "DROP" { + continue + } + if _, err := run(ctx, f.tool, "-P", "FORWARD", "DROP"); err != nil { + return fmt.Errorf("ufw is disabled, and %s's forward policy, which was drop, could not be put back: %w", + f.tool, err) + } + if now, ok := forwardPolicy(ctx, run, f.tool); !ok || now != "DROP" { + return fmt.Errorf("ufw is disabled, and %s's forward policy was put back to drop and reads %q", + f.tool, now) + } + } + return nil +} + +// forwardPolicy reads the forward chain's policy the way iptables prints it: "-P FORWARD DROP". +// Not ok when the tool is absent or says nothing readable. +func forwardPolicy(ctx context.Context, run Runner, tool string) (string, bool) { + out, err := run(ctx, tool, "-S", "FORWARD") + if err != nil { + return "", false + } + for _, line := range strings.Split(out, "\n") { + f := strings.Fields(line) + if len(f) == 3 && f[0] == "-P" && f[1] == "FORWARD" { + return f[2], true + } + } + return "", false } func expectActive(ctx context.Context, run Runner, want bool) error { diff --git a/internal/firewall/firewall_test.go b/internal/firewall/firewall_test.go index c810a03..4605654 100644 --- a/internal/firewall/firewall_test.go +++ b/internal/firewall/firewall_test.go @@ -114,6 +114,41 @@ type fakeUFW struct { ruleset string firewalld bool asked []string + + // iptablesActive and iptablesInactive are what `iptables -S` prints with ufw active and + // after it is disabled; empty is a machine without iptables. forward is a policy set since. + iptablesActive, iptablesInactive string + forward string +} + +// iptables answers `iptables -S FORWARD` from the captured output for ufw's state, and records +// a forward policy set with -P. +func (f *fakeUFW) iptables(name string, args []string) (string, error) { + if f.iptablesActive == "" { + return "", &exec.Error{Name: name, Err: exec.ErrNotFound} + } + if name == "ip6tables" { + return "", &exec.Error{Name: name, Err: exec.ErrNotFound} + } + if len(args) == 3 && args[0] == "-P" && args[1] == "FORWARD" { + f.forward = args[2] + return "", nil + } + captured := f.iptablesInactive + if f.active { + captured = f.iptablesActive + } + var out []string + for _, line := range strings.Split(captured, "\n") { + fields := strings.Fields(line) + if len(fields) >= 2 && fields[1] == "FORWARD" { + if fields[0] == "-P" && f.forward != "" && !f.active { + line = "-P FORWARD " + f.forward + } + out = append(out, line) + } + } + return strings.Join(out, "\n") + "\n", nil } func canonical(args []string) string { @@ -155,6 +190,8 @@ func (f *fakeUFW) run(_ context.Context, name string, args ...string) (string, e return f.ruleset, nil case "iptables-legacy", "ip6tables-legacy": return "", &exec.Error{Name: name, Err: exec.ErrNotFound} + case "iptables", "ip6tables": + return f.iptables(name, args) case "ufw": default: return "", fmt.Errorf("unexpected %s", name) @@ -179,6 +216,7 @@ func (f *fakeUFW) run(_ context.Context, name string, args ...string) (string, e return "Firewall is active and enabled on system startup\n", nil case args[0] == "disable": f.active = false + f.forward = "" return "Firewall stopped and disabled on system startup\n", nil case args[0] == "delete" && len(args) > 1 && args[1] == "route": // As the real ufw answers it (captured in testdata/ufw-delete.txt): a route rule is @@ -424,3 +462,41 @@ func TestARealUfwRulesetIsUfw(t *testing.T) { t.Error("ufw's drop chains, with ufw not known to be active, read as refusing nothing") } } + +func TestRetiringUfwKeepsTheMachineFromRoutingForOthers(t *testing.T) { + // Captured on a lab machine running the container runtime with a published port: ufw active, + // then `ufw disable`. Disabling set the forward policy the runtime had set to drop to accept. + before, err := os.ReadFile("testdata/ufw-disable-iptables-before.txt") + if err != nil { + t.Fatal(err) + } + after, err := os.ReadFile("testdata/ufw-disable-iptables-after.txt") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(before), "-P FORWARD DROP") || !strings.Contains(string(after), "-P FORWARD ACCEPT") { + t.Fatal("the captures no longer show ufw disable opening the forward policy") + } + f := &fakeUFW{installed: true, active: true, iptablesActive: string(before), iptablesInactive: string(after)} + if err := Disable(context.Background(), f.run); err != nil { + t.Fatal(err) + } + if f.active { + t.Fatal("ufw is still active") + } + if f.forward != "DROP" { + t.Errorf("the forward policy was left open after ufw was retired: %v", f.asked) + } + for _, a := range f.asked { + if strings.Contains(a, "-F") || strings.Contains(a, "flush") || strings.Contains(a, "reset") { + t.Errorf("retiring ufw flushed something: %s", a) + } + } +} + +func TestRetiringUfwOnAMachineWithoutIptablesStillRetiresIt(t *testing.T) { + f := &fakeUFW{installed: true, active: true} + if err := Disable(context.Background(), f.run); err != nil || f.active { + t.Fatalf("disable: %v, active %v", err, f.active) + } +} diff --git a/internal/firewall/testdata/ufw-disable-iptables-after.txt b/internal/firewall/testdata/ufw-disable-iptables-after.txt new file mode 100644 index 0000000..22dbc77 --- /dev/null +++ b/internal/firewall/testdata/ufw-disable-iptables-after.txt @@ -0,0 +1,55 @@ +-P INPUT ACCEPT +-P FORWARD ACCEPT +-P OUTPUT ACCEPT +-N DOCKER +-N DOCKER-BRIDGE +-N DOCKER-CT +-N DOCKER-FORWARD +-N DOCKER-INTERNAL +-N DOCKER-USER +-N ufw-after-forward +-N ufw-after-input +-N ufw-after-logging-forward +-N ufw-after-logging-input +-N ufw-after-logging-output +-N ufw-after-output +-N ufw-before-forward +-N ufw-before-input +-N ufw-before-logging-forward +-N ufw-before-logging-input +-N ufw-before-logging-output +-N ufw-before-output +-N ufw-reject-forward +-N ufw-reject-input +-N ufw-reject-output +-N ufw-track-forward +-N ufw-track-input +-N ufw-track-output +-A INPUT -j ufw-before-logging-input +-A INPUT -j ufw-before-input +-A INPUT -j ufw-after-input +-A INPUT -j ufw-after-logging-input +-A INPUT -j ufw-reject-input +-A INPUT -j ufw-track-input +-A FORWARD -j DOCKER-USER +-A FORWARD -j DOCKER-FORWARD +-A FORWARD -j ufw-before-logging-forward +-A FORWARD -j ufw-before-forward +-A FORWARD -j ufw-after-forward +-A FORWARD -j ufw-after-logging-forward +-A FORWARD -j ufw-reject-forward +-A FORWARD -j ufw-track-forward +-A OUTPUT -j ufw-before-logging-output +-A OUTPUT -j ufw-before-output +-A OUTPUT -j ufw-after-output +-A OUTPUT -j ufw-after-logging-output +-A OUTPUT -j ufw-reject-output +-A OUTPUT -j ufw-track-output +-A DOCKER -d 172.17.0.2/32 ! -i docker0 -o docker0 -p tcp -m tcp --dport 80 -j ACCEPT +-A DOCKER ! -i docker0 -o docker0 -j DROP +-A DOCKER-BRIDGE -o docker0 -j DOCKER +-A DOCKER-CT -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +-A DOCKER-FORWARD -j DOCKER-CT +-A DOCKER-FORWARD -j DOCKER-INTERNAL +-A DOCKER-FORWARD -j DOCKER-BRIDGE +-A DOCKER-FORWARD -i docker0 -j ACCEPT diff --git a/internal/firewall/testdata/ufw-disable-iptables-before.txt b/internal/firewall/testdata/ufw-disable-iptables-before.txt new file mode 100644 index 0000000..4e18c44 --- /dev/null +++ b/internal/firewall/testdata/ufw-disable-iptables-before.txt @@ -0,0 +1,119 @@ +-P INPUT DROP +-P FORWARD DROP +-P OUTPUT ACCEPT +-N DOCKER +-N DOCKER-BRIDGE +-N DOCKER-CT +-N DOCKER-FORWARD +-N DOCKER-INTERNAL +-N DOCKER-USER +-N ufw-after-forward +-N ufw-after-input +-N ufw-after-logging-forward +-N ufw-after-logging-input +-N ufw-after-logging-output +-N ufw-after-output +-N ufw-before-forward +-N ufw-before-input +-N ufw-before-logging-forward +-N ufw-before-logging-input +-N ufw-before-logging-output +-N ufw-before-output +-N ufw-logging-allow +-N ufw-logging-deny +-N ufw-not-local +-N ufw-reject-forward +-N ufw-reject-input +-N ufw-reject-output +-N ufw-skip-to-policy-forward +-N ufw-skip-to-policy-input +-N ufw-skip-to-policy-output +-N ufw-track-forward +-N ufw-track-input +-N ufw-track-output +-N ufw-user-forward +-N ufw-user-input +-N ufw-user-limit +-N ufw-user-limit-accept +-N ufw-user-logging-forward +-N ufw-user-logging-input +-N ufw-user-logging-output +-N ufw-user-output +-A INPUT -j ufw-before-logging-input +-A INPUT -j ufw-before-input +-A INPUT -j ufw-after-input +-A INPUT -j ufw-after-logging-input +-A INPUT -j ufw-reject-input +-A INPUT -j ufw-track-input +-A FORWARD -j DOCKER-USER +-A FORWARD -j DOCKER-FORWARD +-A FORWARD -j ufw-before-logging-forward +-A FORWARD -j ufw-before-forward +-A FORWARD -j ufw-after-forward +-A FORWARD -j ufw-after-logging-forward +-A FORWARD -j ufw-reject-forward +-A FORWARD -j ufw-track-forward +-A OUTPUT -j ufw-before-logging-output +-A OUTPUT -j ufw-before-output +-A OUTPUT -j ufw-after-output +-A OUTPUT -j ufw-after-logging-output +-A OUTPUT -j ufw-reject-output +-A OUTPUT -j ufw-track-output +-A DOCKER -d 172.17.0.2/32 ! -i docker0 -o docker0 -p tcp -m tcp --dport 80 -j ACCEPT +-A DOCKER ! -i docker0 -o docker0 -j DROP +-A DOCKER-BRIDGE -o docker0 -j DOCKER +-A DOCKER-CT -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +-A DOCKER-FORWARD -j DOCKER-CT +-A DOCKER-FORWARD -j DOCKER-INTERNAL +-A DOCKER-FORWARD -j DOCKER-BRIDGE +-A DOCKER-FORWARD -i docker0 -j ACCEPT +-A ufw-after-input -p udp -m udp --dport 137 -j ufw-skip-to-policy-input +-A ufw-after-input -p udp -m udp --dport 138 -j ufw-skip-to-policy-input +-A ufw-after-input -p tcp -m tcp --dport 139 -j ufw-skip-to-policy-input +-A ufw-after-input -p tcp -m tcp --dport 445 -j ufw-skip-to-policy-input +-A ufw-after-input -p udp -m udp --dport 67 -j ufw-skip-to-policy-input +-A ufw-after-input -p udp -m udp --dport 68 -j ufw-skip-to-policy-input +-A ufw-after-input -m addrtype --dst-type BROADCAST -j ufw-skip-to-policy-input +-A ufw-after-logging-forward -m limit --limit 3/min --limit-burst 10 -j LOG --log-prefix "[UFW BLOCK] " +-A ufw-after-logging-input -m limit --limit 3/min --limit-burst 10 -j LOG --log-prefix "[UFW BLOCK] " +-A ufw-before-forward -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +-A ufw-before-forward -p icmp -m icmp --icmp-type 3 -j ACCEPT +-A ufw-before-forward -p icmp -m icmp --icmp-type 11 -j ACCEPT +-A ufw-before-forward -p icmp -m icmp --icmp-type 12 -j ACCEPT +-A ufw-before-forward -p icmp -m icmp --icmp-type 8 -j ACCEPT +-A ufw-before-forward -j ufw-user-forward +-A ufw-before-input -i lo -j ACCEPT +-A ufw-before-input -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +-A ufw-before-input -m conntrack --ctstate INVALID -j ufw-logging-deny +-A ufw-before-input -m conntrack --ctstate INVALID -j DROP +-A ufw-before-input -p icmp -m icmp --icmp-type 3 -j ACCEPT +-A ufw-before-input -p icmp -m icmp --icmp-type 11 -j ACCEPT +-A ufw-before-input -p icmp -m icmp --icmp-type 12 -j ACCEPT +-A ufw-before-input -p icmp -m icmp --icmp-type 8 -j ACCEPT +-A ufw-before-input -p udp -m udp --sport 67 --dport 68 -j ACCEPT +-A ufw-before-input -j ufw-not-local +-A ufw-before-input -d 224.0.0.251/32 -p udp -m udp --dport 5353 -j ACCEPT +-A ufw-before-input -d 239.255.255.250/32 -p udp -m udp --dport 1900 -j ACCEPT +-A ufw-before-input -j ufw-user-input +-A ufw-before-output -o lo -j ACCEPT +-A ufw-before-output -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +-A ufw-before-output -j ufw-user-output +-A ufw-logging-allow -m limit --limit 3/min --limit-burst 10 -j LOG --log-prefix "[UFW ALLOW] " +-A ufw-logging-deny -m conntrack --ctstate INVALID -m limit --limit 3/min --limit-burst 10 -j RETURN +-A ufw-logging-deny -m limit --limit 3/min --limit-burst 10 -j LOG --log-prefix "[UFW BLOCK] " +-A ufw-not-local -m addrtype --dst-type LOCAL -j RETURN +-A ufw-not-local -m addrtype --dst-type MULTICAST -j RETURN +-A ufw-not-local -m addrtype --dst-type BROADCAST -j RETURN +-A ufw-not-local -m limit --limit 3/min --limit-burst 10 -j ufw-logging-deny +-A ufw-not-local -j DROP +-A ufw-skip-to-policy-forward -j DROP +-A ufw-skip-to-policy-input -j DROP +-A ufw-skip-to-policy-output -j ACCEPT +-A ufw-track-output -p tcp -m conntrack --ctstate NEW -j ACCEPT +-A ufw-track-output -p udp -m conntrack --ctstate NEW -j ACCEPT +-A ufw-user-input -p tcp -m tcp --dport 22 -j ACCEPT +-A ufw-user-input -p tcp -m tcp --dport 5671 -j ACCEPT +-A ufw-user-input -i mesh0 -p tcp -m tcp --dport 5432 -j ACCEPT +-A ufw-user-limit -m limit --limit 3/min -j LOG --log-prefix "[UFW LIMIT BLOCK] " +-A ufw-user-limit -j REJECT --reject-with icmp-port-unreachable +-A ufw-user-limit-accept -j ACCEPT