diff --git a/pkg/service/amazon/zeroconf.go b/pkg/service/amazon/zeroconf.go index 8d09509..375598f 100644 --- a/pkg/service/amazon/zeroconf.go +++ b/pkg/service/amazon/zeroconf.go @@ -9,7 +9,8 @@ var ErrAddUserNoOp = zeroconf.ErrAddUserNoOp // PushAmazonCredentials pushes Amazon Music credentials to a speaker using the // ZeroConf DH key exchange protocol. Falls back to simplified token push if // the speaker does not support DH (older firmware). -// zcBaseURL is the base URL of the ZeroConf endpoint, e.g. "http://192.168.10.10:8200/zc". -func PushAmazonCredentials(zcBaseURL, username, accessToken string) error { - return zeroconf.PushCredentials(zcBaseURL, username, accessToken) +// host must be a literal private-network IP address. +// port is the ZeroConf port (typically "8200"); pass "" to omit it from the URL. +func PushAmazonCredentials(host, port, username, accessToken string) error { + return zeroconf.PushCredentials(host, port, username, accessToken) } diff --git a/pkg/service/amazon/zeroconf_test.go b/pkg/service/amazon/zeroconf_test.go index 857ae7c..b51358f 100644 --- a/pkg/service/amazon/zeroconf_test.go +++ b/pkg/service/amazon/zeroconf_test.go @@ -3,6 +3,7 @@ package amazon import ( "encoding/base64" "encoding/json" + "net" "net/http" "net/http/httptest" "testing" @@ -100,7 +101,8 @@ func TestPushAmazonCredentials_FullRoundTrip(t *testing.T) { const wantUsername = "amazonuser@example.com" const wantToken = "Atza|access-token" - if err := PushAmazonCredentials(srv.URL+"/zc", wantUsername, wantToken); err != nil { + host, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) + if err := PushAmazonCredentials(host, port, wantUsername, wantToken); err != nil { t.Fatalf("PushAmazonCredentials: %v", err) } @@ -143,7 +145,8 @@ func TestPushAmazonCredentials_FallbackOnGetInfoFailure(t *testing.T) { const wantUsername = "amazonuser@example.com" const wantToken = "Atza|raw-access-token" - if err := PushAmazonCredentials(srv.URL+"/zc", wantUsername, wantToken); err != nil { + host, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) + if err := PushAmazonCredentials(host, port, wantUsername, wantToken); err != nil { t.Fatalf("PushAmazonCredentials: %v", err) } diff --git a/pkg/service/handlers/server.go b/pkg/service/handlers/server.go index 7e00fc7..35459fb 100644 --- a/pkg/service/handlers/server.go +++ b/pkg/service/handlers/server.go @@ -965,14 +965,14 @@ func (s *Server) findExistingDeviceInfoByIP(ip string) *models.ServiceDeviceInfo } func (s *Server) pushSpotifyTokenToDevice(deviceIP, username, accessToken string) error { - var zcURL string - if _, _, err := net.SplitHostPort(deviceIP); err == nil { - zcURL = fmt.Sprintf("http://%s/zc", deviceIP) - } else { - zcURL = fmt.Sprintf("http://%s:8200/zc", deviceIP) + host, port, err := net.SplitHostPort(deviceIP) + if err != nil { + // deviceIP has no port component — use the standard ZeroConf port. + host = deviceIP + port = "8200" } - return spotify.PushSpotifyCredentials(zcURL, username, accessToken) + return spotify.PushSpotifyCredentials(host, port, username, accessToken) } // PrimeDeviceWithAmazon triggers an Amazon Music priming of the speaker if an Amazon account is linked. @@ -1010,14 +1010,14 @@ func (s *Server) PrimeDeviceWithAmazon(deviceIP string) { } func (s *Server) pushAmazonTokenToDevice(deviceIP, username, accessToken string) error { - var zcURL string - if _, _, err := net.SplitHostPort(deviceIP); err == nil { - zcURL = fmt.Sprintf("http://%s/zc", deviceIP) - } else { - zcURL = fmt.Sprintf("http://%s:8200/zc", deviceIP) + host, port, err := net.SplitHostPort(deviceIP) + if err != nil { + // deviceIP has no port component — use the standard ZeroConf port. + host = deviceIP + port = "8200" } - return amazon.PushAmazonCredentials(zcURL, username, accessToken) + return amazon.PushAmazonCredentials(host, port, username, accessToken) } func (s *Server) handleDiscoveredDevice(d models.DiscoveredDevice) { diff --git a/pkg/service/spotify/zeroconf.go b/pkg/service/spotify/zeroconf.go index c8a835f..ad0fb00 100644 --- a/pkg/service/spotify/zeroconf.go +++ b/pkg/service/spotify/zeroconf.go @@ -8,14 +8,17 @@ import "github.com/gesellix/bose-soundtouch/pkg/service/zeroconf" var ErrAddUserNoOp = zeroconf.ErrAddUserNoOp // ZeroConfGetInfo fetches the speaker's DH public key via GET ?action=getInfo. -func ZeroConfGetInfo(zcBaseURL string) ([]byte, error) { - return zeroconf.GetInfo(zcBaseURL) +// host must be a literal private-network IP address. +// port is the ZeroConf port (typically "8200"); pass "" to omit it from the URL. +func ZeroConfGetInfo(host, port string) ([]byte, error) { + return zeroconf.GetInfo(host, port) } // PushSpotifyCredentials pushes Spotify credentials to a speaker using the full // ZeroConf DH key exchange protocol. Falls back to simplified token push if // the speaker does not support DH (older firmware). -// zcBaseURL is the base URL of the ZeroConf endpoint, e.g. "http://192.168.10.10:8200/zc". -func PushSpotifyCredentials(zcBaseURL, username, accessToken string) error { - return zeroconf.PushCredentials(zcBaseURL, username, accessToken) +// host must be a literal private-network IP address. +// port is the ZeroConf port (typically "8200"); pass "" to omit it from the URL. +func PushSpotifyCredentials(host, port, username, accessToken string) error { + return zeroconf.PushCredentials(host, port, username, accessToken) } diff --git a/pkg/service/spotify/zeroconf_test.go b/pkg/service/spotify/zeroconf_test.go index 235a5cd..a8699bd 100644 --- a/pkg/service/spotify/zeroconf_test.go +++ b/pkg/service/spotify/zeroconf_test.go @@ -4,6 +4,7 @@ import ( "encoding/base64" "encoding/json" "fmt" + "net" "net/http" "net/http/httptest" "testing" @@ -83,7 +84,8 @@ func TestPushSpotifyCredentials_FullRoundTrip(t *testing.T) { const wantUsername = "spotifyuser@example.com" const wantToken = "eyJhbGciOiJSUzI1NiJ9.access-token" - if err := PushSpotifyCredentials(srv.URL+"/zc", wantUsername, wantToken); err != nil { + host, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) + if err := PushSpotifyCredentials(host, port, wantUsername, wantToken); err != nil { t.Fatalf("PushSpotifyCredentials: %v", err) } @@ -129,7 +131,8 @@ func TestPushSpotifyCredentials_FallbackOnGetInfoFailure(t *testing.T) { const wantUsername = "spotifyuser@example.com" const wantToken = "raw-access-token" - if err := PushSpotifyCredentials(srv.URL+"/zc", wantUsername, wantToken); err != nil { + host, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) + if err := PushSpotifyCredentials(host, port, wantUsername, wantToken); err != nil { t.Fatalf("PushSpotifyCredentials: %v", err) } diff --git a/pkg/service/zeroconf/zeroconf.go b/pkg/service/zeroconf/zeroconf.go index 77569bc..4dbbfee 100644 --- a/pkg/service/zeroconf/zeroconf.go +++ b/pkg/service/zeroconf/zeroconf.go @@ -180,70 +180,61 @@ func DecryptBlob(encKey, macKey, blob []byte) ([]byte, error) { return plaintext, nil } -// 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). +// validateZcHost parses host as a literal IP and ensures it is on the local +// network. Speakers live on the LAN; rejecting non-local addresses prevents +// SSRF gadgets that could trick the service into reaching arbitrary hosts. // -// 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 { - return nil, fmt.Errorf("zeroconf URL %q: parse: %w", zcBaseURL, err) - } - - if u.Scheme != "http" && u.Scheme != "https" { - return nil, fmt.Errorf("zeroconf URL %q: scheme %q not allowed — must be http or https", zcBaseURL, u.Scheme) - } - - host := u.Hostname() +// Only literal IPs are accepted — no DNS/mDNS hostnames. If the caller has a +// hostname, resolve it first and pass the resulting IP. Doing the lookup here +// would re-introduce the SSRF surface, because malicious DNS could point a +// *.local name at a public address between the lookup and the request. +func validateZcHost(host string) (net.IP, error) { if host == "" { - return nil, fmt.Errorf("zeroconf URL %q: missing host", zcBaseURL) + return nil, fmt.Errorf("zeroconf host must not be empty") } ip := net.ParseIP(host) if ip == nil { return nil, fmt.Errorf( - "zeroconf URL %q: host %q must be a literal IP — resolve the hostname to a private-network IP first "+ + "zeroconf host %q must be a literal IP — resolve the hostname first "+ "(e.g. `getent hosts %s` or `dig +short %s`) and retry with the resolved address", - zcBaseURL, host, host, host) + host, host, host) } if !ip.IsLoopback() && !ip.IsPrivate() && !ip.IsLinkLocalUnicast() { return nil, fmt.Errorf( - "zeroconf URL %q: host %q is not on a local network — only loopback (127.0.0.0/8, ::1), "+ + "zeroconf host %q is not on a local network — only loopback (127.0.0.0/8, ::1), "+ "RFC1918 private (10/8, 172.16/12, 192.168/16) and link-local (169.254/16, fe80::/10) "+ "addresses are accepted", - zcBaseURL, host) + host) } - // 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 ip, 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 { + var host string + + switch { + case port != "": + // net.JoinHostPort brackets IPv6 addresses automatically. + host = net.JoinHostPort(ip.String(), port) + case ip.To4() == nil: + // IPv6 address without a port must be bracketed in a URL host field. + host = "[" + ip.String() + "]" + default: + host = ip.String() } return &url.URL{ - Scheme: u.Scheme, - Host: hostPort, - Path: u.Path, - }, nil + Scheme: "http", + Host: host, + Path: "/zc", + } } // withAction returns the validated base URL with ?action= appended. @@ -257,12 +248,16 @@ func withAction(base *url.URL, action string) string { } // GetInfo fetches the speaker's DH public key via GET ?action=getInfo. -func GetInfo(zcBaseURL string) ([]byte, error) { - base, err := validateZcBaseURL(zcBaseURL) +// host must be a literal private-network IP address. +// port is the ZeroConf port (typically "8200"); pass "" to omit it from the URL. +func GetInfo(host, port string) ([]byte, error) { + ip, err := validateZcHost(host) if err != nil { return nil, fmt.Errorf("getInfo: %w", err) } + base := buildZcBase(ip, port) + client := &http.Client{Timeout: 10 * time.Second} resp, err := client.Get(withAction(base, "getInfo")) @@ -300,17 +295,20 @@ func GetInfo(zcBaseURL string) ([]byte, error) { // PushCredentials pushes OAuth credentials to a speaker using the ZeroConf DH // key exchange protocol. If getInfo fails (older firmware without DH support), // it falls back to the simplified tokenType=accesstoken approach. -// zcBaseURL is the base URL of the ZeroConf endpoint, e.g. "http://192.168.10.10:8200/zc". -func PushCredentials(zcBaseURL, username, accessToken string) error { - base, err := validateZcBaseURL(zcBaseURL) +// host must be a literal private-network IP address. +// port is the ZeroConf port (typically "8200"); pass "" to omit it from the URL. +func PushCredentials(host, port, username, accessToken string) error { + ip, err := validateZcHost(host) if err != nil { return fmt.Errorf("pushCredentials: %w", err) } - speakerPublicKey, err := GetInfo(zcBaseURL) + base := buildZcBase(ip, port) + + speakerPublicKey, err := GetInfo(host, port) if err != nil { log.Printf("[ZeroConf] getInfo failed (%s), falling back to simplified token push", sanitizeErr(err)) - return pushSimplifiedToken(zcBaseURL, username, accessToken) + return pushSimplifiedToken(host, port, username, accessToken) } privateKey, ourPublicKeyBytes, err := GenerateDHKeyPair() @@ -396,12 +394,14 @@ func logAddUserFailure(path string, base *url.URL, username string, resp *http.R // pushSimplifiedToken is the fallback for firmware that does not support DH // key exchange. It sends the raw OAuth access token directly as the blob. -func pushSimplifiedToken(zcBaseURL, username, accessToken string) error { - base, err := validateZcBaseURL(zcBaseURL) +func pushSimplifiedToken(host, port, username, accessToken string) error { + ip, err := validateZcHost(host) if err != nil { return fmt.Errorf("pushSimplifiedToken: %w", err) } + base := buildZcBase(ip, port) + data := url.Values{} data.Set("userName", username) data.Set("blob", accessToken) diff --git a/pkg/service/zeroconf/zeroconf_test.go b/pkg/service/zeroconf/zeroconf_test.go index 44772a4..5368b76 100644 --- a/pkg/service/zeroconf/zeroconf_test.go +++ b/pkg/service/zeroconf/zeroconf_test.go @@ -5,11 +5,18 @@ import ( "encoding/json" "errors" "fmt" + "net" "net/http" "net/http/httptest" "testing" ) +// srvHostPort returns the host and port of a test server's listener. +func srvHostPort(srv *httptest.Server) (host, port string) { + host, port, _ = net.SplitHostPort(srv.Listener.Addr().String()) + return +} + func TestGenerateDHKeyPair(t *testing.T) { priv1, pub1, err := GenerateDHKeyPair() if err != nil { @@ -199,7 +206,8 @@ func TestPushCredentials_FullRoundTrip(t *testing.T) { const wantUsername = "user@example.com" const wantToken = "eyJhbGciOiJSUzI1NiJ9.access-token" - if err := PushCredentials(srv.URL+"/zc", wantUsername, wantToken); err != nil { + host, port := srvHostPort(srv) + if err := PushCredentials(host, port, wantUsername, wantToken); err != nil { t.Fatalf("PushCredentials: %v", err) } @@ -242,7 +250,8 @@ func TestPushCredentials_FallbackOnGetInfoFailure(t *testing.T) { const wantUsername = "user@example.com" const wantToken = "raw-access-token" - if err := PushCredentials(srv.URL+"/zc", wantUsername, wantToken); err != nil { + host, port := srvHostPort(srv) + if err := PushCredentials(host, port, wantUsername, wantToken); err != nil { t.Fatalf("PushCredentials: %v", err) } @@ -314,57 +323,76 @@ func readProtoVarint(data []byte) (uint64, int) { return 0, len(data) } -func TestValidateZcBaseURL(t *testing.T) { +func TestValidateZcHost(t *testing.T) { + // These cases use RFC-1918 192.168/16 addresses — validateZcHost accepts + // loopback, RFC-1918, and link-local only. RFC-5737 doc IPs (192.0.2/24 etc.) + // would be rejected, so tests use real private-range values. cases := []struct { - name string - input string - wantOK bool - wantHost string // expected u.Host on success - wantPath string + name string + input string + wantOK bool }{ - {"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"}, - // The "private 192" and "strips query" cases must use an - // RFC-1918 192.168/16 value — validateZcBaseURL only accepts - // loopback, RFC-1918, and link-local. RFC-5737 doc IPs - // (which we use as placeholders elsewhere) would be rejected - // here, so the test uses a generic-but-real 192.168 value. - {"private 192", "http://192.168.10.10:8200/zc", true, "192.168.10.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.10.10:8200/zc?foo=bar", true, "192.168.10.10:8200", "/zc"}, + {"loopback", "127.0.0.1", true}, + {"private 192", "192.168.10.10", true}, + {"private 10", "10.0.0.5", true}, + {"private 172", "172.16.0.1", true}, + {"link-local v4", "169.254.10.20", true}, + // IPv6 hosts are passed without brackets (brackets are URL syntax). + {"ipv6 loopback", "::1", true}, + {"ipv6 link-local", "fe80::1", true}, - {"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.0.2.10/zc", false, "", ""}, - {"file scheme rejected", "file:///etc/passwd", false, "", ""}, - {"empty host rejected", "http:///zc", false, "", ""}, - {"unparseable rejected", "::not a url::", false, "", ""}, + {"public IP rejected", "1.1.1.1", false}, + {"public ipv6 rejected", "2001:db8::1", false}, + {"hostname rejected", "myspeaker.local", false}, + {"plain hostname rejected", "speaker", false}, + {"empty host rejected", "", false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got, err := validateZcBaseURL(tc.input) + ip, err := validateZcHost(tc.input) if tc.wantOK { if err != nil { - t.Fatalf("validateZcBaseURL(%q) returned error %v, want success", tc.input, err) + t.Fatalf("validateZcHost(%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) + if ip == nil { + t.Fatalf("validateZcHost(%q) returned nil IP, want non-nil", tc.input) } } else if err == nil { - t.Errorf("validateZcBaseURL(%q) succeeded, want error", tc.input) + t.Errorf("validateZcHost(%q) succeeded, want error", tc.input) + } + }) + } +} + +func TestBuildZcBase(t *testing.T) { + cases := []struct { + name string + host string + port string + wantURL string + }{ + {"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"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ip := net.ParseIP(tc.host) + if ip == nil { + t.Fatalf("test setup: net.ParseIP(%q) returned nil", tc.host) + } + got := buildZcBase(ip, tc.port) + if got.String() != tc.wantURL { + t.Errorf("buildZcBase(%q, %q) = %q, want %q", tc.host, tc.port, got.String(), tc.wantURL) + } + if got.Path != "/zc" { + t.Errorf("Path = %q, want /zc", got.Path) + } + if got.RawQuery != "" { + t.Errorf("RawQuery = %q, want empty", got.RawQuery) } }) } @@ -399,7 +427,8 @@ func TestPushCredentials_AddUserNoOp(t *testing.T) { })) defer srv.Close() - err = PushCredentials(srv.URL+"/zc", "gesellix", "fresh-access-token") + host, port := srvHostPort(srv) + err = PushCredentials(host, port, "gesellix", "fresh-access-token") if !errors.Is(err, ErrAddUserNoOp) { t.Fatalf("PushCredentials: got %v, want ErrAddUserNoOp", err) } @@ -421,7 +450,8 @@ func TestPushCredentials_AddUserNoOpInSimplifiedPath(t *testing.T) { })) defer srv.Close() - err := PushCredentials(srv.URL+"/zc", "gesellix", "raw-access-token") + host, port := srvHostPort(srv) + err := PushCredentials(host, port, "gesellix", "raw-access-token") if !errors.Is(err, ErrAddUserNoOp) { t.Fatalf("PushCredentials (simplified path): got %v, want ErrAddUserNoOp", err) } @@ -468,7 +498,8 @@ func TestPushCredentials_AddUserRealError_NotMisclassified(t *testing.T) { })) defer srv.Close() - err := PushCredentials(srv.URL+"/zc", "gesellix", "fresh-access-token") + host, port := srvHostPort(srv) + err := PushCredentials(host, port, "gesellix", "fresh-access-token") if err == nil { t.Fatalf("expected error, got nil") }