Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolvconf: correctly parse IPv6 Scoped Address in nameserver #328

Closed
wants to merge 2 commits into from

Conversation

bikallem
Copy link

@bikallem bikallem commented Nov 29, 2022

This PR improves resolvconf parser error message so that it is helpful when trying to fix resolv.conf related syntax errors. The improved error message is provided in the test. Without this PR, the error message is lexing: empty token which isn't very helpful.

I experienced this issue while attempting to use my local resolv.conf(NixOs 22.05) which content is as follows:

# Generated by resolvconf
search home
nameserver 192.168.1.254
nameserver fe80::c2d7:aaff:fe96:8d82%wlp3s0
options edns0

Note the % char in line 4 which is not recognized by the lexer/parser. I am still investigating if my OS generated resolv.conf is valid or not. Regardless I think this PR helps in resolving similar issue that I faced.

This commit improves resolvconf parser error message so that it is
helpful when trying to fix resolv.conf related syntax errors.
The improved error message is provided in the test.
This commit ensures that the resolv.conf parser doesn't error out
when it encounters a zone index in an IPv6 nameserver. This is similar
to the approach taken by glibc reslov_conf implementation - https://github.com/bminor/glibc/blob/master/resolv/res_init.c#L422 and https://github.com/bminor/glibc/blob/227df6243a2b5b4d70d11772d12c02eb9cb666ca/resolv/netdb.h
@bikallem
Copy link
Author

