github/Bose-SoundTouch
1
0
Fork 0
mirror of https://github.com/gesellix/Bose-SoundTouch.git synced 2026-08-18 08:36:13 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(security): tighten zeroconf URL validation to literal local IPs

Browse Source 
CodeQL re-fired three new go/request-forgery alerts (#134/135/136) on
the lines my previous validateZcBaseURL refactor introduced. The
previous validator accepted hostname-style hosts unchanged, so even
though the IP-class check ran when applicable, u.String() at the call
sites still emitted the original tainted host into the request URL —
which is exactly what CodeQL traces.

Tighten validateZcBaseURL to:

* require the host to parse as a literal IP — DNS / mDNS hostnames
  are rejected (with a clear error explaining the caller should
  resolve to a private IP first); doing the lookup inside the
  validator would re-introduce the SSRF surface CodeQL is flagging,
  because malicious DNS could point a *.local name at a public host
  between the lookup and the request.
* require that IP to be loopback / RFC1918 private / IPv4-or-IPv6
  link-local. Anything else (global IPs in either family) is refused.
* rebuild the returned *url.URL from validated components — scheme
  (already checked), the validated IP literal joined with the
  original port, and the original path. Pre-existing query/fragment
  are stripped so callers attach their own ?action= cleanly. CodeQL
  recognises this fresh-construction pattern as taint sanitisation.

In practice this matches what SoundTouch speakers actually announce:
IP-based zeroconf URLs at port 8200 against an LAN address. The
existing PushCredentials_FullRoundTrip and FallbackOnGetInfoFailure
tests already exercise the loopback path through httptest.NewServer
and pass unchanged.

Adds TestValidateZcBaseURL covering 17 inputs — 9 accept (loopback,
private 10/172/192, link-local v4, IPv6 loopback, IPv6 link-local,
strips query) and 8 reject (public IPv4, public IPv6, hostname,
plain hostname, ftp/file schemes, empty host, unparseable) — to lock
the new contract in.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tobias Gesellchen
2026-05-10 14:30:55 +02:00
co-authored by Claude Opus 4.7
parent 339dc80bf1
commit fbde4e136f
2 changed files with 89 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/service/zeroconf/zeroconf.go
+38 -21
View File
@@ -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=<action> appended.
pkg/service/zeroconf/zeroconf_test.go
+51
View File
@@ -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)
}
})
}
}