On Sat, Jul 25 2020, Sebastian Benoit <be...@openbsd.org> wrote: > Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2020.07.25 14:01:06 +0200: >> On Fri, Jul 17 2020, Jesper Wallin <jes...@ifconfig.se> wrote: >> > Hi all, >> > >> > I recently decided to add SSHFP records for my servers, since I never >> > memorize or write down my key fingerprints. I learned that if I >> > want ssh(1) to trust these records, DNSSEC needs to be enabled for my >> > zone. To validate these records, ssh(1) is using getrrsetbyname(3), >> > which checks if the AD (Authentic Data) flag is set in the response. >> > >> > To get a response with the AD flag set, the request itself also needs >> > to have the AD flag set. It turns out that getrrsetbyname(3) doesn't >> > set this and will therefore never get a validated response, unless the >> > resolver is configured to always send it, no matter what the client >> > requested. >> > >> > It seems like unwind(8) behaves this way but it also responds with the >> > RRSIG records, which is extra overhead and ignored by getrrsetbyname(3). >> > >> > This was mentioned a few years ago [0] and the solution suggested was >> > to add the RES_USE_DNSSEC to _res.options, which also makes the resolver >> > respond with the extra RRSIG records. >> > >> > Instead, by only setting the AD flag, both the request and the response >> > has the same size as without the flag set. The patch below will add >> > RES_USE_AD as an option to _res.options and set it by default. >> > This is also the default behaviour in dig(1), which I understand is a >> > bit different, but that sure added some confusion while debugging this. >> > >> > This let you run unbound(8) or any other validating resolver on your >> > local network and getrrsetbyname(3) will trust it. Do read the CAVEATS >> > in the manual of getrrsetbyname(3) though. >> > >> > As a side note, I noticed that the default value of _res.options was the >> > same value as RES_DEFAULT, so I changed it to RES_DEFAULT instead, for >> > the sake of consistency. >> > >> > Thoughts? >> >> Thanks for addressing this longstanding issue. >> >> I think using the AD bit in queries is a good idea. IIRC Peter J. >> Philipp (cc'd) suggested using it but I was not thrilled because: >> >> 1. the meaning of the AD bit in queries is relatively recent (2013 >> I think[0]) >> 2. getrrsetbyname also collects signature records, and for this you need >> EDNS + the DO bit set. Implementing this in was not 100% trivial, >> I think we had something working but Eric or I were not 100% happy >> with it. >> >> 1. is probably not a concern, after all you're supposed to use >> a trustworthy resolver, which should be modern and understand the >> purpose of the AD bit in queries. >> >> 2. is probably not a concern either. I guess that all getrrsetbyname(3) >> callers only care about the target records and the AD bit, not about the >> signature records. (Why would they use it for anyway?). In the base >> system, only ssh(1) and traceroute(8) use getrrsetbyname(3). >> AFAIK no other system provides getrrsetbyname(3), and ISC has removed >> getrrsetbyname and the whole lwres API in 2017[1]. So I'd say we're >> free to improve our version of getrrsetbyname(3) as we see fit. > > This is a concern for the stub resolver, because edns and AD does not work > everywhere. > Indeed when we switched unbound to validate by default we > learned that this is not a good idea for everyone - which lead to the > development of unwind(8).
Do you by chance have any data regarding fallout because of the AD bit set in queries? I would expect it to be ignored when not supported. EDNS and the DNSSEC DO bit is a different story indeed. >> Now let's discuss the implementation. There are two main things >> I dislike with your diff: >> - it affects all DNS queries done through libc, even if the user doesn't >> care at all about DNSSEC. I suspect there's potential for fallout >> here, but I have no data to back this. >> - trusting the AD bit by default looks a bit too easy. The >> documentation says that people should only trust the AD bit / >> RRSET_VALIDATED if the communication between libc and the resolver is >> secure, but who actually reads the documentation? >> >> For those two reasons I think the feature should be opt-in. That's what >> Florian Weimer did in glibc, with the implementation of RES_TRUSTAD[2][3]. >> The diff below implements the same semantics: >> - by default, don't send queries the AD flag, and don't trust its value >> in replies >> - if "options trust-ad" is set in resolv.conf(5), set the AD flag in >> queries and allow applications to look at the AD flag in replies. >> >> There's one more thing I'd like to check in the res_* API, but I'm >> sitting on this idea since too long already, and I'm happy with my test >> results so far. >> >> Thoughts? Comments? > > Your observation that this should not be on by default is probably correct. > > If you enable trust-ad on a system that moves around, e.g. your laptop, you > will experience failures, which is why unwind tests for this and falls back > to non-trusting dhcp learned resolvers in such cases. In fact it also falls > back to the stub resolver, i.e. our libc resolver. Which makes me think that > trust-ad should not be used when you run unwind (because the whole point of > the fallback is that dnssec does not work). But thats a documentation issue. Thanks for pointing this out. I would expect from unwind(8) that it always clears the AD bit from its responses if it could not validate them. Inclusing when it falls back to dynamically learned resolvers or the libc resolver. Sadly "options trust-ad" is not per-nameserver, so you can't have trust-ad work for 127.0.0.1 or ::1 and not random nameservers learned by DHCP. I'll investiate whet would be needed on the documentation side. > The patch reads ok, fwiw. i'm not familiar with the code. Thanks for the feedback! >> [0] https://tools.ietf.org/html/rfc6840#section-5.7 >> [1] >> https://gitlab.isc.org/isc-projects/bind9/-/commit/8eb88aafee951859264e36c315b1289cd8c2088b >> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1164339 >> [3] >> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=446997ff1433d33452b81dfa9e626b8dccf101a4 >> >> >> >> Index: include/resolv.h >> =================================================================== >> RCS file: /d/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 23 Jul 2020 13:28:15 -0000 >> @@ -186,6 +186,8 @@ struct __res_state_ext { >> #define RES_INSECURE2 0x00000800 /* type 2 security disabled */ >> #define RES_NOALIASES 0x00001000 /* shuts off HOSTALIASES >> feature */ >> #define RES_USE_INET6 0x00002000 /* use/map IPv6 in >> gethostbyname() */ >> +/* GNU extension: use higher bit to avoid conflict with ISC use */ >> +#define RES_TRUSTAD 0x80000000 /* query with AD, trust AD in >> reply */ >> /* KAME extensions: use higher bit to avoid conflict with ISC use */ >> #define RES_USE_EDNS0 0x40000000 /* use EDNS0 */ >> /* DNSSEC extensions: use higher bit to avoid conflict with ISC use */ >> Index: lib/libc/net/res_init.3 >> =================================================================== >> RCS file: /d/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 25 Jul 2020 02:09:58 -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 >> Index: share/man/man5/resolv.conf.5 >> =================================================================== >> RCS file: /d/cvs/src/share/man/man5/resolv.conf.5,v >> retrieving revision 1.60 >> diff -u -p -r1.60 resolv.conf.5 >> --- share/man/man5/resolv.conf.5 25 Apr 2020 14:22:04 -0000 1.60 >> +++ share/man/man5/resolv.conf.5 25 Jul 2020 02:08:17 -0000 >> @@ -285,6 +285,15 @@ 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 >> +Sets 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 >> +.Ic nameserver . >> +This option should be used only if the transport between the >> +.Ic nameserver >> +and the resolver is secure. >> .El >> .El >> .Pp >> Index: lib/libc/asr/asr.c >> =================================================================== >> RCS file: /d/cvs/src/lib/libc/asr/asr.c,v >> retrieving revision 1.64 >> diff -u -p -r1.64 asr.c >> --- lib/libc/asr/asr.c 6 Jul 2020 13:33:05 -0000 1.64 >> +++ lib/libc/asr/asr.c 23 Jul 2020 23:36:06 -0000 >> @@ -640,6 +640,8 @@ pass0(char **tok, int n, struct asr_ctx >> ac->ac_options |= RES_USEVC; >> else if (!strcmp(tok[i], "edns0")) >> ac->ac_options |= RES_USE_EDNS0; >> + else if (!strcmp(tok[i], "trust-ad")) >> + ac->ac_options |= RES_TRUSTAD; >> else if ((!strncmp(tok[i], "ndots:", 6))) { >> e = NULL; >> d = strtonum(tok[i] + 6, 1, 16, &e); >> Index: lib/libc/asr/asr_debug.c >> =================================================================== >> RCS file: /d/cvs/src/lib/libc/asr/asr_debug.c,v >> retrieving revision 1.26 >> diff -u -p -r1.26 asr_debug.c >> --- lib/libc/asr/asr_debug.c 3 Jul 2019 03:24:03 -0000 1.26 >> +++ lib/libc/asr/asr_debug.c 23 Jul 2020 23:36:06 -0000 >> @@ -285,6 +285,7 @@ _asr_dump_config(FILE *f, struct asr *a) >> PRINTOPT(RES_NOALIASES, "NOALIASES"); >> PRINTOPT(RES_USE_EDNS0, "USE_EDNS0"); >> PRINTOPT(RES_USE_DNSSEC, "USE_DNSSEC"); >> + PRINTOPT(RES_TRUSTAD, "TRUSTAD"); >> if (o) >> fprintf(f, " 0x%08x", o); >> fprintf(f, "\n"); >> Index: lib/libc/asr/getrrsetbyname_async.c >> =================================================================== >> RCS file: /d/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 23 Jul 2020 23:36:06 -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; >> >> /* copy name from answer section */ >> Index: lib/libc/asr/res_mkquery.c >> =================================================================== >> RCS file: /d/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 24 Jul 2020 12:34:27 -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; >> 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: /d/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 24 Jul 2020 13:02:38 -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); >> >> as->as.dns.rcode = RCODE(h.flags); >> as->as.dns.ancount = h.ancount; >> >> >> -- >> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >> > <#secure method=pgpmime mode=sign> -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
