Merge pull request #32213 from yuwata/network-ndisc-redirect-fix-sender-address-verification

network/ndisc: fix sender address verification of Redirect message

src/libsystemd-network/sd-ndisc-redirect.c

@@ -55,14 +55,19 @@ int ndisc_redirect_parse(sd_ndisc *nd, sd_ndisc_redirect *rd) {
         rd->target_address = a->nd_rd_target;
         rd->destination_address = a->nd_rd_dst;
 
-        if (in6_addr_is_null(&rd->target_address) || in6_addr_is_multicast(&rd->target_address))
-                return log_ndisc_errno(nd, SYNTHETIC_ERRNO(EBADMSG),
-                                       "Received Redirect message with an invalid target address, ignoring datagram: %m");
-
+        /* RFC 4861 section 8.1
+         * The ICMP Destination Address field in the redirect message does not contain a multicast address. */
         if (in6_addr_is_null(&rd->destination_address) || in6_addr_is_multicast(&rd->destination_address))
                 return log_ndisc_errno(nd, SYNTHETIC_ERRNO(EBADMSG),
                                        "Received Redirect message with an invalid destination address, ignoring datagram: %m");
 
+        /* RFC 4861 section 8.1
+         * The ICMP Target Address is either a link-local address (when redirected to a router) or the same
+         * as the ICMP Destination Address (when redirected to the on-link destination). */
+        if (!in6_addr_is_link_local(&rd->target_address) && !in6_addr_equal(&rd->target_address, &rd->destination_address))
+                return log_ndisc_errno(nd, SYNTHETIC_ERRNO(EBADMSG),
+                                       "Received Redirect message with an invalid target address, ignoring datagram: %m");
+
         r = ndisc_parse_options(rd->packet, &rd->options);
         if (r < 0)
                 return log_ndisc_errno(nd, r, "Failed to parse NDisc options in Redirect message, ignoring datagram: %m");

src/network/networkd-ndisc.c

@@ -177,47 +177,19 @@ static void ndisc_set_route_priority(Link *link, Route *route) {
         }
 }
 
