search

pi-hole / web

Pi-hole Dashboard for stats and more
https://pi-hole.net
Other
2.01k stars 556 forks source link

Invalid domain names are parsed incorrectly #1581

Open m33x opened 4 years ago

m33x commented 4 years ago

Versions

Platform

Expected behavior

Invalid hostnames that include a <space> should be displayed correctly.

Actual behavior / bug

Hostnames are automatically split by <space> thus the invalid hostname "hello world" results in the Domain "hello" and the Client "world".

I explored the issue and found the bug here.

pihole.log contains:

Sep 10 16:48:04 dnsmasq[2904]: query[A] hello world from 10.0.0.100
Sep 10 16:48:04 dnsmasq[2904]: config hello world is NODATA-IPv4

FTL returns valid data:

>getallqueries
1599752884 A hello world imac_lan 3 0 1 27 N/A -1
1599752785 A docs.google.com imac_lan 2 2 4 186 N/A -1

api.php?getAllQueries returns: ["1599752884","A","hello","world","imac_lan","3","0","1","27","N\/A","-1"]

Steps to reproduce

Steps to reproduce the behavior:

dig @pihole-ip 'hello world'

Screenshots

Screenshot 2020-09-10 at 17 48 32
dschaper commented 4 years ago

Glad Pi-hole could help you identify the misbehaving client. I assume everything works when the client is sending the proper name?

m33x commented 4 years ago

Absolutely, all valid requests are displayed correctly.

dschaper commented 4 years ago

I'm not sure if this is something we'll be able to address. @DL6ER That looks like it's just how dnsmasq operates?

m33x commented 4 years ago

I guess the easy fix is to improve the parsing by not relying on the <space> but on some other structure within the message. An alternative is to change the data format from Array to Dict this way a confusion cannot happen. Instead of data[2] = Domain you should do data['domain'] = Domain.

DL6ER commented 4 years ago

This will not solve the issue because it happens in FTL as you said:

>getallqueries
1599752884 A hello world imac_lan 3 0 1 27 N/A -1
1599752785 A docs.google.com imac_lan 2 2 4 186 N/A -1

Here the space separation is the issue. Behind the scenes* we're working on Pi-hole v6.0 which will change the format out of FTL from our legacy telnet-like format to valid JSON, replacing the entire PHP API. This will solve the issue you've found at the same time.

*) actually, this is not hidden at all but a public branch new/http on the FTL repo

fhriley commented 3 years ago

Note that requests with spaces in them are valid requests, which means pi-hole is not handling them properly. This is not uncommon in Bonjour (DNS-SD). For example, I have these TXT requests on my network: HP LaserJet Pro._ipp._tcp.example.com. A DNS-SD service instance name is this: Service Instance Name = <Instance> . <Service> . <Domain> where <Instance> may contain spaces.

Relevant RFC: https://tools.ietf.org/html/rfc6763#section-4.1.1

dschaper commented 3 years ago

Please review the RFCs governing the valid characters used in internet hostnames. Spaces are not valid chars.

https://en.wikipedia.org/wiki/Hostname

dschaper commented 3 years ago

https://tools.ietf.org/html/rfc3696#section-2

  1. Restrictions on domain (DNS) names

    The authoritative definitions of the format and syntax of domain names appear in RFCs 1035 [RFC1035], 1123 [RFC1123], and 2181 [RFC2181].

    Any characters, or combination of bits (as octets), are permitted in DNS names. However, there is a preferred form that is required by most applications. This preferred form has been the only one permitted in the names of top-level domains, or TLDs. In general, it is also the only form permitted in most second-level names registered in TLDs, although some names that are normally not seen by users obey other rules. It derives from the original ARPANET rules for the naming of hosts (i.e., the "hostname" rule) and is perhaps better described as the "LDH rule", after the characters that it permits. The LDH rule, as updated, provides that the labels (words or strings separated by periods) that make up a domain name must consist of only the ASCII [ASCII] alphabetic and numeric characters, plus the hyphen. No other symbols or punctuation characters are permitted, nor is blank space. If the hyphen is used, it is not permitted to appear at either the beginning or end of a label. There is an additional rule that essentially requires that top-level domain names not be all- numeric.

fhriley commented 3 years ago

I did review the relevant RFC, and I linked it above. I'll summarize the relevant bits:

This document specifies that a particular service instance can be described using a DNS SRV [RFC2782] and DNS TXT [RFC1035] record. The SRV record has a name of the form "<Instance> . <Service> . <Domain>" and gives the target host and port where the service instance can be reached. The DNS TXT record of the same name gives additional information about this instance, in a structured form using key/value pairs, described in Section 6. A client discovers the list of available instances of a given service type using a query for a DNS PTR [RFC1035] record with a name of the form "Service.Domain", which returns a set of zero or more names, which are the names of the aforementioned DNS SRV/TXT record pairs.

The result of this PTR lookup for the name "." is a set of zero or more PTR records giving Service Instance Names of the form:

Service Instance Name = <Instance> . <Service> . <Domain>

The <Instance> portion of the Service Instance Name is a user- friendly name consisting of arbitrary Net-Unicode text [RFC5198]. It MUST NOT contain ASCII control characters (byte values 0x00-0x1F and 0x7F) [RFC20] but otherwise is allowed to contain any characters, without restriction, including spaces, uppercase, lowercase, punctuation -- including dots -- accented characters, non-Roman text, and anything else that may be represented using Net-Unicode.

(emphasis mine)

Since the <Instance> can contain spaces, the TXT query to retrieve the key/value pairs for the DNS-SD service can have a query name with spaces in it. As noted in this issue, pi-hole does not handle this correctly.

fhriley commented 3 years ago

https://tools.ietf.org/html/rfc3696#section-2

  1. Restrictions on domain (DNS) names

Correct. Domain names cannot contain spaces. As you can see in the DNS-SD RFC I linked, DNS-SD services have instance names tacked onto the front of the domain. Your RFC does not apply to the instance name.

dschaper commented 3 years ago

Because Pi-hole is not mDNS. Bonjour and friends are serverless. They use multicast for discovery. Pi-hole is not part of that process.

fhriley commented 3 years ago

My mac is querying DNS for the TXT records to get the printer options. Pi-hole does not display those queries properly. The fact is TXT queries can have spaces in them. If the answer is pi-hole can't display TXT records properly, fine. But if the answer is pi-hole should work properly, it currently doesn't.

dschaper commented 3 years ago

Pi-hole works fine. You're asking for Pi-hole to be aware of mDNS, which is a separate thing. Please don't hijack unrelated topics with unrelated posts.

fhriley commented 3 years ago

DNS-SD uses SRV and TXT records. Read the RFC linked above. This is 100% related to pi-hole can't display those queries properly and not hijacking. How on earth can you not see this?

dschaper commented 3 years ago

This is the web interface repo. If you want to throw a hissy fit then do it on the FTL repo.

fhriley commented 3 years ago

This bug exists in both the web interface and the FTL api (neither can handle it properly). Are you suggesting I file a bug against FTL?

dschaper commented 3 years ago

Last comment and then I'm locking and hiding your post.

This isn't the repo to complain that Pi-hole doesn't do multicast DNS.

fhriley commented 3 years ago

How very grown up of you.