ipn: reject advertised routes with non-address bits set (#18649)

* ipn: reject advertised routes with non-address bits set The config file path, EditPrefs local API, and App Connector API were accepting invalid subnet route prefixes with non-address bits set (e.g., 2a01:4f9:c010:c015::1/64 instead of 2a01:4f9:c010:c015::/64). All three paths now reject prefixes where prefix != prefix.Masked() with an error message indicating the expected masked form. Updates tailscale/corp#36738 Signed-off-by: Brendan Creane <bcreane@gmail.com> * address review comments Signed-off-by: Brendan Creane <bcreane@gmail.com> --------- Signed-off-by: Brendan Creane <bcreane@gmail.com>
4 weeks ago · ffa7df2789
parent 79f71beb24
commit ffa7df2789
4 changed files with 198 additions and 0 deletions
--- a/ipn/conf.go
+++ b/ipn/conf.go
@ -4,6 +4,8 @@
 package ipn

 import (
+	"errors"
+	"fmt"
 	"net/netip"

 	"tailscale.com/tailcfg"
@ -101,6 +103,15 @@ func (c *ConfigVAlpha) ToPrefs() (MaskedPrefs, error) {
 		mp.ExitNodeAllowLANAccessSet = true
 	}
 	if c.AdvertiseRoutes != nil {
+		var routeErrs []error
+		for _, route := range c.AdvertiseRoutes {
+			if route != route.Masked() {
+				routeErrs = append(routeErrs, fmt.Errorf("route %s has non-address bits set; expected %s", route, route.Masked()))
+			}
+		}
+		if err := errors.Join(routeErrs...); err != nil {
+			return mp, err
+		}
 		mp.AdvertiseRoutes = c.AdvertiseRoutes
 		mp.AdvertiseRoutesSet = true
 	}
--- a/ipn/conf_test.go
+++ b/ipn/conf_test.go
@ -0,0 +1,66 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+package ipn
+
+import (
+	"net/netip"
+	"testing"
+)
+
+// TestConfigVAlpha_ToPrefs_AdvertiseRoutes tests that ToPrefs validates routes
+// provided directly as netip.Prefix values (not parsed from JSON).
+func TestConfigVAlpha_ToPrefs_AdvertiseRoutes(t *testing.T) {
+	tests := []struct {
+		name    string
+		routes  []netip.Prefix
+		wantErr bool
+	}{
+		{
+			name: "valid_routes",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("10.0.0.0/24"),
+				netip.MustParsePrefix("2001:db8::/32"),
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid_ipv4_route",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("10.0.0.1/24"),
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid_ipv6_route",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("2a01:4f9:c010:c015::1/64"),
+			},
+			wantErr: true,
+		},
+		{
+			name: "mixed_valid_and_invalid",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("10.0.0.0/24"),
+				netip.MustParsePrefix("192.168.1.1/16"),
+				netip.MustParsePrefix("2001:db8::/32"),
+				netip.MustParsePrefix("2a01:4f9::1/64"),
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := ConfigVAlpha{
+				Version:         "alpha0",
+				AdvertiseRoutes: tt.routes,
+			}
+
+			_, err := cfg.ToPrefs()
+			if (err != nil) != tt.wantErr {
+				t.Errorf("cfg.ToPrefs() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@ -4152,6 +4152,9 @@ func (b *LocalBackend) checkPrefsLocked(p *ipn.Prefs) error {
 	if err := b.checkAutoUpdatePrefsLocked(p); err != nil {
 		errs = append(errs, err)
 	}
+	if err := checkAdvertiseRoutes(p); err != nil {
+		errs = append(errs, err)
+	}
 	return errors.Join(errs...)
 }

@ -4249,6 +4252,18 @@ func (b *LocalBackend) checkAutoUpdatePrefsLocked(p *ipn.Prefs) error {
 	return nil
 }

+// checkAdvertiseRoutes validates that all advertised routes have
+// properly masked prefixes (no non-address bits set).
+func checkAdvertiseRoutes(p *ipn.Prefs) error {
+	var errs []error
+	for _, route := range p.AdvertiseRoutes {
+		if route != route.Masked() {
+			errs = append(errs, fmt.Errorf("route %s has non-address bits set; expected %s", route, route.Masked()))
+		}
+	}
+	return errors.Join(errs...)
+}
+
 // SetUseExitNodeEnabled turns on or off the most recently selected exit node.
 //
 // On success, it returns the resulting prefs (or current prefs, in the case of no change).
@ -7260,6 +7275,9 @@ func (b *LocalBackend) AdvertiseRoute(ipps ...netip.Prefix) error {
 	var newRoutes []netip.Prefix

 	for _, ipp := range ipps {
+		if ipp != ipp.Masked() {
+			return fmt.Errorf("route %s has non-address bits set; expected %s", ipp, ipp.Masked())
+		}
 		if !allowedAutoRoute(ipp) {
 			continue
 		}
--- a/ipn/ipnlocal/local_test.go
+++ b/ipn/ipnlocal/local_test.go
@ -7574,3 +7574,106 @@ func TestRouteAllDisabled(t *testing.T) {
 		})
 	}
 }
+
+// TestAdvertiseRoute_InvalidPrefix tests that AdvertiseRoute rejects routes
+// with non-address bits set in the prefix.
+func TestAdvertiseRoute_InvalidPrefix(t *testing.T) {
+	b := newTestLocalBackend(t)
+
+	tests := []struct {
+		name    string
+		routes  []netip.Prefix
+		wantErr bool
+	}{
+		{
+			name: "valid_routes",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("10.0.0.0/24"),
+				netip.MustParsePrefix("2001:db8::/32"),
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid_ipv4_route",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("10.0.0.1/24"), // has non-address bits
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid_ipv6_route",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("2a01:4f9:c010:c015::1/64"), // has non-address bits
+			},
+			wantErr: true,
+		},
+		{
+			name: "mixed_valid_and_invalid",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("10.0.0.0/24"),    // valid
+				netip.MustParsePrefix("192.168.1.1/16"), // invalid - this should cause rejection
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := b.AdvertiseRoute(tt.routes...)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("AdvertiseRoute() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
+// TestEditPrefs_InvalidAdvertiseRoutes tests that EditPrefs (used by the local
+// API) rejects routes with non-address bits set.
+func TestEditPrefs_InvalidAdvertiseRoutes(t *testing.T) {
+	b := newTestLocalBackend(t)
+
+	tests := []struct {
+		name    string
+		routes  []netip.Prefix
+		wantErr bool
+	}{
+		{
+			name: "valid_routes",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("10.0.0.0/24"),
+				netip.MustParsePrefix("2001:db8::/32"),
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid_ipv4_route",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("10.0.0.1/24"), // has non-address bits
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid_ipv6_route",
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("fdf2:8bc1:6276:4f3f:dc33:c4ff:fe0b:120a/64"), // has non-address bits
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mp := &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					AdvertiseRoutes: tt.routes,
+				},
+				AdvertiseRoutesSet: true,
+			}
+
+			_, err := b.EditPrefs(mp)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("EditPrefs() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}