-static int ndisc_request_route(Route *route, Link *link, sd_ndisc_router *rt) {
-        struct in6_addr router;
-        uint8_t hop_limit = 0;
-        uint32_t mtu = 0;
-        bool is_new;
+static int ndisc_request_route(Route *route, Link *link) {
         int r;
 
         assert(route);
         assert(link);
         assert(link->manager);
         assert(link->network);
-        assert(rt);
-
-        r = sd_ndisc_router_get_sender_address(rt, &router);
-        if (r < 0)
-                return r;
-
-        if (link->network->ndisc_use_mtu) {
-                r = sd_ndisc_router_get_mtu(rt, &mtu);
-                if (r < 0 && r != -ENODATA)
-                        return log_link_warning_errno(link, r, "Failed to get MTU from RA: %m");
-        }
-
-        if (link->network->ndisc_use_hop_limit) {
-                r = sd_ndisc_router_get_hop_limit(rt, &hop_limit);
-                if (r < 0 && r != -ENODATA)
-                        return log_link_warning_errno(link, r, "Failed to get hop limit from RA: %m");
-        }
 
         route->source = NETWORK_CONFIG_SOURCE_NDISC;
-        route->provider.in6 = router;
+
         if (!route->table_set)
                 route->table = link_get_ndisc_route_table(link);
-        if (!route->protocol_set)
-                route->protocol = RTPROT_RA;
-        r = route_metric_set(&route->metric, RTAX_MTU, mtu);
-        if (r < 0)
-                return r;
-        r = route_metric_set(&route->metric, RTAX_HOPLIMIT, hop_limit);
-        if (r < 0)
-                return r;
+
         r = route_metric_set(&route->metric, RTAX_QUICKACK, link->network->ndisc_quickack);
         if (r < 0)
                 return r;
@@ -267,7 +239,7 @@ static int ndisc_request_route(Route *route, Link *link, sd_ndisc_router *rt) {
         route->pref = pref_original;
         ndisc_set_route_priority(link, route);
 
-        is_new = route_get(link->manager, route, NULL) < 0;
+        bool is_new = route_get(link->manager, route, NULL) < 0;
 
         r = link_request_route(link, route, &link->ndisc_messages, ndisc_route_handler);
         if (r < 0)
@@ -278,6 +250,23 @@ static int ndisc_request_route(Route *route, Link *link, sd_ndisc_router *rt) {
         return 0;
 }
 
+static int ndisc_request_router_route(Route *route, Link *link, sd_ndisc_router *rt) {
+        int r;
+
+        assert(route);
+        assert(link);
+        assert(rt);
+
+        r = sd_ndisc_router_get_sender_address(rt, &route->provider.in6);
+        if (r < 0)
+                return r;
+
+        if (!route->protocol_set)
+                route->protocol = RTPROT_RA;
+
+        return ndisc_request_route(route, link);
+}
+
 static int ndisc_remove_route(Route *route, Link *link) {
         int r;
 
@@ -420,43 +409,16 @@ static int ndisc_redirect_route_new(sd_ndisc_redirect *rd, Route **ret) {
         }
         route->dst.in6 = destination;
         route->dst_prefixlen = 128;
+        route->protocol = RTPROT_REDIRECT;
+
+        r = sd_ndisc_redirect_get_sender_address(rd, &route->provider.in6);
+        if (r < 0)
+                return r;
 
         *ret = TAKE_PTR(route);
         return 0;
 }
 
-static int ndisc_request_redirect_route(Link *link, sd_ndisc_redirect *rd) {
-        struct in6_addr router, sender;
-        int r;
-
-        assert(link);
-        assert(link->ndisc_default_router);
-        assert(rd);
-
-        r = sd_ndisc_router_get_sender_address(link->ndisc_default_router, &router);
-        if (r < 0)
-                return r;
-
-        r = sd_ndisc_redirect_get_sender_address(rd, &sender);
-        if (r < 0)
-                return r;
-
-        if (!in6_addr_equal(&sender, &router))
-                return 0;
-
-        _cleanup_(route_unrefp) Route *route = NULL;
-        r = ndisc_redirect_route_new(rd, &route);
-        if (r < 0)
-                return r;
-
-        route->protocol = RTPROT_REDIRECT;
-        route->protocol_set = true; /* To make ndisc_request_route() not override the protocol. */
-
-        /* Redirect message does not have the lifetime, let's use the lifetime of the default router, and
-         * update the lifetime of the redirect route every time when we receive RA. */
-        return ndisc_request_route(route, link, link->ndisc_default_router);
-}
-
 static int ndisc_remove_redirect_route(Link *link, sd_ndisc_redirect *rd) {
         _cleanup_(route_unrefp) Route *route = NULL;
         int r;
@@ -548,9 +510,58 @@ static int ndisc_redirect_drop_conflict(Link *link, sd_ndisc_redirect *rd) {
         return ndisc_remove_redirect_route(link, existing);
 }
 
-static int ndisc_redirect_handler(Link *link, sd_ndisc_redirect *rd) {
+static int ndisc_redirect_verify_sender(Link *link, sd_ndisc_redirect *rd) {
+        sd_ndisc_redirect *existing;
         struct in6_addr router, sender;
-        usec_t lifetime_usec, now_usec;
+        int r;
+
+        assert(link);
+        assert(rd);
+
+        /* RFC 4861 section 8.1
+        * The IP source address of the Redirect is the same as the current first-hop router for the specified
+        * ICMP Destination Address. */
+
+        r = sd_ndisc_redirect_get_sender_address(rd, &sender);
+        if (r < 0)
+                return r;
+
+        existing = set_get(link->ndisc_redirects, rd);
+        if (existing) {
+                struct in6_addr target, dest;
+
+                /* If we have received Redirect message for the host, the sender must be the previous target. */
+
+                r = sd_ndisc_redirect_get_target_address(existing, &target);
+                if (r < 0)
+                        return r;
+
+                if (in6_addr_equal(&sender, &target))
+                        return true;
+
+                /* If the existing redirect route is on-link, that is, the destination and target address are
+                 * equivalent, then also accept Redirect message from the current default router. This is not
+                 * mentioned by the RFC, but without this, we cannot update on-link redirect route. */
+                r = sd_ndisc_redirect_get_destination_address(existing, &dest);
+                if (r < 0)
+                        return r;
+
+                if (!in6_addr_equal(&dest, &target))
+                        return false;
+        }
+
+        if (!link->ndisc_default_router)
+                return false;
+
+        r = sd_ndisc_router_get_sender_address(link->ndisc_default_router, &router);
+        if (r < 0)
+                return r;
+
+        /* The sender must be the default router. */
+        return in6_addr_equal(&sender, &router);
+}
+
+static int ndisc_redirect_handler(Link *link, sd_ndisc_redirect *rd) {
         int r;
 
         assert(link);
@@ -560,97 +571,57 @@ static int ndisc_redirect_handler(Link *link, sd_ndisc_redirect *rd) {
         if (!link->network->ndisc_use_redirect)
                 return 0;
 
-        /* Ignore all Redirect messages from non-default router. */
-
-        if (!link->ndisc_default_router)
-                return 0;
-
-        r = sd_ndisc_router_get_lifetime_timestamp(link->ndisc_default_router, CLOCK_BOOTTIME, &lifetime_usec);
-        if (r < 0)
+        r = ndisc_redirect_verify_sender(link, rd);
+        if (r <= 0)
                 return r;
 
-        r = sd_event_now(link->manager->event, CLOCK_BOOTTIME, &now_usec);
-        if (r < 0)
-                return r;
-
-        if (lifetime_usec <= now_usec)
-                return 0; /* The default router is outdated. Ignore the redirect message. */
-
-        r = sd_ndisc_router_get_sender_address(link->ndisc_default_router, &router);
-        if (r < 0)
-                return r;
-
-        r = sd_ndisc_redirect_get_sender_address(rd, &sender);
-        if (r < 0)
-                return r;
-
-        if (!in6_addr_equal(&sender, &router))
-                return 0; /* The redirect message is sent from a non-default router. */
-
-        /* OK, the Redirect message is sent from the current default router. */
-
+        /* First, drop conflicting redirect route, if exists. */
         r = ndisc_redirect_drop_conflict(link, rd);
         if (r < 0)
                 return r;
 
+        /* Then, remember the received message. */
         r = set_ensure_put(&link->ndisc_redirects, &ndisc_redirect_hash_ops, rd);
         if (r < 0)
                 return r;
 
         sd_ndisc_redirect_ref(rd);
 
-        return ndisc_request_redirect_route(link, rd);
+        /* Finally, request the corresponding route. */
+        _cleanup_(route_unrefp) Route *route = NULL;
+        r = ndisc_redirect_route_new(rd, &route);
+        if (r < 0)
+                return r;
+
+        return ndisc_request_route(route, link);
 }
 
-static int ndisc_router_update_redirect(Link *link) {
+static int ndisc_drop_redirect(Link *link, const struct in6_addr *router) {
         int r, ret = 0;
 
         assert(link);
 
-        /* Reconfigure redirect routes to update their lifetime. */
-
         sd_ndisc_redirect *rd;
         SET_FOREACH(rd, link->ndisc_redirects) {
-                r = ndisc_request_redirect_route(link, rd);
-                if (r < 0)
-                        RET_GATHER(ret, log_link_warning_errno(link, r, "Failed to update lifetime of the Redirect route: %m"));
-        }
+                if (router) {
+                        struct in6_addr target;
 
-        return ret;
-}
-
-static int ndisc_drop_redirect(Link *link, const struct in6_addr *router, bool remove) {
-        int r;
-
-        assert(link);
-
-        /* If the router is purged, then drop the redirect routes configured with the Redirect message sent
-         * by the router. */
-
-        if (!router)
-                return 0;
-
-        sd_ndisc_redirect *rd;
-        SET_FOREACH(rd, link->ndisc_redirects) {
-                struct in6_addr sender;
-
-                r = sd_ndisc_redirect_get_sender_address(rd, &sender);
-                if (r < 0)
-                        return r;
-
-                if (!in6_addr_equal(&sender, router))
-                        continue;
-
-                if (remove) {
-                        r = ndisc_remove_redirect_route(link, rd);
+                        r = sd_ndisc_redirect_get_target_address(rd, &target);
                         if (r < 0)
                                 return r;
+
+                        if (!in6_addr_equal(&target, router))
+                                continue;
                 }
 
+                r = ndisc_remove_redirect_route(link, rd);
+                if (r < 0)
+                        RET_GATHER(ret, log_link_warning_errno(link, r, "Failed to remove redirect route, ignoring: %m"));
+
                 sd_ndisc_redirect_unref(set_remove(link->ndisc_redirects, rd));
         }
 
-        return 0;
+        return ret;
 }
 
 static int ndisc_update_redirect_sender(Link *link, const struct in6_addr *original_address, const struct in6_addr *current_address) {
@@ -774,7 +745,7 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) {
                 route->nexthop.gw.in6 = gateway;
                 route->lifetime_usec = lifetime_usec;
 
-                r = ndisc_request_route(route, link, rt);
+                r = ndisc_request_router_route(route, link, rt);
                 if (r < 0)
                         return log_link_warning_errno(link, r, "Could not request default route: %m");
         }
@@ -798,7 +769,7 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) {
                         route->pref = preference;
                 route->lifetime_usec = lifetime_usec;
 
-                r = ndisc_request_route(route, link, rt);
+                r = ndisc_request_router_route(route, link, rt);
                 if (r < 0)
                         return log_link_warning_errno(link, r, "Could not request gateway: %m");
         }
@@ -1253,7 +1224,7 @@ static int ndisc_router_process_onlink_prefix(Link *link, sd_ndisc_router *rt) {
         route->pref = preference;
         route->lifetime_usec = lifetime_usec;
 
-        r = ndisc_request_route(route, link, rt);
+        r = ndisc_request_router_route(route, link, rt);
         if (r < 0)
                 return log_link_warning_errno(link, r, "Could not request prefix route: %m");
 
@@ -1432,7 +1403,7 @@ static int ndisc_router_process_route(Link *link, sd_ndisc_router *rt) {
         route->lifetime_usec = lifetime_usec;
 
         if (lifetime_usec != 0) {
-                r = ndisc_request_route(route, link, rt);
+                r = ndisc_request_router_route(route, link, rt);
                 if (r < 0)
                         return log_link_warning_errno(link, r, "Could not request additional route: %m");
         } else {
@@ -1926,10 +1897,6 @@ static int ndisc_drop_outdated(Link *link, const struct in6_addr *router, usec_t
         if (r < 0)
                 RET_GATHER(ret, log_link_warning_errno(link, r, "Failed to drop outdated default router, ignoring: %m"));
 
-        r = ndisc_drop_redirect(link, router, /* remove = */ false);
-        if (r < 0)
-                RET_GATHER(ret, log_link_warning_errno(link, r, "Failed to drop outdated Redirect messages, ignoring: %m"));
-
         SET_FOREACH(route, link->manager->routes) {
                 if (route->source != NETWORK_CONFIG_SOURCE_NDISC)
                         continue;
@@ -1937,6 +1904,9 @@ static int ndisc_drop_outdated(Link *link, const struct in6_addr *router, usec_t
                 if (route->nexthop.ifindex != link->ifindex)
                         continue;
 
+                if (route->protocol == RTPROT_REDIRECT)
+                        continue; /* redirect route will be dropped by ndisc_drop_redirect(). */
+
                 if (route->lifetime_usec > timestamp_usec)
                         continue; /* the route is still valid */
 
@@ -2140,7 +2110,6 @@ static int ndisc_start_dhcp6_client(Link *link, sd_ndisc_router *rt) {
 static int ndisc_router_handler(Link *link, sd_ndisc_router *rt) {
         struct in6_addr router;
         usec_t timestamp_usec;
-        bool is_default;
         int r;
 
         assert(link);
@@ -2181,7 +2150,6 @@ static int ndisc_router_handler(Link *link, sd_ndisc_router *rt) {
         r = ndisc_remember_default_router(link, rt);
         if (r < 0)
                 return r;
-        is_default = r;
 
         r = ndisc_start_dhcp6_client(link, rt);
         if (r < 0)
@@ -2215,16 +2183,8 @@ static int ndisc_router_handler(Link *link, sd_ndisc_router *rt) {
         if (r < 0)
                 return r;
 
-        if (is_default) {
-                r = ndisc_router_update_redirect(link);
-                if (r < 0)
-                        return r;
-
-        } else if (sd_ndisc_router_get_lifetime(rt, NULL) <= 0) {
-                r = ndisc_drop_redirect(link, &router, /* remove = */ true);
-                if (r < 0)
-                        return r;
-        }
+        if (sd_ndisc_router_get_lifetime(rt, NULL) <= 0)
+                (void) ndisc_drop_redirect(link, &router);
 
         if (link->ndisc_messages == 0)
                 link->ndisc_configured = true;
@@ -2255,6 +2215,8 @@ static int ndisc_neighbor_handle_non_router_message(Link *link, sd_ndisc_neighbo
                 return r;
 
         (void) ndisc_drop_outdated(link, /* router = */ &address, /* timestamp_usec = */ USEC_INFINITY);
+        (void) ndisc_drop_redirect(link, &address);
+
         return 0;
 }
 
@@ -2535,6 +2497,7 @@ void ndisc_flush(Link *link) {
 
         /* Remove all addresses, routes, RDNSS, DNSSL, and Captive Portal entries, without exception. */
         (void) ndisc_drop_outdated(link, /* router = */ NULL, /* timestamp_usec = */ USEC_INFINITY);
+        (void) ndisc_drop_redirect(link, /* router = */ NULL);
 
         link->ndisc_rdnss = set_free(link->ndisc_rdnss);
         link->ndisc_dnssl = set_free(link->ndisc_dnssl);

test/test-network/systemd-networkd-tests.py

@@ -5551,17 +5551,17 @@ class NetworkdRATests(unittest.TestCase, Utilities):
 
         # Introduce two redirect routes.
         check_output(f'{test_ndisc_send} --interface veth-peer --type redirect --target-address 2002:da8:1:1:1a:2b:3c:4d --redirect-destination 2002:da8:1:1:1a:2b:3c:4d')
-        check_output(f'{test_ndisc_send} --interface veth-peer --type redirect --target-address 2002:da8:1::1 --redirect-destination 2002:da8:1:2:1a:2b:3c:4d')
-        self.wait_route('veth99', r'2002:da8:1:1:1a:2b:3c:4d proto redirect', ipv='-6', timeout_sec=10)
-        self.wait_route('veth99', r'2002:da8:1:2:1a:2b:3c:4d via 2002:da8:1::1 proto redirect', ipv='-6', timeout_sec=10)
+        check_output(f'{test_ndisc_send} --interface veth-peer --type redirect --target-address 2002:da8:1:2:1a:2b:3c:4d --redirect-destination 2002:da8:1:2:1a:2b:3c:4d')
+        self.wait_route('veth99', '2002:da8:1:1:1a:2b:3c:4d proto redirect', ipv='-6', timeout_sec=10)
+        self.wait_route('veth99', '2002:da8:1:2:1a:2b:3c:4d proto redirect', ipv='-6', timeout_sec=10)
 
         # Change the target address of the redirects.
-        check_output(f'{test_ndisc_send} --interface veth-peer --type redirect --target-address 2002:da8:1::2 --redirect-destination 2002:da8:1:1:1a:2b:3c:4d')
-        check_output(f'{test_ndisc_send} --interface veth-peer --type redirect --target-address 2002:da8:1::3 --redirect-destination 2002:da8:1:2:1a:2b:3c:4d')
-        self.wait_route_dropped('veth99', r'2002:da8:1:1:1a:2b:3c:4d proto redirect', ipv='-6', timeout_sec=10)
-        self.wait_route_dropped('veth99', r'2002:da8:1:2:1a:2b:3c:4d via 2002:da8:1::1 proto redirect', ipv='-6', timeout_sec=10)
-        self.wait_route('veth99', r'2002:da8:1:1:1a:2b:3c:4d via 2002:da8:1::2 proto redirect', ipv='-6', timeout_sec=10)
-        self.wait_route('veth99', r'2002:da8:1:2:1a:2b:3c:4d via 2002:da8:1::3 proto redirect', ipv='-6', timeout_sec=10)
+        check_output(f'{test_ndisc_send} --interface veth-peer --type redirect --target-address fe80::1 --redirect-destination 2002:da8:1:1:1a:2b:3c:4d')
+        check_output(f'{test_ndisc_send} --interface veth-peer --type redirect --target-address fe80::2 --redirect-destination 2002:da8:1:2:1a:2b:3c:4d')
+        self.wait_route_dropped('veth99', '2002:da8:1:1:1a:2b:3c:4d proto redirect', ipv='-6', timeout_sec=10)
+        self.wait_route_dropped('veth99', '2002:da8:1:2:1a:2b:3c:4d proto redirect', ipv='-6', timeout_sec=10)
+        self.wait_route('veth99', '2002:da8:1:1:1a:2b:3c:4d via fe80::1 proto redirect', ipv='-6', timeout_sec=10)
+        self.wait_route('veth99', '2002:da8:1:2:1a:2b:3c:4d via fe80::2 proto redirect', ipv='-6', timeout_sec=10)
 
         # Send Neighbor Advertisement without the router flag to announce the default router is not available anymore.
         # Then, verify that all redirect routes and the default route are dropped.
@@ -5569,9 +5569,13 @@ class NetworkdRATests(unittest.TestCase, Utilities):
         veth_peer_ipv6ll = re.search('fe80:[:0-9a-f]*', output).group()
         print(f'veth-peer IPv6LL address: {veth_peer_ipv6ll}')
         check_output(f'{test_ndisc_send} --interface veth-peer --type neighbor-advertisement --target-address {veth_peer_ipv6ll} --is-router no')
-        self.wait_route_dropped('veth99', 'proto redirect', ipv='-6', timeout_sec=10)
         self.wait_route_dropped('veth99', 'proto ra', ipv='-6', timeout_sec=10)
 
+        output = check_output('ip -6 route show dev veth99')
+        print(output)
+        self.assertIn('2002:da8:1:1:1a:2b:3c:4d via fe80::1 proto redirect', output)
+        self.assertIn('2002:da8:1:2:1a:2b:3c:4d via fe80::2 proto redirect', output)
+
     def check_ndisc_mtu(self, mtu):
         for _ in range(20):
             output = read_ipv6_sysctl_attr('veth99', 'mtu')
@@ -5668,15 +5672,6 @@ class NetworkdRATests(unittest.TestCase, Utilities):
         self.assertIn('2002:da8:1:0:b47e:7975:fc7a:7d6e', output)
         self.assertIn('2002:da8:2:0:f689:561a:8eda:7443', output)
 
-    def check_router_hop_limit(self, hop_limit):
-        self.wait_route('client', rf'default via fe80::1034:56ff:fe78:9a99 proto ra .* hoplimit {hop_limit}', ipv='-6', timeout_sec=10)
-
-        output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a99')
-        print(output)
-        self.assertIn(f'hoplimit {hop_limit}', output)
-
-        self.check_ipv6_sysctl_attr('client', 'hop_limit', f'{hop_limit}')
-
     def test_router_hop_limit(self):
         copy_network_unit('25-veth-client.netdev',
                           '25-veth-router.netdev',
@@ -5686,18 +5681,24 @@ class NetworkdRATests(unittest.TestCase, Utilities):
                           '25-veth-router-hop-limit.network',
                           '25-bridge99.network')
         start_networkd()
-        self.wait_online('client-p:enslaved',
+        self.wait_online('client:routable', 'client-p:enslaved',
                          'router:degraded', 'router-p:enslaved',
                          'bridge99:routable')
 
-        self.check_router_hop_limit(42)
+        self.check_ipv6_sysctl_attr('client', 'hop_limit', '42')
 
         with open(os.path.join(network_unit_dir, '25-veth-router-hop-limit.network'), mode='a', encoding='utf-8') as f:
             f.write('\n[IPv6SendRA]\nHopLimit=43\n')
 
         networkctl_reload()
 
-        self.check_router_hop_limit(43)
+        for _ in range(20):
+            output = read_ipv6_sysctl_attr('client', 'hop_limit')
+            if output == '43':
+                break
+            time.sleep(0.5)
+
+        self.check_ipv6_sysctl_attr('client', 'hop_limit', '43')
 
     def test_router_preference(self):
         copy_network_unit('25-veth-client.netdev',