[Dnsmasq-discuss] [PATCH] Accept /32 and /0 as valid CIDR prefixes for rev-server directive

Discussion:

o***@sigexec.com

2017-02-13 23:31:21 UTC

From: Olivier Gayot <***@sigexec.com>

[ excerpt from the man page ]
The rev-server directive provides a syntactic sugar to make specifying
address-to-name queries easier. For example
--rev-server=1.2.3.0/24,192.168.0.1 is exactly equivalent to
--server=/3.2.1.in-addr.arpa/192.168.0.1

It is not mentioned in the man page but specifying anything but /8 or
/24 as the CIDR prefix has the same effect as specifying /16.

It is not a big deal for subnets on non-octet boundaries since they
cannot be represented using a single in-addr.arpa address. However, it
is unconvenient for /32 and /0 prefixes while their analogous server
directives behave as expected. E.g. the following server directives work
as expected:

server=/42.10.168.192.in-addr.arpa/1.2.3.4
server=/in-addr.arpa/1.2.3.4

but the following do not:

rev-server=192.168.10.42/32,1.2.3.4
rev-server=192.168.10.42/0,1.2.3.4

and, in practice, they behave the same as:

server=/168.192.in-addr.arpa/1.2.3.4
server=/168.192.in-addr.arpa/1.2.3.4

This strange behaviour is fixed by accepting /32 and /0 CIDR prefixes as
valid values. Any other value will still be considered the same as /16.

