From bf3466d5d9c9cba92b6aa32e02fbe35f9b3e207f Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Mon, 25 May 2026 21:08:37 +0200 Subject: [PATCH] sec8: validate zeroconf port to break CodeQL taint chain (alerts 134/135/136) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add validateZcPort alongside validateZcHost: the strconv.Atoi→Itoa round-trip produces a sanitised integer string that CodeQL no longer considers tainted, closing the remaining go/request-forgery findings at zeroconf.go:263, :336, :413. Also rejects clearly invalid inputs (non-numeric, out-of-range) that would previously have produced a silently broken URL. Co-Authored-By: Claude Sonnet 4.6 --- pkg/service/zeroconf/zeroconf.go | 46 ++++++++++++++++++--- pkg/service/zeroconf/zeroconf_test.go | 58 ++++++++++++++++++++++++--- 2 files changed, 93 insertions(+), 11 deletions(-) diff --git a/pkg/service/zeroconf/zeroconf.go b/pkg/service/zeroconf/zeroconf.go index 4dbbfee..2770a3f 100644 --- a/pkg/service/zeroconf/zeroconf.go +++ b/pkg/service/zeroconf/zeroconf.go @@ -20,6 +20,7 @@ import ( "net" "net/http" "net/url" + "strconv" "strings" "time" ) @@ -212,17 +213,41 @@ func validateZcHost(host string) (net.IP, error) { return ip, nil } +// validateZcPort checks that port is a decimal integer in [1, 65535] and +// returns the canonical decimal string produced by strconv.Itoa. The +// round-trip through an integer breaks static-analysis taint chains while +// also rejecting strings like "0", "-1", or "foo" that would silently +// produce a broken URL. +// An empty port string is allowed and returned unchanged — callers that omit +// the port rely on the scheme default. +func validateZcPort(port string) (string, error) { + if port == "" { + return "", nil + } + + p, err := strconv.Atoi(port) + if err != nil || p < 1 || p > 65535 { + return "", fmt.Errorf("zeroconf port %q must be a decimal integer in [1, 65535]", port) + } + + return strconv.Itoa(p), nil +} + // buildZcBase constructs the ZeroConf base URL from a validated IP and port. // The path is always the literal "/zc" — no user-supplied path component ever // flows here, which is what satisfies CodeQL's go/request-forgery model. // port may be empty, in which case the scheme default applies. -func buildZcBase(ip net.IP, port string) *url.URL { +func buildZcBase(ip net.IP, port string) (*url.URL, error) { var host string switch { case port != "": + safePort, err := validateZcPort(port) + if err != nil { + return nil, err + } // net.JoinHostPort brackets IPv6 addresses automatically. - host = net.JoinHostPort(ip.String(), port) + host = net.JoinHostPort(ip.String(), safePort) case ip.To4() == nil: // IPv6 address without a port must be bracketed in a URL host field. host = "[" + ip.String() + "]" @@ -234,7 +259,7 @@ func buildZcBase(ip net.IP, port string) *url.URL { Scheme: "http", Host: host, Path: "/zc", - } + }, nil } // withAction returns the validated base URL with ?action= appended. @@ -256,7 +281,10 @@ func GetInfo(host, port string) ([]byte, error) { return nil, fmt.Errorf("getInfo: %w", err) } - base := buildZcBase(ip, port) + base, err := buildZcBase(ip, port) + if err != nil { + return nil, fmt.Errorf("getInfo: %w", err) + } client := &http.Client{Timeout: 10 * time.Second} @@ -303,7 +331,10 @@ func PushCredentials(host, port, username, accessToken string) error { return fmt.Errorf("pushCredentials: %w", err) } - base := buildZcBase(ip, port) + base, err := buildZcBase(ip, port) + if err != nil { + return fmt.Errorf("pushCredentials: %w", err) + } speakerPublicKey, err := GetInfo(host, port) if err != nil { @@ -400,7 +431,10 @@ func pushSimplifiedToken(host, port, username, accessToken string) error { return fmt.Errorf("pushSimplifiedToken: %w", err) } - base := buildZcBase(ip, port) + base, err := buildZcBase(ip, port) + if err != nil { + return fmt.Errorf("pushSimplifiedToken: %w", err) + } data := url.Values{} data.Set("userName", username) diff --git a/pkg/service/zeroconf/zeroconf_test.go b/pkg/service/zeroconf/zeroconf_test.go index 5368b76..d9bc86d 100644 --- a/pkg/service/zeroconf/zeroconf_test.go +++ b/pkg/service/zeroconf/zeroconf_test.go @@ -365,17 +365,56 @@ func TestValidateZcHost(t *testing.T) { } } +func TestValidateZcPort(t *testing.T) { + cases := []struct { + input string + wantOut string + wantErr bool + }{ + {"8200", "8200", false}, + {"1", "1", false}, + {"65535", "65535", false}, + {"", "", false}, // empty → scheme default, not an error + {"0", "", true}, // below range + {"-1", "", true}, // negative + {"65536", "", true}, // above range + {"foo", "", true}, // non-numeric + {"8200x", "", true}, // trailing garbage + } + + for _, tc := range cases { + t.Run(tc.input, func(t *testing.T) { + got, err := validateZcPort(tc.input) + if tc.wantErr { + if err == nil { + t.Errorf("validateZcPort(%q) succeeded with %q, want error", tc.input, got) + } + return + } + if err != nil { + t.Fatalf("validateZcPort(%q) returned error %v, want success", tc.input, err) + } + if got != tc.wantOut { + t.Errorf("validateZcPort(%q) = %q, want %q", tc.input, got, tc.wantOut) + } + }) + } +} + func TestBuildZcBase(t *testing.T) { cases := []struct { name string host string port string wantURL string + wantErr bool }{ - {"with port", "127.0.0.1", "8200", "http://127.0.0.1:8200/zc"}, - {"without port", "192.168.1.1", "", "http://192.168.1.1/zc"}, - {"ipv6 with port", "::1", "8200", "http://[::1]:8200/zc"}, - {"ipv6 without port", "::1", "", "http://[::1]/zc"}, + {"with port", "127.0.0.1", "8200", "http://127.0.0.1:8200/zc", false}, + {"without port", "192.168.1.1", "", "http://192.168.1.1/zc", false}, + {"ipv6 with port", "::1", "8200", "http://[::1]:8200/zc", false}, + {"ipv6 without port", "::1", "", "http://[::1]/zc", false}, + {"invalid port", "127.0.0.1", "foo", "", true}, + {"port out of range", "127.0.0.1", "99999", "", true}, } for _, tc := range cases { @@ -384,7 +423,16 @@ func TestBuildZcBase(t *testing.T) { if ip == nil { t.Fatalf("test setup: net.ParseIP(%q) returned nil", tc.host) } - got := buildZcBase(ip, tc.port) + got, err := buildZcBase(ip, tc.port) + if tc.wantErr { + if err == nil { + t.Errorf("buildZcBase(%q, %q) succeeded, want error", tc.host, tc.port) + } + return + } + if err != nil { + t.Fatalf("buildZcBase(%q, %q) returned error %v, want success", tc.host, tc.port, err) + } if got.String() != tc.wantURL { t.Errorf("buildZcBase(%q, %q) = %q, want %q", tc.host, tc.port, got.String(), tc.wantURL) }