From ffa7df2789b883b806be221161babfb4c3f53ab2 Mon Sep 17 00:00:00 2001 From: Brendan Creane Date: Fri, 20 Mar 2026 10:10:43 -0700 Subject: [PATCH] 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 * address review comments Signed-off-by: Brendan Creane --------- Signed-off-by: Brendan Creane --- ipn/conf.go | 11 ++++ ipn/conf_test.go | 66 ++++++++++++++++++++++++ ipn/ipnlocal/local.go | 18 +++++++ ipn/ipnlocal/local_test.go | 103 +++++++++++++++++++++++++++++++++++++ 4 files changed, 198 insertions(+) create mode 100644 ipn/conf_test.go diff --git a/ipn/conf.go b/ipn/conf.go index ef753a0b4..acedc44c1 100644 --- 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 } diff --git a/ipn/conf_test.go b/ipn/conf_test.go new file mode 100644 index 000000000..41b5c4506 --- /dev/null +++ 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) + } + }) + } +} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 915e6ddd7..1ff409b76 100644 --- 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 } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 544bc60ad..ab186cf68 100644 --- 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) + } + }) + } +}