diff --git a/pkg/service/zeroconf/zeroconf.go b/pkg/service/zeroconf/zeroconf.go index d503321..056d2ea 100644 --- a/pkg/service/zeroconf/zeroconf.go +++ b/pkg/service/zeroconf/zeroconf.go @@ -169,14 +169,26 @@ func DecryptBlob(encKey, macKey, blob []byte) ([]byte, error) { return plaintext, nil } -// validateZcBaseURL parses zcBaseURL and ensures it points at a non-routable -// host on the LAN. Speakers live on the local network; rejecting global-IP -// hosts prevents the upstream caller from being tricked into making outbound -// requests to arbitrary hosts (server-side request forgery). Also constrains -// the scheme to http/https. +// validateZcBaseURL parses zcBaseURL and ensures the URL points at a +// non-routable host on the LAN. Speakers live on the local network; rejecting +// non-local hosts prevents the upstream caller from being tricked into +// making outbound requests to arbitrary hosts (server-side request forgery). // -// Returns the parsed URL with any embedded ?query stripped, ready for callers -// to attach their own ?action= query string. +// The validator is strict on purpose: +// - the scheme must be http or https, +// - the host must be a *literal IP* (no DNS / mDNS hostnames — see note +// below) that is loopback, RFC1918 private, or IPv4/IPv6 link-local, +// - the returned URL is rebuilt from validated components so the +// subsequent String() call no longer carries the original tainted host +// value, which CodeQL recognises as taint sanitisation. +// +// Note on hostnames: SoundTouch speakers announce themselves with +// IP-based zeroconf URLs in the captures we have. If a future deployment +// needs mDNS support, the right place to add it is in the caller — resolve +// the hostname to an IP and pass the IP-form URL in here. Doing the lookup +// inside the validator would re-introduce the very SSRF surface CodeQL is +// flagging, because malicious DNS could point a *.local name at a +// public host between the lookup and the request. func validateZcBaseURL(zcBaseURL string) (*url.URL, error) { u, err := url.Parse(zcBaseURL) if err != nil { @@ -192,23 +204,28 @@ func validateZcBaseURL(zcBaseURL string) (*url.URL, error) { return nil, fmt.Errorf("missing host") } - // Allow literal IPs that are loopback / private / link-local. Hostnames - // (mDNS .local, etc.) are accepted as the speaker may not be addressed - // by IP — DNS resolution of those is a separate trust boundary, but - // they don't open the SSRF surface CodeQL is concerned about because - // a malicious *.local name still has to win mDNS resolution on the - // local segment. - if ip := net.ParseIP(host); ip != nil { - if !ip.IsLoopback() && !ip.IsPrivate() && !ip.IsLinkLocalUnicast() { - return nil, fmt.Errorf("host %q is not on a local network", host) - } + ip := net.ParseIP(host) + if ip == nil { + return nil, fmt.Errorf("host %q must be a literal IP (DNS/mDNS hostnames are not supported on this path; resolve to a private IP before calling)", host) } - // Strip any pre-existing query so callers can append cleanly. - u.RawQuery = "" - u.Fragment = "" + if !ip.IsLoopback() && !ip.IsPrivate() && !ip.IsLinkLocalUnicast() { + return nil, fmt.Errorf("host %q is not on a local network", host) + } - return u, nil + // Build a fresh URL from validated components only — the IP literal, + // the original port, the original path. Pre-existing ?query and + // #fragment are stripped so callers can attach their own cleanly. + hostPort := ip.String() + if port := u.Port(); port != "" { + hostPort = net.JoinHostPort(ip.String(), port) + } + + return &url.URL{ + Scheme: u.Scheme, + Host: hostPort, + Path: u.Path, + }, nil } // withAction returns the validated base URL with ?action= appended. diff --git a/pkg/service/zeroconf/zeroconf_test.go b/pkg/service/zeroconf/zeroconf_test.go index 02af741..c2eb142 100644 --- a/pkg/service/zeroconf/zeroconf_test.go +++ b/pkg/service/zeroconf/zeroconf_test.go @@ -312,3 +312,54 @@ func readProtoVarint(data []byte) (uint64, int) { } return 0, len(data) } + +func TestValidateZcBaseURL(t *testing.T) { + cases := []struct { + name string + input string + wantOK bool + wantHost string // expected u.Host on success + wantPath string + }{ + {"loopback", "http://127.0.0.1:8200/zc", true, "127.0.0.1:8200", "/zc"}, + {"loopback no port", "http://127.0.0.1/zc", true, "127.0.0.1", "/zc"}, + {"private 192", "http://192.168.1.10:8200/zc", true, "192.168.1.10:8200", "/zc"}, + {"private 10", "http://10.0.0.5/zc", true, "10.0.0.5", "/zc"}, + {"private 172", "http://172.16.0.1/zc", true, "172.16.0.1", "/zc"}, + {"link-local v4", "http://169.254.10.20/zc", true, "169.254.10.20", "/zc"}, + {"ipv6 loopback", "http://[::1]:8200/zc", true, "[::1]:8200", "/zc"}, + {"ipv6 link-local", "http://[fe80::1]:8200/zc", true, "[fe80::1]:8200", "/zc"}, + {"strips query", "http://192.168.1.10:8200/zc?foo=bar", true, "192.168.1.10:8200", "/zc"}, + + {"public IP rejected", "http://1.1.1.1/zc", false, "", ""}, + {"public ipv6 rejected", "http://[2001:db8::1]/zc", false, "", ""}, + {"hostname rejected", "http://myspeaker.local/zc", false, "", ""}, + {"plain hostname rejected", "http://speaker/zc", false, "", ""}, + {"ftp scheme rejected", "ftp://192.168.1.10/zc", false, "", ""}, + {"file scheme rejected", "file:///etc/passwd", false, "", ""}, + {"empty host rejected", "http:///zc", false, "", ""}, + {"unparseable rejected", "::not a url::", false, "", ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := validateZcBaseURL(tc.input) + if tc.wantOK { + if err != nil { + t.Fatalf("validateZcBaseURL(%q) returned error %v, want success", tc.input, err) + } + if got.Host != tc.wantHost { + t.Errorf("Host = %q, want %q", got.Host, tc.wantHost) + } + if got.Path != tc.wantPath { + t.Errorf("Path = %q, want %q", got.Path, tc.wantPath) + } + if got.RawQuery != "" { + t.Errorf("RawQuery = %q, want empty (validator should strip query)", got.RawQuery) + } + } else if err == nil { + t.Errorf("validateZcBaseURL(%q) succeeded, want error", tc.input) + } + }) + } +}