From 0957258f8476d18daefdbc8f403cd64118157c5c Mon Sep 17 00:00:00 2001 From: James Tucker Date: Tue, 19 Dec 2023 09:33:38 -0800 Subject: [PATCH] appc,ipn: prevent undesirable route advertisements Individual route advertisements that are covered by existing routes are no longer advertised. If an upstream returns 0.0.0.0, 127.x, and other common unwanted addresses those are also rejected. Updates #16425 Signed-off-by: James Tucker --- appc/appconnector.go | 4 +--- ipn/ipnlocal/local.go | 48 ++++++++++++++++++++++++++++++++++---- ipn/ipnlocal/local_test.go | 23 +++++++++++++++--- 3 files changed, 64 insertions(+), 11 deletions(-) diff --git a/appc/appconnector.go b/appc/appconnector.go index 9b8398811..d60d0198c 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -206,9 +206,8 @@ func (e *AppConnector) ObserveDNSResponse(res []byte) { if slices.Contains(addrs, addr) { continue } - // TODO(raggi): check for existing prefixes if err := e.routeAdvertiser.AdvertiseRoute(netip.PrefixFrom(addr, addr.BitLen())); err != nil { - e.logf("failed to advertise route for %v: %v", addr, err) + e.logf("failed to advertise route for %s: %v: %v", domain, addr, err) continue } e.logf("[v2] advertised route for %v: %v", domain, addr) @@ -217,5 +216,4 @@ func (e *AppConnector) ObserveDNSResponse(res []byte) { e.domains[domain] = append(addrs, addr) e.mu.Unlock() } - } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 1c81ba045..4f72a305e 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -774,7 +774,7 @@ func (b *LocalBackend) UpdateStatus(sb *ipnstate.StatusBuilder) { } if !prefs.ExitNodeID().IsZero() { if exitPeer, ok := b.netMap.PeerWithStableID(prefs.ExitNodeID()); ok { - var online = false + online := false if v := exitPeer.Online(); v != nil { online = *v } @@ -855,7 +855,7 @@ func (b *LocalBackend) populatePeerStatusLocked(sb *ipnstate.StatusBuilder) { if p.LastSeen() != nil { lastSeen = *p.LastSeen() } - var tailscaleIPs = make([]netip.Addr, 0, p.Addresses().Len()) + tailscaleIPs := make([]netip.Addr, 0, p.Addresses().Len()) for i := range p.Addresses().LenIter() { addr := p.Addresses().At(i) if addr.IsSingleIP() && tsaddr.IsTailscaleIP(addr.Addr()) { @@ -4200,7 +4200,6 @@ func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State) { default: b.logf("[unexpected] unknown newState %#v", newState) } - } // hasNodeKey reports whether a non-zero node key is present in the current @@ -5781,12 +5780,21 @@ func (b *LocalBackend) ObserveDNSResponse(res []byte) { appConnector.ObserveDNSResponse(res) } +// ErrDisallowedAutoRoute is returned by AdvertiseRoute when a route that is not allowed is requested. +var ErrDisallowedAutoRoute = errors.New("route is not allowed") + // AdvertiseRoute implements the appc.RouteAdvertiser interface. It sets a new // route advertisement if one is not already present in the existing routes. +// If the route is disallowed, ErrDisallowedAutoRoute is returned. func (b *LocalBackend) AdvertiseRoute(ipp netip.Prefix) error { + if !allowedAutoRoute(ipp) { + return ErrDisallowedAutoRoute + } currentRoutes := b.Prefs().AdvertiseRoutes() - // TODO(raggi): check if the new route is a subset of an existing route. - if currentRoutes.ContainsFunc(func(r netip.Prefix) bool { return r == ipp }) { + if currentRoutes.ContainsFunc(func(r netip.Prefix) bool { + // TODO(raggi): add support for subset checks and avoid subset route creations. + return ipp.IsSingleIP() && r.Contains(ipp.Addr()) || r == ipp + }) { return nil } routes := append(currentRoutes.AsSlice(), ipp) @@ -5799,6 +5807,36 @@ func (b *LocalBackend) AdvertiseRoute(ipp netip.Prefix) error { return err } +var ( + disallowedAddrs = []netip.Addr{ + netip.MustParseAddr("::1"), + netip.MustParseAddr("::"), + netip.MustParseAddr("0.0.0.0"), + } + disallowedRanges = []netip.Prefix{ + netip.MustParsePrefix("127.0.0.0/8"), + netip.MustParsePrefix("224.0.0.0/4"), + netip.MustParsePrefix("ff00::/8"), + } +) + +// allowedAutoRoute determines if the route being added via AdvertiseRoute (the app connector featuge) should be allowed. +func allowedAutoRoute(ipp netip.Prefix) bool { + // Note: blocking the addrs for globals, not solely the prefixes. + for _, addr := range disallowedAddrs { + if ipp.Addr() == addr { + return false + } + } + for _, pfx := range disallowedRanges { + if pfx.Overlaps(ipp) { + return false + } + } + // TODO(raggi): exclude tailscale service IPs and so on as well. + return true +} + // mayDeref dereferences p if non-nil, otherwise it returns the zero value. func mayDeref[T any](p *T) (v T) { if p == nil { diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 71d08822b..17d374866 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -265,7 +265,6 @@ func TestPeerRoutes(t *testing.T) { } }) } - } func TestPeerAPIBase(t *testing.T) { @@ -700,7 +699,6 @@ func TestPacketFilterPermitsUnlockedNodes(t *testing.T) { } }) } - } func TestStatusWithoutPeers(t *testing.T) { @@ -1173,6 +1171,26 @@ func TestRouteAdvertiser(t *testing.T) { } } +func TestRouterAdvertiserIgnoresContainedRoutes(t *testing.T) { + b := newTestBackend(t) + testPrefix := netip.MustParsePrefix("192.0.0.0/24") + ra := appc.RouteAdvertiser(b) + must.Do(ra.AdvertiseRoute(testPrefix)) + + routes := b.Prefs().AdvertiseRoutes() + if routes.Len() != 1 || routes.At(0) != testPrefix { + t.Fatalf("got routes %v, want %v", routes, []netip.Prefix{testPrefix}) + } + + must.Do(ra.AdvertiseRoute(netip.MustParsePrefix("192.0.0.8/32"))) + + // the above /32 is not added as it is contained within the /24 + routes = b.Prefs().AdvertiseRoutes() + if routes.Len() != 1 || routes.At(0) != testPrefix { + t.Fatalf("got routes %v, want %v", routes, []netip.Prefix{testPrefix}) + } +} + func TestObserveDNSResponse(t *testing.T) { b := newTestBackend(t) @@ -1886,7 +1904,6 @@ func TestApplySysPolicy(t *testing.T) { }) t.Run("set prefs", func(t *testing.T) { - b := newTestBackend(t) b.SetPrefs(tt.prefs.Clone()) if !b.Prefs().Equals(tt.wantPrefs.View()) {