On Sat, Nov 20 2021, Florian Obser <flor...@openbsd.org> wrote:
> On 2021-11-20 17:05 +01, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote:
[...]
>> Index: lib/libc/asr/getrrsetbyname_async.c
>> ===================================================================
>> RCS file: /home/cvs/src/lib/libc/asr/getrrsetbyname_async.c,v
>> retrieving revision 1.11
>> diff -u -p -r1.11 getrrsetbyname_async.c
>> --- lib/libc/asr/getrrsetbyname_async.c 23 Feb 2017 17:04:02 -0000
>> 1.11
>> +++ lib/libc/asr/getrrsetbyname_async.c 20 Nov 2021 14:24:08 -0000
>> @@ -32,7 +32,8 @@
>> #include "asr_private.h"
>>
>> static int getrrsetbyname_async_run(struct asr_query *, struct asr_result
>> *);
>> -static void get_response(struct asr_result *, const char *, int);
>> +static void get_response(struct asr_query *, struct asr_result *, const
>> char *,
>> + int);
>>
>> struct asr_query *
>> getrrsetbyname_async(const char *hostname, unsigned int rdclass,
>> @@ -150,7 +151,7 @@ getrrsetbyname_async_run(struct asr_quer
>> break;
>> }
>>
>> - get_response(ar, ar->ar_data, ar->ar_datalen);
>> + get_response(as, ar, ar->ar_data, ar->ar_datalen);
>> free(ar->ar_data);
>> async_set_state(as, ASR_STATE_HALT);
>> break;
>> @@ -255,7 +256,8 @@ static void free_dns_response(struct dns
>> static int count_dns_rr(struct dns_rr *, u_int16_t, u_int16_t);
>>
>> static void
>> -get_response(struct asr_result *ar, const char *pkt, int pktlen)
>> +get_response(struct asr_query *as, struct asr_result *ar, const char *pkt,
>> + int pktlen)
>> {
>> struct rrsetinfo *rrset = NULL;
>> struct dns_response *response = NULL;
>> @@ -287,7 +289,7 @@ get_response(struct asr_result *ar, cons
>> rrset->rri_nrdatas = response->header.ancount;
>>
>> /* check for authenticated data */
>> - if (response->header.ad == 1)
>> + if (as->as_ctx->ac_options & RES_TRUSTAD && response->header.ad == 1)
>> rrset->rri_flags |= RRSET_VALIDATED;
>
> we can skip all this if we mask AD correctly in the header
That's much nicer indeed!
>> /* copy name from answer section */
>> Index: lib/libc/asr/res_mkquery.c
>> ===================================================================
>> RCS file: /home/cvs/src/lib/libc/asr/res_mkquery.c,v
>> retrieving revision 1.13
>> diff -u -p -r1.13 res_mkquery.c
>> --- lib/libc/asr/res_mkquery.c 14 Jan 2019 06:49:42 -0000 1.13
>> +++ lib/libc/asr/res_mkquery.c 20 Nov 2021 14:24:08 -0000
>> @@ -62,6 +62,9 @@ res_mkquery(int op, const char *dname, i
>> h.flags |= RD_MASK;
>> if (ac->ac_options & RES_USE_CD)
>> h.flags |= CD_MASK;
>> + if ((ac->ac_options & RES_TRUSTAD) &&
>> + !(ac->ac_options & RES_USE_DNSSEC))
>> + h.flags |= AD_MASK;
>
> do you remember why you check for !RES_USE_DNSSEC?
> I'd like to leave it out.
First, here's my understanding of RES_USE_DNSSEC: it's supposed to
activate EDNS and set the DO bit. The server is then supposed to reply
with AD set only if the data has been validated (with or without
DNSSEC), and possibly append add DNSSEC data if available.
Since I didn't know the semantics of both setting AD and DO in a query
(I would expect such semantics to be sane) I wrote those more
conservative checks instead. Does this make sense? If so, maybe
a comment would help?
/* Set the AD flag unless we already plan to set the DNSSEC DO bit. */
if ((ac->ac_options & RES_TRUSTAD) &&
!(ac->ac_options & RES_USE_DNSSEC))
> In fact I don't think RES_USE_DNSSEC is useful
> at all.
> If you want to set the DO flag and start DNSSEC from first principles
> you are better of talking to the network directly, i.e. rewrite unwind.
RES_USE_DNSSEC has been there since some time already and it's used by
software in the ports tree, precisely to detect AD=1 - and not so much
for the key records I think.
>> h.qdcount = 1;
>> if (ac->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
>> h.arcount = 1;
>> Index: lib/libc/asr/res_send_async.c
>> ===================================================================
>> RCS file: /home/cvs/src/lib/libc/asr/res_send_async.c,v
>> retrieving revision 1.39
>> diff -u -p -r1.39 res_send_async.c
>> --- lib/libc/asr/res_send_async.c 28 Sep 2019 11:21:07 -0000 1.39
>> +++ lib/libc/asr/res_send_async.c 20 Nov 2021 14:24:08 -0000
>> @@ -378,6 +378,9 @@ setup_query(struct asr_query *as, const
>> h.flags |= RD_MASK;
>> if (as->as_ctx->ac_options & RES_USE_CD)
>> h.flags |= CD_MASK;
>> + if ((as->as_ctx->ac_options & RES_TRUSTAD) &&
>> + !(as->as_ctx->ac_options & RES_USE_DNSSEC))
>> + h.flags |= AD_MASK;
>> h.qdcount = 1;
>> if (as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
>> h.arcount = 1;
>> @@ -700,6 +703,9 @@ validate_packet(struct asr_query *as)
>> /* Must be a response */
>> if ((h.flags & QR_MASK) == 0)
>> goto inval;
>> + /* Zap the AD flag unless the server is considered trusted. */
>> + if (!(as->as_ctx->ac_options & RES_TRUSTAD))
>> + h.flags &= ~(AD_MASK);
>
> this does not work, you are masking on a copy.
Indeed, thanks. Your fix looks correct to me.
>>
>> as->as.dns.rcode = RCODE(h.flags);
>> as->as.dns.ancount = h.ancount;
>> Index: lib/libc/net/res_init.3
>> ===================================================================
>> RCS file: /home/cvs/src/lib/libc/net/res_init.3,v
>> retrieving revision 1.4
>> diff -u -p -r1.4 res_init.3
>> --- lib/libc/net/res_init.3 25 Apr 2020 21:06:17 -0000 1.4
>> +++ lib/libc/net/res_init.3 20 Nov 2021 14:24:08 -0000
>> @@ -179,6 +179,17 @@ This option has no effect.
>> In the past, it turned off the legacy
>> .Ev HOSTALIASES
>> feature.
>> +.It Dv RES_TRUSTAD
>> +If set, the resolver routines will set the AD flag in DNS queries and
>> +preserve the value of the AD flag in DNS replies (this flag is stripped
>> +by default).
>> +Useful when applications want to ensure that the DNS resources they
>> +look up have been signed with DNSSEC and validated by the name server.
>> +Direct use of this option to enable AD bit processing is discouraged.
>> +Instead the use of trusted name servers should be annotated with
>> +.Dq options trust-ad
>> +in
>> +.Xr resolv.conf 5 .
>> .It Dv RES_USE_INET6
>> With this option
>> .Xr gethostbyname 3
>>
>>
>
> I have incorporated the parts of your diff that I was missing,
> implemented masking of the AD flag in the header and tweaked the
> documentation a bit.
>
> I think we are slowly converging on the same diff :)
>
> Anything I missed?
Please see below for some attempts at improving the docs.
> OK?
>
> p.s. I think this should be jca's commit.
Patience pays off, thanks. :)
[...]
> diff --git lib/libc/net/res_init.3 lib/libc/net/res_init.3
> index 4a4d0950a5e..557a0068232 100644
> --- lib/libc/net/res_init.3
> +++ lib/libc/net/res_init.3
> @@ -179,6 +179,16 @@ This option has no effect.
> In the past, it turned off the legacy
> .Ev HOSTALIASES
> feature.
> +.It Dv RES_TRUSTAD
> +If set, the resolver routines will set the AD flag in DNS queries and
> +preserve the value of the AD flag in DNS replies.
> +If not set, the resolver routines will not set the AD flag in queries
> +and will unset the AD flag in responses
This second sentence could be shortened.
> unless localhost is queried.
I think that this part ("unless localhost is queried") should be more
precise.
> +Direct use of this option to enable AD bit processing is discouraged.
> +Instead the use of trusted name servers should be annotated with
> +.Dq options trust-ad
> +in
> +.Xr resolv.conf 5 .
> .It Dv RES_USE_INET6
> With this option
> .Xr gethostbyname 3
> diff --git share/man/man5/resolv.conf.5 share/man/man5/resolv.conf.5
> index 8d3b91c0832..483727f0f2d 100644
> --- share/man/man5/resolv.conf.5
> +++ share/man/man5/resolv.conf.5
> @@ -259,6 +259,17 @@ first as an absolute name before any search list
> elements are appended to it.
> .It Cm tcp
> Forces the use of TCP for queries.
> Normal behaviour is to query via UDP but fall back to TCP on failure.
> +.It Cm trust-ad
> +A nameserver indicating that it performed DNSSEC validation by setting the
I suggest nameserver -> name server for consistency with the rest of the
file.
> +Authentic Data (AD) flag in the answer can only be trusted if the
> +nameserver itself is trusted and the network path is trusted.
> +Generally this is not the case and the AD flag is cleared in the answer.
> +The
> +.Cm trust-ad
> +flag lets the system administrator indicate that the nameserver and the
> +network path are trusted.
> +Nameservers on localhost can be trusted and
> +.Cm trust-ad is enabled per default.
Already mentioned above, this last sentence doesn't really describe the
precise behavior you proposa. We *could* have a mix of localhost and
non-localhost servers in resolv.conf, and use/trust AD=1 only for
localhost servers. But the automatic detection code you wrote enables
trust-ad if resolv.conf(5) *only lists localhost entries*. Which is
perfectly fine as far as I'm concerned, and lead to my first proposal:
+It is enabled by default if
+.Nm resolv.conf
+only lists nameservers on localhost.
Here's an updated diff with the proposals mentioned.
Index: include/resolv.h
===================================================================
RCS file: /home/cvs/src/include/resolv.h,v
retrieving revision 1.22
diff -u -p -r1.22 resolv.h
--- include/resolv.h 14 Jan 2019 06:23:06 -0000 1.22
+++ include/resolv.h 21 Nov 2021 19:15:30 -0000
@@ -191,6 +191,7 @@ struct __res_state_ext {
/* DNSSEC extensions: use higher bit to avoid conflict with ISC use */
#define RES_USE_DNSSEC 0x20000000 /* use DNSSEC using OK bit in
OPT */
#define RES_USE_CD 0x10000000 /* set Checking Disabled flag */
+#define RES_TRUSTAD 0x80000000 /* Request AD, keep it in
responses. */
#define RES_DEFAULT (RES_RECURSE | RES_DEFNAMES | RES_DNSRCH)
Index: share/man/man5/resolv.conf.5
===================================================================
RCS file: /home/cvs/src/share/man/man5/resolv.conf.5,v
retrieving revision 1.62
diff -u -p -r1.62 resolv.conf.5
--- share/man/man5/resolv.conf.5 24 Aug 2021 07:30:32 -0000 1.62
+++ share/man/man5/resolv.conf.5 21 Nov 2021 20:22:49 -0000
@@ -259,6 +259,18 @@ first as an absolute name before any sea
.It Cm tcp
Forces the use of TCP for queries.
Normal behaviour is to query via UDP but fall back to TCP on failure.
+.It Cm trust-ad
+A name server indicating that it performed DNSSEC validation by setting the
+Authentic Data (AD) flag in the answer can only be trusted if the
+name server itself is trusted and the network path is trusted.
+Generally this is not the case and the AD flag is cleared in the answer.
+The
+.Cm trust-ad
+option lets the system administrator indicate that the name server and the
+network path are trusted.
+This option is automatically enabled if
+.Nm resolv.conf
+only lists name servers on localhost.
.El
.El
.Pp
Index: lib/libc/asr/asr.c
===================================================================
RCS file: /home/cvs/src/lib/libc/asr/asr.c,v
retrieving revision 1.66
diff -u -p -r1.66 asr.c
--- lib/libc/asr/asr.c 5 Nov 2021 13:08:58 -0000 1.66
+++ lib/libc/asr/asr.c 21 Nov 2021 19:15:30 -0000
@@ -661,7 +661,8 @@ pass0(char **tok, int n, struct asr_ctx
d = strtonum(tok[i] + 6, 1, 16, &e);
if (e == NULL)
ac->ac_ndots = d;
- }
+ } else if (!strcmp(tok[i], "trust-ad"))
+ ac->ac_options |= RES_TRUSTAD;
}
}
}
@@ -672,7 +673,10 @@ pass0(char **tok, int n, struct asr_ctx
static int
asr_ctx_from_string(struct asr_ctx *ac, const char *str)
{
- char buf[512], *ch;
+ struct sockaddr_in6 *sin6;
+ struct sockaddr_in *sin;
+ int i, trustad;
+ char buf[512], *ch;
asr_ctx_parse(ac, str);
@@ -701,6 +705,27 @@ asr_ctx_from_string(struct asr_ctx *ac,
if (ch && asr_ndots(++ch) == 0)
break;
}
+
+ trustad = 1;
+ for (i = 0; i < ac->ac_nscount && trustad; i++) {
+ switch (ac->ac_ns[i]->sa_family) {
+ case AF_INET:
+ sin = (struct sockaddr_in *)ac->ac_ns[i];
+ if (sin->sin_addr.s_addr != htonl(INADDR_LOOPBACK))
+ trustad = 0;
+ break;
+ case AF_INET6:
+ sin6 = (struct sockaddr_in6 *)ac->ac_ns[i];
+ if (!IN6_IS_ADDR_LOOPBACK(&sin6->sin6_addr))
+ trustad = 0;
+ break;
+ default:
+ trustad = 0;
+ break;
+ }
+ }
+ if (trustad)
+ ac->ac_options |= RES_TRUSTAD;
return (0);
}
Index: lib/libc/asr/asr_debug.c
===================================================================
RCS file: /home/cvs/src/lib/libc/asr/asr_debug.c,v
retrieving revision 1.27
diff -u -p -r1.27 asr_debug.c
--- lib/libc/asr/asr_debug.c 2 Apr 2021 07:00:30 -0000 1.27
+++ lib/libc/asr/asr_debug.c 21 Nov 2021 19:15:30 -0000
@@ -286,6 +286,7 @@ _asr_dump_config(FILE *f, struct asr *a)
PRINTOPT(RES_USE_EDNS0, "USE_EDNS0");
PRINTOPT(RES_USE_DNSSEC, "USE_DNSSEC");
PRINTOPT(RES_USE_CD, "USE_CD");
+ PRINTOPT(RES_TRUSTAD, "TRUSTAD");
if (o)
fprintf(f, " 0x%08x", o);
fprintf(f, "\n");
Index: lib/libc/asr/res_mkquery.c
===================================================================
RCS file: /home/cvs/src/lib/libc/asr/res_mkquery.c,v
retrieving revision 1.13
diff -u -p -r1.13 res_mkquery.c
--- lib/libc/asr/res_mkquery.c 14 Jan 2019 06:49:42 -0000 1.13
+++ lib/libc/asr/res_mkquery.c 21 Nov 2021 20:09:08 -0000
@@ -62,6 +62,10 @@ res_mkquery(int op, const char *dname, i
h.flags |= RD_MASK;
if (ac->ac_options & RES_USE_CD)
h.flags |= CD_MASK;
+ /* Set the AD flag unless we already plan to set the DNSSEC DO bit. */
+ if ((ac->ac_options & RES_TRUSTAD) &&
+ !(ac->ac_options & RES_USE_DNSSEC))
+ h.flags |= AD_MASK;
h.qdcount = 1;
if (ac->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
h.arcount = 1;
Index: lib/libc/asr/res_send_async.c
===================================================================
RCS file: /home/cvs/src/lib/libc/asr/res_send_async.c,v
retrieving revision 1.39
diff -u -p -r1.39 res_send_async.c
--- lib/libc/asr/res_send_async.c 28 Sep 2019 11:21:07 -0000 1.39
+++ lib/libc/asr/res_send_async.c 21 Nov 2021 20:09:41 -0000
@@ -42,6 +42,7 @@ static int udp_recv(struct asr_query *);
static int tcp_write(struct asr_query *);
static int tcp_read(struct asr_query *);
static int validate_packet(struct asr_query *);
+static void clear_ad(struct asr_result *);
static int setup_query(struct asr_query *, const char *, const char *, int,
int);
static int ensure_ibuf(struct asr_query *, size_t);
static int iter_ns(struct asr_query *);
@@ -258,6 +259,8 @@ res_send_async_run(struct asr_query *as,
as->as.dns.ibuf = NULL;
ar->ar_errno = 0;
ar->ar_rcode = as->as.dns.rcode;
+ if (!(as->as_ctx->ac_options & RES_TRUSTAD))
+ clear_ad(ar);
async_set_state(as, ASR_STATE_HALT);
break;
@@ -378,6 +381,11 @@ setup_query(struct asr_query *as, const
h.flags |= RD_MASK;
if (as->as_ctx->ac_options & RES_USE_CD)
h.flags |= CD_MASK;
+ /* Set the AD flag unless we already plan to set the DNSSEC DO bit. */
+ if ((as->as_ctx->ac_options & RES_TRUSTAD) &&
+ !(as->as_ctx->ac_options & RES_USE_DNSSEC))
+ h.flags |= AD_MASK;
+
h.qdcount = 1;
if (as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
h.arcount = 1;
@@ -745,6 +753,21 @@ validate_packet(struct asr_query *as)
inval:
errno = EINVAL;
return (-1);
+}
+
+/*
+ * Clear AD flag in the answer.
+ */
+static void
+clear_ad(struct asr_result *ar)
+{
+ struct asr_dns_header *h;
+ uint16_t flags;
+
+ h = (struct asr_dns_header *)ar->ar_data;
+ flags = ntohs(h->flags);
+ flags &= ~(AD_MASK);
+ h->flags = htons(flags);
}
/*
Index: lib/libc/net/res_init.3
===================================================================
RCS file: /home/cvs/src/lib/libc/net/res_init.3,v
retrieving revision 1.4
diff -u -p -r1.4 res_init.3
--- lib/libc/net/res_init.3 25 Apr 2020 21:06:17 -0000 1.4
+++ lib/libc/net/res_init.3 21 Nov 2021 20:20:22 -0000
@@ -179,6 +179,18 @@ This option has no effect.
In the past, it turned off the legacy
.Ev HOSTALIASES
feature.
+.It Dv RES_TRUSTAD
+If set, the resolver routines will set the AD flag in DNS queries and
+preserve the value of the AD flag in DNS replies.
+If not set, the resolver routines will clear the AD flag in responses.
+Direct use of this option to enable AD bit processing is discouraged.
+Instead the use of trusted name servers should be annotated with
+.Dq options trust-ad
+in
+.Xr resolv.conf 5 .
+This option is automatically enabled if
+.Xr resolv.conf 5
+only lists name servers on localhost.
.It Dv RES_USE_INET6
With this option
.Xr gethostbyname 3
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
