resolved: address DVE-2018-0001

This is an updated version of #8608 with more restrictive logic. To
quite the original bug:

    Some captive portals, lie and do not respond with the captive portal
    IP address, if the query is with EDNS0 enabled and D0 bit set to
    zero. Thus retry "secure" domain name look ups with less secure
    methods, upon NXDOMAIN.

https://github.com/dns-violations/dns-violations/blob/master/2018/DVE-2018-0001.md

Yes, this fix sucks hard, but I guess this is what we need to do to make
sure resolved works IRL.

Heavily based on the original patch from Dimitri John Ledkov, and I
copied the commentary verbatim.

Replaces: #8608

.github/workflows/build_test.sh

@@ -12,6 +12,7 @@ ARGS=(
     "--optimization=s"
     "--optimization=3 -Db_lto=true"
     "--optimization=3 -Db_lto=false"
+    "--optimization=3 -Ddns-over-tls=openssl"
     "-Db_ndebug=true"
 )
 PACKAGES=(

man/systemd.network.xml

@@ -1321,15 +1321,25 @@ IPv6Token=prefixstable:2002:da8:1::</programlisting></para>
 
       <variablelist class='network-directives'>
         <varlistentry>
-          <term><varname>Gateway=</varname></term>
+          <term><varname>Id=</varname></term>
           <listitem>
-            <para>As in the [Network] section. This is mandatory.</para>
+            <para>The id of the next hop. Takes an unsigned integer in the range 1…4294967295. If left
+            unspecified, then automatically chosen by kernel.</para>
           </listitem>
         </varlistentry>
         <varlistentry>
-          <term><varname>Id=</varname></term>
+          <term><varname>Gateway=</varname></term>
           <listitem>
-            <para>The id of the nexthop (an unsigned integer). If unspecified or '0' then automatically chosen by kernel.</para>
+            <para>As in the [Network] section.</para>
+          </listitem>
+        </varlistentry>
+        <varlistentry>
+          <term><varname>Family=</varname></term>
+          <listitem>
+            <para>Takes one of the special values <literal>ipv4</literal> or <literal>ipv6</literal>.
+            By default, the family is determined by the address specified in
+            <varname>Gateway=</varname>. If <varname>Gateway=</varname> is not specified, then defaults
+            to <literal>ipv4</literal>.</para>
           </listitem>
         </varlistentry>
       </variablelist>

src/network/networkd-network-gperf.gperf

@@ -186,6 +186,7 @@ Route.TTLPropagate,                          config_parse_route_boolean,
 Route.MultiPathRoute,                        config_parse_multipath_route,                             0,                             0
 NextHop.Id,                                  config_parse_nexthop_id,                                  0,                             0
 NextHop.Gateway,                             config_parse_nexthop_gateway,                             0,                             0
+NextHop.Family,                              config_parse_nexthop_family,                              0,                             0
 DHCPv4.ClientIdentifier,                     config_parse_dhcp_client_identifier,                      0,                             offsetof(Network, dhcp_client_identifier)
 DHCPv4.UseDNS,                               config_parse_dhcp_use_dns,                                0,                             0
 DHCPv4.RoutesToDNS,                          config_parse_bool,                                        0,                             offsetof(Network, dhcp_routes_to_dns)

src/network/networkd-nexthop.c

@@ -269,9 +269,11 @@ static int nexthop_configure(NextHop *nexthop, Link *link) {
         if (r < 0)
                 return log_link_error_errno(link, r, "Could not create RTM_NEWNEXTHOP message: %m");
 
+        if (nexthop->id > 0) {
                 r = sd_netlink_message_append_u32(req, NHA_ID, nexthop->id);
                 if (r < 0)
                         return log_link_error_errno(link, r, "Could not append NHA_ID attribute: %m");
+        }
 
         r = sd_netlink_message_append_u32(req, NHA_OIF, link->ifindex);
         if (r < 0)
@@ -440,8 +442,9 @@ static int nexthop_section_verify(NextHop *nh) {
         if (section_is_invalid(nh->section))
                 return -EINVAL;
 
-        if (in_addr_is_null(nh->family, &nh->gw) < 0)
-                return -EINVAL;
+        if (nh->family == AF_UNSPEC)
+                /* When no Gateway= is specified, assume IPv4. */
+                nh->family = AF_INET;
 
         return 0;
 }
@@ -470,6 +473,7 @@ int config_parse_nexthop_id(
 
         _cleanup_(nexthop_free_or_set_invalidp) NextHop *n = NULL;
         Network *network = userdata;
+        uint32_t id;
         int r;
 
         assert(filename);
@@ -482,13 +486,25 @@ int config_parse_nexthop_id(
         if (r < 0)
                 return log_oom();
 
-        r = safe_atou32(rvalue, &n->id);
+        if (isempty(rvalue)) {
+                n->id = 0;
+                TAKE_PTR(n);
+                return 0;
+        }
+
+        r = safe_atou32(rvalue, &id);
         if (r < 0) {
                 log_syntax(unit, LOG_WARNING, filename, line, r,
                            "Could not parse nexthop id \"%s\", ignoring assignment: %m", rvalue);
                 return 0;
         }
+        if (id == 0) {
+                log_syntax(unit, LOG_WARNING, filename, line, 0,
+                           "Invalid nexthop id \"%s\", ignoring assignment: %m", rvalue);
+                return 0;
+        }
 
+        n->id = id;
         TAKE_PTR(n);
         return 0;
 }
@@ -519,6 +535,14 @@ int config_parse_nexthop_gateway(
         if (r < 0)
                 return log_oom();
 
+        if (isempty(rvalue)) {
+                n->family = AF_UNSPEC;
+                n->gw = IN_ADDR_NULL;
+
+                TAKE_PTR(n);
+                return 0;
+        }
+
         r = in_addr_from_string_auto(rvalue, &n->family, &n->gw);
         if (r < 0) {
                 log_syntax(unit, LOG_WARNING, filename, line, r,
@@ -529,3 +553,69 @@ int config_parse_nexthop_gateway(
         TAKE_PTR(n);
         return 0;
 }
+
+int config_parse_nexthop_family(
+                const char *unit,
+                const char *filename,
+                unsigned line,
+                const char *section,
+                unsigned section_line,
+                const char *lvalue,
+                int ltype,
+                const char *rvalue,
+                void *data,
+                void *userdata) {
+
+        _cleanup_(nexthop_free_or_set_invalidp) NextHop *n = NULL;
+        Network *network = userdata;
+        AddressFamily a;
+        int r;
+
+        assert(filename);
+        assert(section);
+        assert(lvalue);
+        assert(rvalue);
+        assert(data);
+
+        r = nexthop_new_static(network, filename, section_line, &n);
+        if (r < 0)
+                return log_oom();
+
+        if (isempty(rvalue) &&
+            in_addr_is_null(n->family, &n->gw) != 0) {
+                /* Accept an empty string only when Gateway= is null or not specified. */
+                n->family = AF_UNSPEC;
+                TAKE_PTR(n);
+                return 0;
+        }
+
+        a = nexthop_address_family_from_string(rvalue);
+        if (a < 0) {
+                log_syntax(unit, LOG_WARNING, filename, line, 0,
+                           "Invalid %s='%s', ignoring assignment: %m", lvalue, rvalue);
+                return 0;
+        }
+
+        if (in_addr_is_null(n->family, &n->gw) == 0 &&
+            ((a == ADDRESS_FAMILY_IPV4 && n->family == AF_INET6) ||
+             (a == ADDRESS_FAMILY_IPV6 && n->family == AF_INET))) {
+                log_syntax(unit, LOG_WARNING, filename, line, 0,
+                           "Specified family '%s' conflicts with the family of the previously specified Gateway=, "
+                           "ignoring assignment.", rvalue);
+                return 0;
+        }
+
+        switch(a) {
+        case ADDRESS_FAMILY_IPV4:
+                n->family = AF_INET;
+                break;
+        case ADDRESS_FAMILY_IPV6:
+                n->family = AF_INET6;
+                break;
+        default:
+                assert_not_reached("Invalid family.");
+        }
+
+        TAKE_PTR(n);
+        return 0;
+}

src/network/networkd-nexthop.h

@@ -39,3 +39,4 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message,
 
 CONFIG_PARSER_PROTOTYPE(config_parse_nexthop_id);
 CONFIG_PARSER_PROTOTYPE(config_parse_nexthop_gateway);
+CONFIG_PARSER_PROTOTYPE(config_parse_nexthop_family);

src/network/networkd-route.c

@@ -623,7 +623,7 @@ static void log_route_debug(const Route *route, const char *str, const Link *lin
                 }
                 if (!in_addr_is_null(route->family, &route->src))
                         (void) in_addr_to_string(route->family, &route->src, &src);
-                if (!in_addr_is_null(route->gw_family, &route->gw))
+                if (in_addr_is_null(route->gw_family, &route->gw) == 0)
                         (void) in_addr_to_string(route->gw_family, &route->gw, &gw);
                 if (!in_addr_is_null(route->family, &route->prefsrc))
                         (void) in_addr_to_string(route->family, &route->prefsrc, &prefsrc);
@@ -1260,7 +1260,7 @@ int link_set_routes(Link *link) {
                         if (rt->gateway_from_dhcp_or_ra)
                                 continue;
 
-                        if ((in_addr_is_null(rt->gw_family, &rt->gw) && ordered_set_isempty(rt->multipath_routes)) != (phase == PHASE_NON_GATEWAY))
+                        if ((in_addr_is_null(rt->gw_family, &rt->gw) != 0 && ordered_set_isempty(rt->multipath_routes)) != (phase == PHASE_NON_GATEWAY))
                                 continue;
 
                         r = route_configure(rt, link, route_handler, NULL);
@@ -2573,6 +2573,17 @@ static int route_section_verify(Route *route, Network *network) {
                 route->gateway_onlink = true;
         }
 
+        if (route->family == AF_INET6) {
+                MultipathRoute *m;
+
+                ORDERED_SET_FOREACH(m, route->multipath_routes)
+                        if (m->gateway.family == AF_INET)
+                                return log_warning_errno(SYNTHETIC_ERRNO(EINVAL),
+                                                         "%s: IPv4 multipath route is specified for IPv6 route. "
+                                                         "Ignoring [Route] section from line %u.",
+                                                         route->section->filename, route->section->line);
+        }
+
         return 0;
 }
 

src/network/networkd-util.c

@@ -21,6 +21,11 @@ static const char* const routing_policy_rule_address_family_table[_ADDRESS_FAMIL
         [ADDRESS_FAMILY_IPV6]          = "ipv6",
 };
 
+static const char* const nexthop_address_family_table[_ADDRESS_FAMILY_MAX] = {
+        [ADDRESS_FAMILY_IPV4]          = "ipv4",
+        [ADDRESS_FAMILY_IPV6]          = "ipv6",
+};
+
 static const char* const duplicate_address_detection_address_family_table[_ADDRESS_FAMILY_MAX] = {
         [ADDRESS_FAMILY_NO]            = "none",
         [ADDRESS_FAMILY_YES]           = "both",
@@ -55,6 +60,7 @@ AddressFamily link_local_address_family_from_string(const char *s) {
 }
 
 DEFINE_STRING_TABLE_LOOKUP(routing_policy_rule_address_family, AddressFamily);
+DEFINE_STRING_TABLE_LOOKUP(nexthop_address_family, AddressFamily);
 DEFINE_STRING_TABLE_LOOKUP(duplicate_address_detection_address_family, AddressFamily);
 DEFINE_CONFIG_PARSE_ENUM(config_parse_link_local_address_family, link_local_address_family,
                          AddressFamily, "Failed to parse option");

src/network/networkd-util.h

@@ -38,6 +38,9 @@ AddressFamily link_local_address_family_from_string(const char *s) _pure_;
 const char *routing_policy_rule_address_family_to_string(AddressFamily b) _const_;
 AddressFamily routing_policy_rule_address_family_from_string(const char *s) _pure_;
 
+const char *nexthop_address_family_to_string(AddressFamily b) _const_;
+AddressFamily nexthop_address_family_from_string(const char *s) _pure_;
+
 const char *duplicate_address_detection_address_family_to_string(AddressFamily b) _const_;
 AddressFamily duplicate_address_detection_address_family_from_string(const char *s) _pure_;
 

src/resolve/resolved-dns-transaction.c

@@ -280,7 +280,8 @@ int dns_transaction_new(
                 .query_flags = query_flags,
                 .bypass = dns_packet_ref(bypass),
                 .current_feature_level = _DNS_SERVER_FEATURE_LEVEL_INVALID,
-                .clamp_feature_level = _DNS_SERVER_FEATURE_LEVEL_INVALID,
+                .clamp_feature_level_servfail = _DNS_SERVER_FEATURE_LEVEL_INVALID,
+                .clamp_feature_level_nxdomain = _DNS_SERVER_FEATURE_LEVEL_INVALID,
                 .id = pick_new_id(s->manager),
         };
 
@@ -472,15 +473,20 @@ static int dns_transaction_pick_server(DnsTransaction *t) {
 
         /* If we changed the server invalidate the feature level clamping, as the new server might have completely
          * different properties. */
-        if (server != t->server)
-                t->clamp_feature_level = _DNS_SERVER_FEATURE_LEVEL_INVALID;
+        if (server != t->server) {
+                t->clamp_feature_level_servfail = _DNS_SERVER_FEATURE_LEVEL_INVALID;
+                t->clamp_feature_level_nxdomain = _DNS_SERVER_FEATURE_LEVEL_INVALID;
+        }
 
         t->current_feature_level = dns_server_possible_feature_level(server);
 
         /* Clamp the feature level if that is requested. */
-        if (t->clamp_feature_level != _DNS_SERVER_FEATURE_LEVEL_INVALID &&
-            t->current_feature_level > t->clamp_feature_level)
-                t->current_feature_level = t->clamp_feature_level;
+        if (t->clamp_feature_level_servfail != _DNS_SERVER_FEATURE_LEVEL_INVALID &&
+            t->current_feature_level > t->clamp_feature_level_servfail)
+                t->current_feature_level = t->clamp_feature_level_servfail;
+        if (t->clamp_feature_level_nxdomain != _DNS_SERVER_FEATURE_LEVEL_INVALID &&
+            t->current_feature_level > t->clamp_feature_level_nxdomain)
+                t->current_feature_level = t->clamp_feature_level_nxdomain;
 
         log_debug("Using feature level %s for transaction %u.", dns_server_feature_level_to_string(t->current_feature_level), t->id);
 
@@ -1124,19 +1130,19 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p, bool encrypt
                         /* Reduce this feature level by one and try again. */
                         switch (t->current_feature_level) {
                         case DNS_SERVER_FEATURE_LEVEL_TLS_DO:
-                                t->clamp_feature_level = DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN;
+                                t->clamp_feature_level_servfail = DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN;
                                 break;
                         case DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN + 1:
                                 /* Skip plain TLS when TLS is not supported */
-                                t->clamp_feature_level = DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN - 1;
+                                t->clamp_feature_level_servfail = DNS_SERVER_FEATURE_LEVEL_TLS_PLAIN - 1;
                                 break;
                         default:
-                                t->clamp_feature_level = t->current_feature_level - 1;
+                                t->clamp_feature_level_servfail = t->current_feature_level - 1;
                         }
 
                         log_debug("Server returned error %s, retrying transaction with reduced feature level %s.",
                                   dns_rcode_to_string(DNS_PACKET_RCODE(p)),
-                                  dns_server_feature_level_to_string(t->clamp_feature_level));
+                                  dns_server_feature_level_to_string(t->clamp_feature_level_servfail));
 
                         dns_transaction_retry(t, false /* use the same server */);
                         return;
@@ -1222,13 +1228,52 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p, bool encrypt
                 return;
         }
 
+        if (t->scope->protocol == DNS_PROTOCOL_DNS &&
+            !t->bypass &&
+            DNS_PACKET_RCODE(p) == DNS_RCODE_NXDOMAIN &&
+            p->opt && !DNS_PACKET_DO(p) &&
+            DNS_SERVER_FEATURE_LEVEL_IS_EDNS0(t->current_feature_level) &&
+            DNS_SERVER_FEATURE_LEVEL_IS_UDP(t->current_feature_level) &&
+            t->scope->dnssec_mode != DNSSEC_YES) {
+
+                /* Some captive portals are special in that the Aruba/Datavalet hardware will miss
+                 * replacing the packets with the local server IP to point to the authenticated side
+                 * of the network if EDNS0 is enabled. Instead they return NXDOMAIN, with DO bit set
+                 * to zero... nothing to see here, yet respond with the captive portal IP, when using
+                 * the more simple UDP level.
+                 *
+                 * Common portal names that fail like so are:
+                 *     secure.datavalet.io
+                 *     securelogin.arubanetworks.com
+                 *     securelogin.networks.mycompany.com
+                 *
+                 * Thus retry NXDOMAIN RCODES with a lower feature level.
+                 *
+                 * Do not lower the server's tracked feature level, as the captive portal should not
+                 * be lying for the wider internet (e.g. _other_ queries were observed fine with
+                 * EDNS0 on these networks, post auth), i.e. let's just lower the level transaction's
+                 * feature level.
+                 *
+                 * This is reported as https://github.com/dns-violations/dns-violations/blob/master/2018/DVE-2018-0001.md
+                 */
+
+                t->clamp_feature_level_nxdomain = DNS_SERVER_FEATURE_LEVEL_UDP;
+
+                log_debug("Server returned error %s in EDNS0 mode, retrying transaction with reduced feature level %s (DVE-2018-0001 mitigation)",
+                          dns_rcode_to_string(DNS_PACKET_RCODE(p)),
+                          dns_server_feature_level_to_string(t->clamp_feature_level_nxdomain));
+
+                dns_transaction_retry(t, false /* use the same server */);
+                return;
+        }
+
         if (t->server) {
                 /* Report that we successfully received a valid packet with a good rcode after we initially got a bad
                  * rcode and subsequently downgraded the protocol */
 
                 if (IN_SET(DNS_PACKET_RCODE(p), DNS_RCODE_SUCCESS, DNS_RCODE_NXDOMAIN) &&
-                    t->clamp_feature_level != _DNS_SERVER_FEATURE_LEVEL_INVALID)
-                        dns_server_packet_rcode_downgrade(t->server, t->clamp_feature_level);
+                    t->clamp_feature_level_servfail != _DNS_SERVER_FEATURE_LEVEL_INVALID)
+                        dns_server_packet_rcode_downgrade(t->server, t->clamp_feature_level_servfail);
 
                 /* Report that the OPT RR was missing */
                 if (!p->opt)

src/resolve/resolved-dns-transaction.h

@@ -108,8 +108,11 @@ struct DnsTransaction {
         /* The features of the DNS server at time of transaction start */
         DnsServerFeatureLevel current_feature_level;
 
-        /* If we got SERVFAIL back, we retry the lookup, using a lower feature level than we used before. */
-        DnsServerFeatureLevel clamp_feature_level;
+        /* If we got SERVFAIL back, we retry the lookup, using a lower feature level than we used
+         * before. Similar, if we get NXDOMAIN in pure EDNS0 mode, we check in EDNS0-less mode before giving
+         * up (as mitigation for DVE-2018-0001). */
+        DnsServerFeatureLevel clamp_feature_level_servfail;
+        DnsServerFeatureLevel clamp_feature_level_nxdomain;
 
         /* Query candidates this transaction is referenced by and that
          * shall be notified about this specific transaction

test/fuzz/fuzz-network-parser/directives.network

@@ -349,6 +349,7 @@ SendVendorOption=
 [NextHop]
 Id=
 Gateway=
+Family=
 [QDisc]
 Parent=
 Handle=

test/test-network/conf/25-nexthop.network

@@ -3,9 +3,25 @@ Name=veth99
 
 [Network]
 IPv6AcceptRA=no
+Address=2001:1234:5:8f63::1/120
 Address=192.168.5.10/24
 Gateway=192.168.5.1
 
 [NextHop]
 Id=1
 Gateway=192.168.5.1
+
+[NextHop]
+Id=2
+Gateway=2001:1234:5:8f63::2
+
+[NextHop]
+Id=3
+Family=ipv6
+
+[NextHop]
+Id=4
+Family=ipv4
+
+[NextHop]
+Gateway=192.168.5.2

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

@@ -2799,7 +2799,11 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities):
 
         output = check_output('ip nexthop list dev veth99')
         print(output)
-        self.assertRegex(output, '192.168.5.1')
+        self.assertIn('id 1 via 192.168.5.1 dev veth99', output)
+        self.assertIn('id 2 via 2001:1234:5:8f63::2 dev veth99', output)
+        self.assertIn('id 3 dev veth99', output)
+        self.assertIn('id 4 dev veth99', output)
+        self.assertRegex(output, r'id [0-9]* via 192.168.5.2 dev veth99')
 
     def test_qdisc(self):
         copy_unit_to_networkd_unit_path('25-qdisc-clsact-and-htb.network', '12-dummy.netdev',