[PATCH] dhcpv6: Properly separate domain names in search list

[PATCH] dhcpv6: Properly separate domain names in search list

[Stefano Brivio]

If we go over the flattened list of search domains and just replace
dots and zero bytes with the length of the next label to implement
the encoding specified by section 3.1 of RFC 1035, if there are
multiple domains in the search list, we'll also replace separators
between two domain names with the length of the first label of the
second domain, plus one. Those should remain as zero bytes to
separate domains, though.

To distinguish between label separators and domain names separators,
for simplicity, introduce a dot before the first label of every
domain we copy to form the list. All dots are then replaced by label
lengths, and separators (zero bytes) remain as they are.

As we do this, we need to make sure we don't replace the trailing
dot, if present: that's already a separator. Skip copying it, and
just add separators as needed.

Now that we don't copy those, though, we might end up with
zero-length domains: skip them, as they're meaningless anyway.

And as we might skip domains, we can't use the index 'i' to check if
we're at the beginning of the option -- use 'srch' instead.

This is very similar to how we prepare the list for NDP option 31,
except that we don't need padding (RFC 8106, 5.2) here, and we should
refactor this into common functions, but it probably makes sense to
rework the NDP responder (https://bugs.passt.top/show_bug.cgi?id=21)
first.

Reported-by: Sebastian Mitterle <smitterl@redhat.com>
Link: https://bugs.passt.top/show_bug.cgi?id=75
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 dhcpv6.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/dhcpv6.c b/dhcpv6.c
index fc42a84..58171bb 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -376,24 +376,34 @@ search:
 		return offset;
 
 	for (i = 0; *c->dns_search[i].n; i++) {
-		if (!i) {
+		size_t name_len = strlen(c->dns_search[i].n);
+
+		/* We already append separators, don't duplicate if present */
+		if (c->dns_search[i].n[name_len - 1] == '.')
+			name_len--;
+
+		/* Skip root-only search domains */
+		if (!name_len)
+			continue;
+
+		if (!srch) {
 			srch = (struct opt_dns_search *)(buf + offset);
 			offset += sizeof(struct opt_hdr);
 			srch->hdr.t = OPT_DNS_SEARCH;
 			srch->hdr.l = 0;
 			p = srch->list;
-			*p = 0;
 		}
 
-		p = stpcpy(p + 1, c->dns_search[i].n);
-		*(p++) = 0;
-		srch->hdr.l += strlen(c->dns_search[i].n) + 2;
-		offset += strlen(c->dns_search[i].n) + 2;
+		*p = '.';
+		p = stpncpy(p + 1, c->dns_search[i].n, name_len);
+		p++;
+		srch->hdr.l += name_len + 2;
+		offset += name_len + 2;
 	}
 
 	if (srch) {
 		for (i = 0; i < srch->hdr.l; i++) {
-			if (srch->list[i] == '.' || !srch->list[i]) {
+			if (srch->list[i] == '.') {
 				srch->list[i] = strcspn(srch->list + i + 1,
 							".");
 			}
-- 
2.39.2

Re: [PATCH] dhcpv6: Properly separate domain names in search list

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3508 bytes --]

On Wed, Sep 20, 2023 at 05:05:06PM +0200, Stefano Brivio wrote:
> If we go over the flattened list of search domains and just replace
> dots and zero bytes with the length of the next label to implement
> the encoding specified by section 3.1 of RFC 1035, if there are
> multiple domains in the search list, we'll also replace separators
> between two domain names with the length of the first label of the
> second domain, plus one.

That is... an impressively long sentence.  Any chance you could reword
that in shorter ones that are easier to follow ;).

> Those should remain as zero bytes to
> separate domains, though.
> 
> To distinguish between label separators and domain names separators,
> for simplicity, introduce a dot before the first label of every
> domain we copy to form the list. All dots are then replaced by label
> lengths, and separators (zero bytes) remain as they are.
> 
> As we do this, we need to make sure we don't replace the trailing
> dot, if present: that's already a separator. Skip copying it, and
> just add separators as needed.
> 
> Now that we don't copy those, though, we might end up with
> zero-length domains: skip them, as they're meaningless anyway.
> 
> And as we might skip domains, we can't use the index 'i' to check if
> we're at the beginning of the option -- use 'srch' instead.
> 
> This is very similar to how we prepare the list for NDP option 31,
> except that we don't need padding (RFC 8106, 5.2) here, and we should
> refactor this into common functions, but it probably makes sense to
> rework the NDP responder (https://bugs.passt.top/show_bug.cgi?id=21)
> first.
> 
> Reported-by: Sebastian Mitterle <smitterl@redhat.com>
> Link: https://bugs.passt.top/show_bug.cgi?id=75
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  dhcpv6.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/dhcpv6.c b/dhcpv6.c
> index fc42a84..58171bb 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -376,24 +376,34 @@ search:
>  		return offset;
>  
>  	for (i = 0; *c->dns_search[i].n; i++) {
> -		if (!i) {
> +		size_t name_len = strlen(c->dns_search[i].n);
> +
> +		/* We already append separators, don't duplicate if present */
> +		if (c->dns_search[i].n[name_len - 1] == '.')
> +			name_len--;
> +
> +		/* Skip root-only search domains */
> +		if (!name_len)
> +			continue;

Should we consider doing this normalisation when we build
c->dns_search, rather than here?

> +		if (!srch) {
>  			srch = (struct opt_dns_search *)(buf + offset);
>  			offset += sizeof(struct opt_hdr);
>  			srch->hdr.t = OPT_DNS_SEARCH;
>  			srch->hdr.l = 0;
>  			p = srch->list;
> -			*p = 0;
>  		}
>  
> -		p = stpcpy(p + 1, c->dns_search[i].n);
> -		*(p++) = 0;
> -		srch->hdr.l += strlen(c->dns_search[i].n) + 2;
> -		offset += strlen(c->dns_search[i].n) + 2;
> +		*p = '.';
> +		p = stpncpy(p + 1, c->dns_search[i].n, name_len);
> +		p++;
> +		srch->hdr.l += name_len + 2;
> +		offset += name_len + 2;
>  	}
>  
>  	if (srch) {
>  		for (i = 0; i < srch->hdr.l; i++) {
> -			if (srch->list[i] == '.' || !srch->list[i]) {
> +			if (srch->list[i] == '.') {
>  				srch->list[i] = strcspn(srch->list + i + 1,
>  							".");
>  			}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] dhcpv6: Properly separate domain names in search list

[Stefano Brivio]

On Thu, 21 Sep 2023 09:55:11 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Sep 20, 2023 at 05:05:06PM +0200, Stefano Brivio wrote:
> > If we go over the flattened list of search domains and just replace
> > dots and zero bytes with the length of the next label to implement
> > the encoding specified by section 3.1 of RFC 1035, if there are
> > multiple domains in the search list, we'll also replace separators
> > between two domain names with the length of the first label of the
> > second domain, plus one.  
> 
> That is... an impressively long sentence.  Any chance you could reword
> that in shorter ones that are easier to follow ;).

Oops. :) What about:

  To prepare the DHCPv6 domain search list option, we go over the
  flattened list of domains, and replace both dots and zero bytes with
  a counter of bytes in the next label, implementing the encoding specified
  by section 3.1 of RFC 1035.

  If there are multiple domains in the list, however, zero bytes serve
  as markers for the end of a domain name, and we'll replace them with
  the length of the first label of the next domain, plus one. This is
  wrong. We should only convert the dots before the labels.

?

> > Those should remain as zero bytes to
> > separate domains, though.
> > 
> > To distinguish between label separators and domain names separators,
> > for simplicity, introduce a dot before the first label of every
> > domain we copy to form the list. All dots are then replaced by label
> > lengths, and separators (zero bytes) remain as they are.
> > 
> > As we do this, we need to make sure we don't replace the trailing
> > dot, if present: that's already a separator. Skip copying it, and
> > just add separators as needed.
> > 
> > Now that we don't copy those, though, we might end up with
> > zero-length domains: skip them, as they're meaningless anyway.
> > 
> > And as we might skip domains, we can't use the index 'i' to check if
> > we're at the beginning of the option -- use 'srch' instead.
> > 
> > This is very similar to how we prepare the list for NDP option 31,
> > except that we don't need padding (RFC 8106, 5.2) here, and we should
> > refactor this into common functions, but it probably makes sense to
> > rework the NDP responder (https://bugs.passt.top/show_bug.cgi?id=21)
> > first.
> > 
> > Reported-by: Sebastian Mitterle <smitterl@redhat.com>
> > Link: https://bugs.passt.top/show_bug.cgi?id=75
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  dhcpv6.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/dhcpv6.c b/dhcpv6.c
> > index fc42a84..58171bb 100644
> > --- a/dhcpv6.c
> > +++ b/dhcpv6.c
> > @@ -376,24 +376,34 @@ search:
> >  		return offset;
> >  
> >  	for (i = 0; *c->dns_search[i].n; i++) {
> > -		if (!i) {
> > +		size_t name_len = strlen(c->dns_search[i].n);
> > +
> > +		/* We already append separators, don't duplicate if present */
> > +		if (c->dns_search[i].n[name_len - 1] == '.')
> > +			name_len--;
> > +
> > +		/* Skip root-only search domains */
> > +		if (!name_len)
> > +			continue;  
> 
> Should we consider doing this normalisation when we build
> c->dns_search, rather than here?

I quickly looked at that, but it complicates the (insane) compression
scheme from RFC 1035 4.1.4 implemented by the DHCP server -- and also
we would need to convert them back before printing them.

I don't think it's necessarily a bad idea though. I guess we should
have a few explicit functions to convert between different encodings and
then stick to the storage format that turns out to be the most
convenient. But that's beyond the scope of this fix.

-- 
Stefano

Re: [PATCH] dhcpv6: Properly separate domain names in search list

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4372 bytes --]

On Thu, Sep 21, 2023 at 05:08:22PM +0200, Stefano Brivio wrote:
> On Thu, 21 Sep 2023 09:55:11 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Sep 20, 2023 at 05:05:06PM +0200, Stefano Brivio wrote:
> > > If we go over the flattened list of search domains and just replace
> > > dots and zero bytes with the length of the next label to implement
> > > the encoding specified by section 3.1 of RFC 1035, if there are
> > > multiple domains in the search list, we'll also replace separators
> > > between two domain names with the length of the first label of the
> > > second domain, plus one.  
> > 
> > That is... an impressively long sentence.  Any chance you could reword
> > that in shorter ones that are easier to follow ;).
> 
> Oops. :) What about:
> 
>   To prepare the DHCPv6 domain search list option, we go over the
>   flattened list of domains, and replace both dots and zero bytes with
>   a counter of bytes in the next label, implementing the encoding specified
>   by section 3.1 of RFC 1035.
> 
>   If there are multiple domains in the list, however, zero bytes serve
>   as markers for the end of a domain name, and we'll replace them with
>   the length of the first label of the next domain, plus one. This is
>   wrong. We should only convert the dots before the labels.
> 
> ?

Better..  I think my main confusion is that I don't remember enough
about DNS to recall what the distinction is between domains and
labels.

> > > Those should remain as zero bytes to
> > > separate domains, though.
> > > 
> > > To distinguish between label separators and domain names separators,
> > > for simplicity, introduce a dot before the first label of every
> > > domain we copy to form the list. All dots are then replaced by label
> > > lengths, and separators (zero bytes) remain as they are.
> > > 
> > > As we do this, we need to make sure we don't replace the trailing
> > > dot, if present: that's already a separator. Skip copying it, and
> > > just add separators as needed.
> > > 
> > > Now that we don't copy those, though, we might end up with
> > > zero-length domains: skip them, as they're meaningless anyway.
> > > 
> > > And as we might skip domains, we can't use the index 'i' to check if
> > > we're at the beginning of the option -- use 'srch' instead.
> > > 
> > > This is very similar to how we prepare the list for NDP option 31,
> > > except that we don't need padding (RFC 8106, 5.2) here, and we should
> > > refactor this into common functions, but it probably makes sense to
> > > rework the NDP responder (https://bugs.passt.top/show_bug.cgi?id=21)
> > > first.
> > > 
> > > Reported-by: Sebastian Mitterle <smitterl@redhat.com>
> > > Link: https://bugs.passt.top/show_bug.cgi?id=75
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  dhcpv6.c | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/dhcpv6.c b/dhcpv6.c
> > > index fc42a84..58171bb 100644
> > > --- a/dhcpv6.c
> > > +++ b/dhcpv6.c
> > > @@ -376,24 +376,34 @@ search:
> > >  		return offset;
> > >  
> > >  	for (i = 0; *c->dns_search[i].n; i++) {
> > > -		if (!i) {
> > > +		size_t name_len = strlen(c->dns_search[i].n);
> > > +
> > > +		/* We already append separators, don't duplicate if present */
> > > +		if (c->dns_search[i].n[name_len - 1] == '.')
> > > +			name_len--;
> > > +
> > > +		/* Skip root-only search domains */
> > > +		if (!name_len)
> > > +			continue;  
> > 
> > Should we consider doing this normalisation when we build
> > c->dns_search, rather than here?
> 
> I quickly looked at that, but it complicates the (insane) compression
> scheme from RFC 1035 4.1.4 implemented by the DHCP server -- and also
> we would need to convert them back before printing them.

Ok, fair enough.

> I don't think it's necessarily a bad idea though. I guess we should
> have a few explicit functions to convert between different encodings and
> then stick to the storage format that turns out to be the most
> convenient. But that's beyond the scope of this fix.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]