linux resolv.conf supports IPv6 Scoped Address Architecture (https://datatracker.ietf.org/doc/html/rfc4007#section-11) which is where the % character comes from.

I have updated the resolve_conf parser to not error out when encountering zone index tokens. Instead we just discard them as glibc resolv.conf implemenetation does. (https://github.com/bminor/glibc/blob/master/resolv/res_init.c#L422)

/cc @hannesm

This PR is ready for review.

@bikallem bikallem changed the title resolvconf: improve parser error message resolvconf: correctly parse IPv6 Scoped Address in nameserver Nov 30, 2022
@hannesm
Copy link
Member

hannesm commented Dec 1, 2022

Hello,

thanks for working on this PR. I'm curious about (a) why to reject / discard that %yyy in the lexer? and (b) the handling of newlines is vastly different from the other lexer (in the zone subdir) - why? can we - at least in this repository - be consistent about the lexer usage (I don't know which way is "correct", since the other lexer was taken from the earlier DNS implementation).

Semantically, I wonder what the desired "scoped IPv6" nameserver entry should be? Should that nameserver only ever be asked via the provided interface name (in your example)? I'm rather confused why your resolv.conf contains such a scope, and wonder whether dropping it could lead to information leakage (since ocaml-dns may use a different interface than the given one to conduct DNS requests).

@bikallem
Copy link
Author

bikallem commented Dec 1, 2022

I'm curious about (a) why to reject / discard that %yyy in the lexer?

It is part of IPv6 standard rather than resolv.conf, i.e fe80::1ff:fe23:4567:890a%eth2 is a valid IPv6 address. For now we discard the IPv6 scoped literal since Ipaddr can't parse it. There is existing issue for it in mirage/ocaml-ipaddr#76. Regardless, from my initial understanding, the use case for IPv6 scoped literal is that it is used mostly in the OS/kernel/IP protocol level rather than the dns particulars(this repo).

Secondly, the resolv.conf implementation as it exists in glibc currently also discards this information when parsing /etc/resolv.conf as linked to in my previous comment. So there is a prior art sort of reason for discarding this data, i.e. since glibc does it and we do it, we are probably okay.

Finally, the current dns-client information functions okay without this data.

why? can we - at least in this repository - be consistent about the lexer usage (I don't know which way is "correct", since the other lexer was taken from the earlier DNS implementation).

I am not sure if there is an accepted standard way to handle newlines in ocamllex. Most likely a particular use case demands we handle newlines in a particular way. In the zone lexer, a data structure parserstate maintains the lineinfo (

mutable lineno : int ;
). As such it handles it accordingly. However, for the resolvconf use case, we don't define such data structure and use the one available from Lexing.lexbuf - which is adequate for the use case.

@bikallem
Copy link
Author

bikallem commented Dec 1, 2022

A bit outside of the scope of this PR, but I think the resolvconf parser is bit superfluous in that we lex/parse token that we are not interested in, i.e. we only seem to need the nameserver ... bit from resolv.conf but we seem to generate tokens for a lot more. These can be safely removed. Wdyt? Is there an appetite for this?

@hannesm
Copy link
Member

hannesm commented Dec 1, 2022

Secondly, the resolv.conf implementation as it exists in glibc currently also discards this information

That's not leading anywhere. Yes indeed there is lots of software which behaves strangely/weirdly. I'm not keen on adapting the behaviour based on random libc implementations (yes indeed there are others than glibc, e.g. FreeBSD libc and OpenBSD libc, but also musl libc, ...). If there is a speciication how to deal with "scoped addresses", we should follow that specification. I haven't had time to read the RFC 4007 and intended behaviour.

Was there a reason for your Linux distribution to embed the scoped identifier into /etc/resolv.conf? If yes, why? If no, why did they put it there?

Finally, the current dns-client information functions okay without this data.

As I tried to outline, this is fine for academic software, but for potentially security-relevant (leaking hostnames on an unintended network interface) pieces of software, I'd like to have a better argument than "it works fine".

which is adequate for the use case

Maybe. But maybe from a maintenance point of view, this is a burden if these lexers and parsers are behaving differently. Of course you may argue that "it works fine", but tbh I'd prefer some streamline in this repository where also in 5 years time I can easily wrap my head around what is happening.

but we seem to generate tokens for a lot more. These can be safely removed. Wdyt? Is there an appetite for this?

Maybe have a read through the old issues, such as #243 -- there's some interest in handling more features. What would be the benefit from removing it? Also, the comment in the lexer, "(* inspired by https://github.com/tailhook/resolv-conf/blob/master/src/grammar.rs *)", may be worth a look -- since there is no POSIX (or RFC) specification for /etc/resolv.conf, the easiest for me is to keep in sync with a grammar other languages have developed.

This also leads to the question: how does the rust implementation handle scoped addresses?

@hannesm
Copy link
Member

hannesm commented Feb 15, 2023

Coming back to this issue, I'd appreciate to fix this in ocaml-ipaddr instead. There's some active discussion about the presentation of an Ipaddr.V6.t mirage/ocaml-ipaddr#113 (comment)

@bikallem
Copy link
Author

I did some more research on this topic recently.:

  1. I am unable to conclude that zone index information should be part of an IPv6 address since none of the IPv6 RFCS that I managed to pore through specifies how to store the zone index information in the IPv6 address itself.
  2. The ipv6 parsing libraries for rust, golang and libc all don't accept zone index information, i.e. they error out when attempting to do so.
  3. Zone index are only applicable to link local addresses (fe80), therefore zone index information is not usable outside of the link.
  4. Other projects that has to parse resolv.conf also ignores/strips away the zone index information attached to a "nameserver". In addition to glibc example I cited above, it docker also strips away the zone index information.

References:

  1. https://stackoverflow.com/questions/6248596/how-to-include-ipv6-addresses-with-or-without-zone-indexes-in-uri-for-net-rem
  2. https://github.com/moby/libnetwork/pull/1590/files

hannesm added a commit to hannesm/ocaml-dns that referenced this pull request Feb 15, 2023
…resses

Inspired by the bug report in mirage#328 by @bikallem, test case taken from there
hannesm added a commit to hannesm/ocaml-dns that referenced this pull request Feb 15, 2023
…resses

Inspired by the bug report in mirage#328 by @bikallem, test case taken from there
hannesm added a commit to hannesm/ocaml-dns that referenced this pull request Feb 15, 2023
…resses

Inspired by the bug report in mirage#328 by @bikallem, test case taken from there
@hannesm
Copy link
Member

hannesm commented Feb 15, 2023

Thanks for your investigations. Would you mind to take a look at #334 and let me know what you think of that PR? The main difference is that the zone_id is kept for the parser and discarded there. The lexer is as well improved to know about line numbers.

@hannesm
Copy link
Member

hannesm commented Feb 16, 2023

Thanks a lot, merged #334 which fixes the issue reported here.

@hannesm hannesm closed this Feb 16, 2023
@bikallem bikallem deleted the resolvconf-lex-error branch February 16, 2023 09:33
hannesm added a commit to hannesm/opam-repository that referenced this pull request Feb 16, 2023
…er, dns-mirage, dns-client, dns-client-mirage, dns-client-lwt, dns-cli and dns-certify (7.0.0)

CHANGES:

* BREAKING: dns-client is split into 3 packages: dns-client-lwt,
  dns-client-mirage. If your dune file contains dns-client.lwt, use
  dns-client-lwt now. If your dune file contains dns-client.mirage, use
  dns-client-mirage now (mirage/ocaml-dns#331 @hannesm)
* update to mirage-crypto 0.11.0 API changes and tls 0.16.0 packaging changes
  (mirage/ocaml-dns#331 @hannesm)
* dns-client.resolvconf: add line number to parser (mirage/ocaml-dns#334 @hannesm, inspired by
  mirage/ocaml-dns#328 @bikallem)
* dns-client.resolvconf: allow zone idx (RFC 4007) for IPv6 entries
  (mirage/ocaml-dns#334 @hannesm, inspired by mirage/ocaml-dns#328 @bikallem)
* dns-server.zone: allow zone files without final newline (add a newline to the
  buffer if the last character is not \n) (mirage/ocaml-dns#333 @hannesm)
* dns-client-{lwt,mirage}: do not log when the resolver closed the connection,
  but there are no pending requests (mirage/ocaml-dns#332 @reynir)
* dns-certify: in Dns_certify_mirage use X509.Private_key.of_string, the
  behaviour when both key_data and key_seed is provided changed, and leads to
  an exception now (mirage/ocaml-dns#330 @hannesm)
hannesm added a commit to hannesm/opam-repository that referenced this pull request Feb 16, 2023
…er, dns-mirage, dns-client, dns-client-mirage, dns-client-lwt, dns-cli and dns-certify (7.0.0)

CHANGES:

* BREAKING: dns-client is split into 3 packages: dns-client-lwt,
  dns-client-mirage. If your dune file contains dns-client.lwt, use
  dns-client-lwt now. If your dune file contains dns-client.mirage, use
  dns-client-mirage now (mirage/ocaml-dns#331 @hannesm)
* update to mirage-crypto 0.11.0 API changes and tls 0.16.0 packaging changes
  (mirage/ocaml-dns#331 @hannesm)
* dns-client.resolvconf: add line number to parser (mirage/ocaml-dns#334 @hannesm, inspired by
  mirage/ocaml-dns#328 @bikallem)
* dns-client.resolvconf: allow zone idx (RFC 4007) for IPv6 entries
  (mirage/ocaml-dns#334 @hannesm, inspired by mirage/ocaml-dns#328 @bikallem)
* dns-server.zone: allow zone files without final newline (add a newline to the
  buffer if the last character is not \n) (mirage/ocaml-dns#333 @hannesm)
* dns-client-{lwt,mirage}: do not log when the resolver closed the connection,
  but there are no pending requests (mirage/ocaml-dns#332 @reynir)
* dns-certify: in Dns_certify_mirage use X509.Private_key.of_string, the
  behaviour when both key_data and key_seed is provided changed, and leads to
  an exception now (mirage/ocaml-dns#330 @hannesm)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants