Reject port numbers outside [0, 65535] in firewall rule parsing (#1724)

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)
+}

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