On 2021-11-20 17:05 +01, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote:
> First, I'm happy to this subject considered again, even if I don't use
> DNSSEC these days I think it makes sense to provide this support in libc.
>
> On Sat, Nov 20 2021, Florian Obser <flor...@openbsd.org> wrote:
>> On 2021-11-20 14:40 +01, Otto Moerbeek <o...@drijf.net> wrote:
>>> On Sat, Nov 20, 2021 at 12:20:32PM +0100, Florian Obser wrote:
>>>> diff --git share/man/man5/resolv.conf.5 share/man/man5/resolv.conf.5
>>>> index 8d3b91c0832..ac64d3e6fd6 100644
>>>> --- share/man/man5/resolv.conf.5
>>>> +++ share/man/man5/resolv.conf.5
>>>> @@ -259,6 +259,12 @@ 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
>>>> +Request DNSSEC validated data from the nameservers and preserve the
>>>> +Authentic Data (AD) flag in responses.
>>>
>>>> +Otherwise the Authentic Data (AD) flag is removed from responses.
>>>
>>> This is not what happens (though the DNS header itself is not exposed
>>> in the API). Maybe describe it as:
>>>
>>
>> That is a very good point. And hints at that the diff is not complete.
>> I think we should fiddle with the header. I think res_send_async_run()
>> should unset the AD flag in ASR_STATE_PACKET. The api exposes the raw
>> packet in the res_query(3) family of functions. And this is actually
>> what glibc does.
>
> Indeed, my previous proposal used to do this (I don't remember if
> I tested the invalidation though).
>
>>> Request DNSSEC validated data from the nameservers and evaluate the AD
>>> flag in responses.
>>>
>>>> +The nameservers and the network path to them must be trusted.
>>>
>>> Maybe:
>>>
>>> Only set this flag if the nameservers and the network paths to them are
>>> trusted.
>>>
>>
>> I wanted to focus less on the technical details (AD flag) and more on
>> what this means. Of course I failed at that ;)
>> I think we should mention the AD flag so that people who know how DNSSEC
>> works can find it, but we need to better explain when random user should
>> set the flag (never!).
>>
>> I'll rework the diff.
>>
>>>> +This is the default for nameservers on localhost.
>>>> .El
>>>> .El
>>>> .Pp
>
>
> Here's a refreshed version of the diff initially sent in this thread:
>
> https://marc.info/?l=openbsd-tech&m=159567881530990&w=2
>
> It's quite similar to Florian's diff so I'd like to re-submit it.
>
>
> I like a lot the "automatic localhost trust" from Florian so
> I incorporated his improvement here, but I can leave it out from at
> commit time. In the proposal below resolv.conf(5) says:
>
> +This option should be used only if the transport between the
> +.Ic nameserver
> +and the resolver is secure.
> +It is enabled by default if
> +.Nm resolv.conf
> +only lists nameservers on localhost.
>
> which I think is more accurate regarding nameservers on localhost.
> The wording in res_init(3) is unchanged from my first proposal, it
> should also be updated if localhost is to be automatically trusted.
>
> The diff also adds trust-ad support in asr_debug.c.
>
> How do you folks like this?
>
> 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 20 Nov 2021 14:24:08 -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: 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 20 Nov 2021 15:58:39 -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
> +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.
> +It is enabled by default if
> +.Nm resolv.conf
> +only lists nameservers 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 20 Nov 2021 15:59:32 -0000
> @@ -656,6 +656,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);
> @@ -672,7 +674,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;
> + char buf[512], *ch;
> + struct sockaddr_in6 *sin6;
> + struct sockaddr_in *sin;
> + int i, trustad;
>
> asr_ctx_parse(ac, str);
>
> @@ -701,6 +706,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 20 Nov 2021 14:24:58 -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/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
>
> /* 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. 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.
> 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.
>
> 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?
OK?
p.s. I think this should be jca's commit.
diff --git include/resolv.h include/resolv.h
index fb02483871e..2422deb5484 100644
--- include/resolv.h
+++ include/resolv.h
@@ -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)
diff --git lib/libc/asr/asr.c lib/libc/asr/asr.c
index 8bcb61b6000..77bc3854420 100644
--- lib/libc/asr/asr.c
+++ lib/libc/asr/asr.c
@@ -661,7 +661,8 @@ pass0(char **tok, int n, struct asr_ctx *ac)
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 *ac)
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);
@@ -702,6 +706,27 @@ asr_ctx_from_string(struct asr_ctx *ac, const char *str)
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);
}
diff --git lib/libc/asr/asr_debug.c lib/libc/asr/asr_debug.c
index f9378d156b7..e6092e3b178 100644
--- lib/libc/asr/asr_debug.c
+++ lib/libc/asr/asr_debug.c
@@ -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");
diff --git lib/libc/asr/res_mkquery.c lib/libc/asr/res_mkquery.c
index c3d5af30f29..97b965e4c2a 100644
--- lib/libc/asr/res_mkquery.c
+++ lib/libc/asr/res_mkquery.c
@@ -62,6 +62,8 @@ res_mkquery(int op, const char *dname, int class, int type,
h.flags |= RD_MASK;
if (ac->ac_options & RES_USE_CD)
h.flags |= CD_MASK;
+ if (ac->ac_options & RES_TRUSTAD)
+ h.flags |= AD_MASK;
h.qdcount = 1;
if (ac->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
h.arcount = 1;
diff --git lib/libc/asr/res_send_async.c lib/libc/asr/res_send_async.c
index c5cc41f56df..834d4d3443a 100644
--- lib/libc/asr/res_send_async.c
+++ lib/libc/asr/res_send_async.c
@@ -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, struct asr_result
*ar)
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,9 @@ setup_query(struct asr_query *as, const char *name, const
char *dom,
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)
+ h.flags |= AD_MASK;
+
h.qdcount = 1;
if (as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
h.arcount = 1;
@@ -747,6 +753,21 @@ validate_packet(struct asr_query *as)
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);
+}
+
/*
* Set the async context nameserver index to the next nameserver, cycling
* over the list until the maximum retry counter is reached. Return 0 on
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 unless localhost is queried.
+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
+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.
.El
.El
.Pp
--
I'm not entirely sure you are real.
