From 074a123a4bb51e6dba649f309c713eaab0af96c2 Mon Sep 17 00:00:00 2001 From: randomizedcoder <64496590+randomizedcoder@users.noreply.github.com> Date: Mon, 18 May 2026 10:23:10 -0700 Subject: [PATCH] Reject port numbers outside [0, 65535] in firewall rule parsing (#1724) --- firewall.go | 39 +++++++++++++++++++-------- firewall_test.go | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 11 deletions(-) diff --git a/firewall.go b/firewall.go index 904c71b2..eb120fa6 100644 --- a/firewall.go +++ b/firewall.go @@ -1055,7 +1055,6 @@ func (r *rule) sanity() error { } func parsePort(s string) (int32, int32, error) { - var err error const notAPort int32 = -2 if s == "any" { return firewall.PortAny, firewall.PortAny, nil @@ -1064,11 +1063,11 @@ func parsePort(s string) (int32, int32, error) { return firewall.PortFragment, firewall.PortFragment, nil } if !strings.Contains(s, `-`) { - rPort, err := strconv.Atoi(s) + rPort, err := parsePortValue("", s) if err != nil { - return notAPort, notAPort, fmt.Errorf("was not a number; `%s`", s) + return notAPort, notAPort, err } - return int32(rPort), int32(rPort), nil + return rPort, rPort, nil } sPorts := strings.SplitN(s, `-`, 2) @@ -1079,22 +1078,40 @@ func parsePort(s string) (int32, int32, error) { return notAPort, notAPort, fmt.Errorf("appears to be a range but could not be parsed; `%s`", s) } - rStartPort, err := strconv.Atoi(sPorts[0]) + startPort, err := parsePortValue("beginning range ", sPorts[0]) if err != nil { - return notAPort, notAPort, fmt.Errorf("beginning range was not a number; `%s`", sPorts[0]) + return notAPort, notAPort, err } - rEndPort, err := strconv.Atoi(sPorts[1]) + endPort, err := parsePortValue("ending range ", sPorts[1]) if err != nil { - return notAPort, notAPort, fmt.Errorf("ending range was not a number; `%s`", sPorts[1]) + return notAPort, notAPort, err } - startPort := int32(rStartPort) - endPort := int32(rEndPort) - if startPort == firewall.PortAny { endPort = firewall.PortAny } return startPort, endPort, nil } + +// parsePortValue accepts a base-10 decimal in [0, 65535] and returns it +// widened to int32. Using strconv.ParseUint with bitSize 16 rejects +// negative input, out-of-range input (>65535), and any non-decimal byte +// by construction, so the int32 widening that follows is provably safe +// and cannot collide with firewall.PortAny (0) or firewall.PortFragment +// (-1) via integer truncation. +// +// prefix is prepended to both error messages so callers can disambiguate +// the single-port path (prefix="") from the range bounds (prefix="beginning +// range " / "ending range "), preserving the historical error strings. +func parsePortValue(prefix, s string) (int32, error) { + n, err := strconv.ParseUint(s, 10, 16) + if err == nil { + return int32(n), nil + } + if errors.Is(err, strconv.ErrRange) { + return 0, fmt.Errorf("%sout of range [0,65535]; `%s`", prefix, s) + } + return 0, fmt.Errorf("%swas not a number; `%s`", prefix, s) +} diff --git a/firewall_test.go b/firewall_test.go index 40b57477..9373f1fd 100644 --- a/firewall_test.go +++ b/firewall_test.go @@ -1029,6 +1029,75 @@ func Test_parsePort(t *testing.T) { require.NoError(t, err) } +// Test_parsePort_invalid covers inputs that must error. The named bug is +// that int32(strconv.Atoi("4294967296")) truncates to 0 == firewall.PortAny, +// silently turning a typo into a match-all-ports rule; the rest are +// representative syntax/range probes. +func Test_parsePort_invalid(t *testing.T) { + tests := []struct { + name string + input string + wantErrContains string + }{ + // Numeric overflow (the named bug + boundary). + {"named bug: 2^32 truncates to PortAny", "4294967296", "out of range"}, + {"just above max real port", "65536", "out of range"}, + + // Negatives route through the range branch and hit the empty-half + // guard; included as defense in depth so a future refactor cannot + // accidentally reach the int32 cast. + {"negative", "-1", "could not be parsed"}, + + // Syntax probes. + {"NUL between digits", "4\x002", "was not a number"}, + {"hex notation", "0x10", "was not a number"}, + {"scientific notation", "1e3", "was not a number"}, + {"leading whitespace", " 42", "was not a number"}, + {"fullwidth digits", "４２", "was not a number"}, + + // Range branch. + {"range upper out of range", "1-65536", "ending range out of range"}, + {"range lower out of range", "65536-65537", "beginning range out of range"}, + {"range with negative upper", "1--1", "ending range was not a number"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, _, err := parsePort(tc.input) + require.Error(t, err, "input %q must error", tc.input) + require.ErrorContains(t, err, tc.wantErrContains) + }) + } +} + +// Test_parsePort_valid_boundaries locks in success cases at 0, 1, and 65535 +// so a future refactor cannot regress the boundaries. +func Test_parsePort_valid_boundaries(t *testing.T) { + tests := []struct { + name string + input string + wantStart int32 + wantEnd int32 + }{ + {"zero is PortAny", "0", 0, 0}, + {"min real port", "1", 1, 1}, + {"max real port", "65535", 65535, 65535}, + {"range zero to max forces end to zero", "0-65535", 0, 0}, + {"range max to max", "65535-65535", 65535, 65535}, + {"range one to max", "1-65535", 1, 65535}, + {"range with whitespace inside", " 1 - 2 ", 1, 2}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + s, e, err := parsePort(tc.input) + require.NoError(t, err) + assert.Equal(t, tc.wantStart, s, "start port") + assert.Equal(t, tc.wantEnd, e, "end port") + }) + } +} + func TestNewFirewallFromConfig(t *testing.T) { l := test.NewLogger() // Test a bad rule definition