edns-client-subnet: FORMERR if not IPv[46] family

Previously, if the client's option had enough bytes for the
IPv[46] source and scope masks, and the source mask field was zero
with no address data, and the family was not IPv4 or IPv6, we
ignored the data for lookup purposes, but returned (with fixed
scope mask zero) a reflection of the sent arbitrary family value
plus two bytes for the zero source and scope masks.

RFC 7871 has some confusing language on this issue, and I'm
switching interpretations here.  Either way I don't think using
non IPv[46] families is probably very useful in practice on the
Internet, but our new interpretation is to FORMERR any family that
isn't IPv4 or IPv6.

---

The ambiguity stems from these bits of the standard:

Section 7.1.2 Stub Resolvers says:
> [...] This is for interoperability; at least one major
> authoritative server will ignore the option if FAMILY is not 1
> or 2, even though it is irrelevant if there are no ADDRESS bits.

The implication is the "one major authoritative server" is doing
something standards-questionable in its described action, and I
previously took this sentence to imply that authservers should
accept the option and reflect an unknown family value so long as
the source mask specified zero address bits.

However, Section 7.2.1 Authoritative Nameserver says:
> A query with a wrongly formatted option (e.g., an unknown
> FAMILY) MUST be rejected and a FORMERR response MUST be returned
> to the sender [...]

Which, at least in its example, seems to state that an unknown
family (which for us is anything but IPv4 or IPv6) requires a
FORMERR response regardless of source mask or the rest of the
contents.

The clencher on this issue, IMHO, is that the description of the
wire format in Section 6 explicitly says:

> The format of the address part depends on the value of FAMILY.
> This document only defines the format for FAMILY 1 (IPv4) and
> FAMILY 2 (IPv6), which are as follows:

... with the fields up through FAMILY described above that
language, and the definition of the source mask, scope mask, and
address fields below that language.  Therefore, I believe the
standards document is saying that in the case of other family
values (not IPv[46]), there is no declared interpretation of what
the rest of the data even means, and therefore it wouldn't be
legitimate to even look at the source mask field to see that it's
zero and decide to reflect the family value and/or send back zero
source and scope mask fields that are only defined for IPv[46].

This is a little bit contradictary to some interpretations of
7.1.2's interop example, in my opinion.  Perhaps 7.1.2 really
meant "zero bytes of data beyond the family value" when it said
"no ADDRESS bits", but then that interpretation wouldn't be very
useful for interop with a future new RFC that did define data for
a new family type, unless that RFC defined that the new address
family sent no address data, but could be used to select a
different response address based on family alone.

Therefore, I think the sanest approach is probably to just FORMERR
any unknown family for now, unless/until some future RFC clarifies
this point and/or defines another family.  If nothing else, it's
the least-surprising interpretation of the language of the section
that's acutally about authoritative server responses.

src/dnspacket.c

@@ -288,26 +288,23 @@ static bool handle_edns_client_subnet(dnsp_ctx_t* ctx, unsigned opt_len, const u
             break;
         }
 
-        // Validate family iff src_mask is non-zero, and validate non-zero
-        // src_mask as appropriate for the know families.
-        if (src_mask) {
-            if (family == 1U) { // IPv4
-                if (src_mask > 32U) {
-                    log_devdebug("edns_client_subnet: invalid src_mask of %u for IPv4", src_mask);
-                    rv = true;
-                    break;
-                }
-            } else if (family == 2U) { // IPv6
-                if (src_mask > 128U) {
-                    log_devdebug("edns_client_subnet: invalid src_mask of %u for IPv6", src_mask);
-                    rv = true;
-                    break;
-                }
-            } else {
-                log_devdebug("edns_client_subnet has unknown family %u", family);
+        // Validate family and validate non-zero src_mask as appropriate
+        if (family == 1U) { // IPv4
+            if (src_mask > 32U) {
+                log_devdebug("edns_client_subnet: invalid src_mask of %u for IPv4", src_mask);
                 rv = true;
                 break;
             }
+        } else if (family == 2U) { // IPv6
+            if (src_mask > 128U) {
+                log_devdebug("edns_client_subnet: invalid src_mask of %u for IPv6", src_mask);
+                rv = true;
+                break;
+            }
+        } else {
+            log_devdebug("edns_client_subnet has unknown family %u", family);
+            rv = true;
+            break;
         }
 
         // There should be exactly enough address bytes to cover the provided source mask (possibly 0)
@@ -348,7 +345,7 @@ static bool handle_edns_client_subnet(dnsp_ctx_t* ctx, unsigned opt_len, const u
         ctx->this_max_response -= (8 + addr_bytes); // leave room for response option
         ctx->respond_edns_client_subnet = true;
         ctx->client_info.edns_client_mask = src_mask;
-        ctx->edns_client_family = family; // copy family literally, in case src_mask==0 + junk family echo
+        ctx->edns_client_family = family; // copy family for output
     } while (0);
 
     gdnsd_assert(ctx->stats);

t/013edns_clientsub/039clientsub.t

@@ -279,18 +279,18 @@ _GDT->test_dns(
     stats => [qw/udp_reqs edns edns_clientsub formerr/],
 );
 
-# reflect arbitrary junk family without error if src_mask==0
-my $optrr_junkfam_ok = Net::DNS::RR->new(@optrr_base,
+# formerr for arbitrary junk family
+my $optrr_junkfam = Net::DNS::RR->new(@optrr_base,
     optioncode => 0x0008,
     optiondata => pack('nCC', 42, 0, 0)
 );
 _GDT->test_dns(
     v4_only => 1,
     qname => 'reflect-best.example.com', qtype => 'A',
-    q_optrr => $optrr_junkfam_ok,
-    answer => 'reflect-best.example.com 60 A 127.0.0.1',
-    addtl => $optrr_junkfam_ok,
-    stats => [qw/udp_reqs edns edns_clientsub noerror/],
+    q_optrr => $optrr_junkfam,
+    header => { rcode => 'FORMERR', aa => 0 },
+    addtl => $optrr_basic,
+    stats => [qw/udp_reqs edns edns_clientsub formerr/],
 );
 
 _GDT->test_kill_daemon($pid);