Signed-off-by: Olivier Gayot <***@sigexec.com>
---
src/option.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/option.c b/src/option.c
index 4a5ef5f..eeca3d6 100644
--- a/src/option.c
+++ b/src/option.c
@@ -850,19 +850,30 @@ char *parse_server(char *arg, union mysockaddr *addr, union mysockaddr *source_a
static struct server *add_rev4(struct in_addr addr, int msize)
{
struct server *serv = opt_malloc(sizeof(struct server));
- in_addr_t a = ntohl(addr.s_addr) >> 8;
+ in_addr_t a = ntohl(addr.s_addr);
char *p;

memset(serv, 0, sizeof(struct server));
- p = serv->domain = opt_malloc(25); /* strlen("xxx.yyy.zzz.in-addr.arpa")+1 */
-
- if (msize == 24)
- p += sprintf(p, "%d.", a & 0xff);
- a = a >> 8;
- if (msize != 8)
- p += sprintf(p, "%d.", a & 0xff);
- a = a >> 8;
- p += sprintf(p, "%d.in-addr.arpa", a & 0xff);
+ p = serv->domain = opt_malloc(29); /* strlen("xxx.yyy.zzz.ttt.in-addr.arpa")+1 */
+
+ switch (msize)
+ {
+ case 32:
+ p += sprintf(p, "%d.", a & 0xff);
+ /* fall through */
+ case 24:
+ p += sprintf(p, "%d.", (a >> 8) & 0xff);
+ /* fall through */
+ default:
+ case 16:
+ p += sprintf(p, "%d.", (a >> 16) & 0xff);
+ /* fall through */
+ case 8:
+ p += sprintf(p, "%d.in-addr.arpa", (a >> 24) & 0xff);
+ break;
+ case 0:
+ p += sprintf(p, "in-addr.arpa");
+ }

serv->flags = SERV_HAS_DOMAIN;
serv->next = daemon->servers;

--
2.11.1

/dev/rob0

2017-02-14 14:16:02 UTC

Permalink

Post by o***@sigexec.com
[ excerpt from the man page ]
The rev-server directive provides a syntactic sugar to make
specifying address-to-name queries easier. For example
--rev-server=1.2.3.0/24,192.168.0.1 is exactly equivalent to
--server=/3.2.1.in-addr.arpa/192.168.0.1
It is not mentioned in the man page but specifying anything but /8
or /24 as the CIDR prefix has the same effect as specifying /16.
It is not a big deal for subnets on non-octet boundaries since they
cannot be represented using a single in-addr.arpa address. However,
it is unconvenient for /32 and /0 prefixes while their analogous
server directives behave as expected. E.g. the following server
server=/42.10.168.192.in-addr.arpa/1.2.3.4
server=/in-addr.arpa/1.2.3.4
rev-server=192.168.10.42/32,1.2.3.4
rev-server=192.168.10.42/0,1.2.3.4

The second is a bad example, and to my mind it should not work,
because x.x.x.x/0 is not a valid CIDR expression unless each x=0.
Did you try "rev-server=0.0.0.0/0,1.2.3.4"? From the patch I am
supposing you did and got 0.0.in-addr.arpa as the zone?

Post by o***@sigexec.com
server=/168.192.in-addr.arpa/1.2.3.4
server=/168.192.in-addr.arpa/1.2.3.4
This strange behaviour is fixed by accepting /32 and /0 CIDR
prefixes as valid values. Any other value will still be
considered the same as /16.

A /0 zone is very strange and likely to break most reverse address
resolution, but a /32 zone is not unusual at all; I run 8 /32
in-addr.arpa zones for my /29 netblock.

--
http://rob0.nodns4.us/
Offlist GMX mail is seen only if "/dev/rob0" is in the Subject:

Olivier Gayot

2017-02-15 21:48:26 UTC

Permalink

Post by /dev/rob0

Post by o***@sigexec.com
server=/42.10.168.192.in-addr.arpa/1.2.3.4
server=/in-addr.arpa/1.2.3.4
rev-server=192.168.10.42/32,1.2.3.4
rev-server=192.168.10.42/0,1.2.3.4

You are right, the example is inappropriate. Only 0.0.0.0/0 should be
considered valid as you mentioned.

And yes, I tried with this value and it indeed gives 0.0.in-addr.arpa as
a result.

Post by /dev/rob0

A /0 zone is very strange and likely to break most reverse address
resolution, but a /32 zone is not unusual at all; I run 8 /32
in-addr.arpa zones for my /29 netblock.

You are right again. Having multiple /32 in order to manage subnets with
prefixes greater than /24 is most useful and this is a reason why I came
with this patch. I made the decision to change the behaviour of /0 to
make the model complete but it is definitely something that can be
reverted back to the old behaviour.

Simon Kelley

2017-02-14 15:17:54 UTC

Permalink

That's an improvement, but I tend to agree that /0 doesn't make much
sense. If we're going to patch this, it seems to make more sense to
reject anything other that /32 /24 /16 or /8.

The ideal solution would be to accept any prefix length and generate
the (up to) 256 --server equivalents that it corresponds to. If
you're going to have syntactic sugar, it may as well work for you.

Cheers,

Simon.

[ excerpt from the man page ] The rev-server directive provides a
syntactic sugar to make specifying address-to-name queries easier.
For example --rev-server=1.2.3.0/24,192.168.0.1 is exactly
equivalent to --server=/3.2.1.in-addr.arpa/192.168.0.1
It is not mentioned in the man page but specifying anything but /8
or /24 as the CIDR prefix has the same effect as specifying /16.
It is not a big deal for subnets on non-octet boundaries since
they cannot be represented using a single in-addr.arpa address.
However, it is unconvenient for /32 and /0 prefixes while their
analogous server directives behave as expected. E.g. the following
server=/42.10.168.192.in-addr.arpa/1.2.3.4
server=/in-addr.arpa/1.2.3.4
rev-server=192.168.10.42/32,1.2.3.4
rev-server=192.168.10.42/0,1.2.3.4
server=/168.192.in-addr.arpa/1.2.3.4
server=/168.192.in-addr.arpa/1.2.3.4
This strange behaviour is fixed by accepting /32 and /0 CIDR
prefixes as valid values. Any other value will still be considered
the same as /16.
src/option.c | 31 +++++++++++++++++++++---------- 1 file changed,
21 insertions(+), 10 deletions(-)
diff --git a/src/option.c b/src/option.c index 4a5ef5f..eeca3d6
char *parse_server(char *arg, union mysockaddr *addr, union
mysockaddr *source_a static struct server *add_rev4(struct in_addr
addr, int msize) { struct server *serv = opt_malloc(sizeof(struct
server)); - in_addr_t a = ntohl(addr.s_addr) >> 8; + in_addr_t
a = ntohl(addr.s_addr); char *p;
memset(serv, 0, sizeof(struct server)); - p = serv->domain =
opt_malloc(25); /* strlen("xxx.yyy.zzz.in-addr.arpa")+1 */ - - if
(msize == 24) - p += sprintf(p, "%d.", a & 0xff); - a = a >>
8; - if (msize != 8) - p += sprintf(p, "%d.", a & 0xff); - a =
a >> 8; - p += sprintf(p, "%d.in-addr.arpa", a & 0xff); + p =
serv->domain = opt_malloc(29); /*
strlen("xxx.yyy.zzz.ttt.in-addr.arpa")+1 */ + + switch (msize) +
{ + case 32: + p += sprintf(p, "%d.", a & 0xff); + /*
fall through */ + case 24: + p += sprintf(p, "%d.", (a >>
8) & 0xff); + /* fall through */ + default: + case 16: +
p += sprintf(p, "%d.", (a >> 16) & 0xff); + /* fall through
*/ + case 8: + p += sprintf(p, "%d.in-addr.arpa", (a >> 24)
& 0xff); + break; + case 0: + p += sprintf(p,
"in-addr.arpa"); + }
serv->flags = SERV_HAS_DOMAIN; serv->next = daemon->servers;

Olivier Gayot

2017-02-15 22:46:26 UTC

Permalink

Post by Simon Kelley
That's an improvement, but I tend to agree that /0 doesn't make much
sense. If we're going to patch this, it seems to make more sense to
reject anything other that /32 /24 /16 or /8.
The ideal solution would be to accept any prefix length and generate
the (up to) 256 --server equivalents that it corresponds to. If
you're going to have syntactic sugar, it may as well work for you.

That would be fantastic! I guess that would be up to 128 though.
However it sounds like a much bigger change than what I came up with. It
will add some complexity.

To summerize, after reading your answer and rob0's answer, I think that
there are three things that can be addressed:

- The first one being that /32 is not considered a valid value in the
rev-server directive. And it would really be useful if it were. (That
was the purpose of my patch in the first place).

- The second one being that values considered invalid (or at least not
considered valid) in the rev-server directive are implicitly converted
to /16. And I think they should be rejected instead (0 included).
Besides, /55555 is also currently converted to /16 without notice.

- And the last thing being a possible improvement: accepting any CIDR in
the range [1; 32]. And indeed we would need to generate multiple
server directives accordingly.

If you agree with the above, Simon, I think that I can quickly come up
with two patches to address the first two issues. Would that be okay for
you as a first step ?

Kind regards,

Olivier

Simon Kelley

2017-02-19 18:03:48 UTC

Permalink

Post by Olivier Gayot

Post by Simon Kelley
That's an improvement, but I tend to agree that /0 doesn't make
much sense. If we're going to patch this, it seems to make more
sense to reject anything other that /32 /24 /16 or /8.
The ideal solution would be to accept any prefix length and
generate the (up to) 256 --server equivalents that it
corresponds to. If you're going to have syntactic sugar, it may
as well work for you.

That would be fantastic! I guess that would be up to 128 though.
However it sounds like a much bigger change than what I came up
with. It will add some complexity.
To summerize, after reading your answer and rob0's answer, I think
- The first one being that /32 is not considered a valid value in
the rev-server directive. And it would really be useful if it were.
(That was the purpose of my patch in the first place).
- The second one being that values considered invalid (or at least
not considered valid) in the rev-server directive are implicitly
converted to /16. And I think they should be rejected instead (0
included). Besides, /55555 is also currently converted to /16
without notice.
- And the last thing being a possible improvement: accepting any
CIDR in the range [1; 32]. And indeed we would need to generate
multiple server directives accordingly.
If you agree with the above, Simon, I think that I can quickly come
up with two patches to address the first two issues. Would that be
okay for you as a first step ?

It would certainly be OK, and if you could also then do number three,
I'd be happy to have that patch too.

Cheers,

Simon.

Post by Olivier Gayot
Kind regards,
Olivier

Olivier Gayot

2017-03-05 10:13:02 UTC

Permalink

The rev-server directive only handles the following CIDR prefixes
properly: /8, /16, /24, /32.

Any other value was silently converted to /16 which could result in
unexpected behaviour.

This patch rejects any other value instead of making a silent
conversion.

Signed-off-by: Olivier Gayot <***@sigexec.com>
---
src/option.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/option.c b/src/option.c
index 548560c..0e1beb0 100644
--- a/src/option.c
+++ b/src/option.c
@@ -864,13 +864,14 @@ static struct server *add_rev4(struct in_addr addr, int msize)
case 24:
p += sprintf(p, "%d.", (a >> 8) & 0xff);
/* fall through */
- default:
case 16:
p += sprintf(p, "%d.", (a >> 16) & 0xff);
/* fall through */
case 8:
p += sprintf(p, "%d.", (a >> 24) & 0xff);
break;
+ default:
+ return NULL;
}

p += sprintf(p, "in-addr.arpa");
@@ -2078,6 +2079,9 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
/* generate the equivalent of
local=/xxx.yyy.zzz.in-addr.arpa/ */
struct server *serv = add_rev4(new->start, msize);
+ if (!serv)
+ ret_err(_("bad prefix"));
+
serv->flags |= SERV_NO_ADDR;

/* local=/<domain>/ */
@@ -2449,7 +2453,11 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
ret_err(gen_err);

if (inet_pton(AF_INET, arg, &addr4))
- serv = add_rev4(addr4, size);
+ {
+ serv = add_rev4(addr4, size);
+ if (!serv)
+ ret_err(_("bad prefix"));
+ }
#ifdef HAVE_IPV6
else if (inet_pton(AF_INET6, arg, &addr6))
serv = add_rev6(&addr6, size);

--
2.12.0

Simon Kelley

2017-03-06 22:18:40 UTC

Permalink

Both patches applied.

Cheers,

Simon.

Post by Olivier Gayot
The rev-server directive only handles the following CIDR prefixes
properly: /8, /16, /24, /32.
Any other value was silently converted to /16 which could result in
unexpected behaviour.
This patch rejects any other value instead of making a silent
conversion.

Olivier Gayot

2017-03-05 10:13:01 UTC

Permalink

[ excerpt from the man page ]
The rev-server directive provides a syntactic sugar to make specifying
address-to-name queries easier. For example
--rev-server=1.2.3.0/24,192.168.0.1 is exactly equivalent to
--server=/3.2.1.in-addr.arpa/192.168.0.1

It is not mentioned in the man page but the only prefixes that the
directive properly handles when dealing with IPv4 are /8, /16 and /24.
Specifying anything else as the same effect as specifying /16.

It is not a big deal for subnets on non-octet boundaries since they
cannot be represented using a single in-addr.arpa address. However, it
is unconvenient for /32 prefix while the analogous server directive
behaves as expected. E.g. the following server directive work
as expected:

server=/42.10.168.192.in-addr.arpa/1.2.3.4

but the following does not:

rev-server=192.168.10.42/32,1.2.3.4

and, in practice, the later behaves the same as:

server=/168.192.in-addr.arpa/1.2.3.4

This strange behaviour is fixed by accepting /32 CIDR prefixes as a
valid value. Any other value will still be considered the same as /16.

Signed-off-by: Olivier Gayot <***@sigexec.com>
---
src/option.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/option.c b/src/option.c
index 4a5ef5f..548560c 100644
--- a/src/option.c
+++ b/src/option.c
@@ -850,19 +850,30 @@ char *parse_server(char *arg, union mysockaddr *addr, union mysockaddr *source_a
static struct server *add_rev4(struct in_addr addr, int msize)
{
struct server *serv = opt_malloc(sizeof(struct server));
- in_addr_t a = ntohl(addr.s_addr) >> 8;
+ in_addr_t a = ntohl(addr.s_addr);
char *p;

memset(serv, 0, sizeof(struct server));
- p = serv->domain = opt_malloc(25); /* strlen("xxx.yyy.zzz.in-addr.arpa")+1 */
-
- if (msize == 24)
- p += sprintf(p, "%d.", a & 0xff);
- a = a >> 8;
- if (msize != 8)
- p += sprintf(p, "%d.", a & 0xff);
- a = a >> 8;
- p += sprintf(p, "%d.in-addr.arpa", a & 0xff);
+ p = serv->domain = opt_malloc(29); /* strlen("xxx.yyy.zzz.ttt.in-addr.arpa")+1 */
+
+ switch (msize)
+ {
+ case 32:
+ p += sprintf(p, "%d.", a & 0xff);
+ /* fall through */
+ case 24:
+ p += sprintf(p, "%d.", (a >> 8) & 0xff);
+ /* fall through */
+ default:
+ case 16:
+ p += sprintf(p, "%d.", (a >> 16) & 0xff);
+ /* fall through */
+ case 8:
+ p += sprintf(p, "%d.", (a >> 24) & 0xff);
+ break;
+ }
+
+ p += sprintf(p, "in-addr.arpa");

serv->flags = SERV_HAS_DOMAIN;
serv->next = daemon->servers;

--
2.12.0

Dave Taht

2017-02-19 20:14:08 UTC

Permalink

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
That's an improvement, but I tend to agree that /0 doesn't make much
sense. If we're going to patch this, it seems to make more sense to
reject anything other that /32 /24 /16 or /8.
The ideal solution would be to accept any prefix length and generate
the (up to) 256 --server equivalents that it corresponds to. If
you're going to have syntactic sugar, it may as well work for you.

+1 on accepting any size netmask universally. I am perpetually getting
/48s... or /56s... or /60s or /61s, /62s or /63 and prefix splitting
in shell to generate the appropriate things for rev-server, etc, is a
PITA. I'm sitting here trying to remember which one of these two broke
with different sized netmasks....

synth-domain=ula.taht.net,fd32:7d58:8d63::/64,ula-
rev-server=fd32:7d58:8d63::/48,fd32:7d58:8d63::1

And I did look at the code for both and was too dumb to figure out how
to apply the technique universally.

Cheers,
Simon.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBCAAGBQJYox+hAAoJEBXN2mrhkTWiBSUQAJQxq6yD3Tcw/39On0QcLcfy
aZTerALrzgrwlpyH4yVWeztrA3pFK1uVLIhnQP161zR+NJRI5Bo0J7tAOElETm3k
QQ0HEu16skPHdmzlmbR1MMLoVKn4myh6hDm5iFSAf+jsCpItAzYo5Cy9/oAz3PNU
Hp1SKxYNwrcSTw5FQrLpNuhZxbqvkA5KiU3URcnzv3mW2YbMzcjrULL7hF7xfN0t
2iwI8shObm/3FMDux7jX1wuRPQALWokaXFhAyOTDUVBONavA4W/PxS+8VvV4noi4
dFh6FMhJYPggUh7v02PTOoPtSuvaLNGt1vgWeHt1sqTXEo6rVsft085fKwH1uONw
SGWrGhnFaVDewHeEoB46K6qg7LYSoLa1cgv8li8QJ9ZTSiFC7ZqIWsXBQ5oqlGzr
0iR6jo1yqISvwyek8nogsgNWI4zx/mmC1AXhR/OjE8Y/3MA87rhpY+t/U2ZJug5e
f611DvKCl4iQuL/EyWY7hCIK7XCHi4ACx7sosN21zgL2/ToLshaF7i3rcYC6F/Bx
5GGgv6x6WiXWRMk82YiqcEphnOdphsWen4ZMHTdlBIzZ1EXpD5XwhDHTzzmD3SlT
oNjwPR1Gmkt1yXxLSvvr6mp7XFRDQOJMWDHvmfroH4p2hcxyB/2dSbhLjrfri0nL
WsjDDAhdIM1aHokLmLqi
=mtD9
-----END PGP SIGNATURE-----
_______________________________________________
Dnsmasq-discuss mailing list
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

--
Dave Täht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org