<div dir="ltr"><div dir="ltr"><div>Simon, thank you for such a quick and detailed reply! I understood everything. Your commit is also very appreciated.<br></div>Have a nice day!<br></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Tue, Jul 15, 2025 at 5:45 PM Simon Kelley <<a href="mailto:simon@thekelleys.org.uk">simon@thekelleys.org.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 7/11/25 13:31, Mikhail Dmitrichenko wrote:<br>
> Hello!<br>
> <br>
> First of all, I want to clarify that I'm new to dnsmasq, so please <br>
> excuse me if my message seems silly.<br>
> <br>
> My question is about constructing srv record name in function src/ <br>
> option.c/read_opts, v2.91. Specifically, this part:<br>
> <br>
>    if (daemon->domain_suffix)<br>
>      {<br>
>         /* add domain for any srv record without one. */<br>
>        struct mx_srv_record *srv;<br>
> <br>
>        for (srv = daemon->mxnames; srv; srv = srv->next)<br>
>      if (srv->issrv &&<br>
>        strchr(srv->name, '.') &&<br>
>        strchr(srv->name, '.') == strrchr(srv->name, '.'))<br>
>       {<br>
>        strcpy(buff, srv->name);<br>
>        strcat(buff, ".");<br>
>        strcat(buff, daemon->domain_suffix);<br>
>        free(srv->name);<br>
>        srv->name = opt_string_alloc(buff);<br>
>       }<br>
>      }<br>
> <br>
> Here, `buff` is a buffer of length ((MAXDNAME * 2) + 1) bytes. If I <br>
> understand it correctly, after lines:<br>
> <br>
>     strcpy(buff, srv->name);<br>
>     strcat(buff, ".");<br>
> <br>
> maximum len of `buff` will be (MAXDNAME + 1) bytes. My concern is about <br>
> next line:<br>
> <br>
>     strcat(buff, daemon->domain_suffix);<br>
> <br>
> As I see it, there is a possible scenario, where domain name in <br>
> `resolv.conf` contains non-ASCII symbols. If IDN library is present, <br>
> this name will be encoded and set as `daemon->domain_suffix` value. I <br>
> noticed that in function src/util.c/canonicalise, there is a call to <br>
> `check_name`, which checks len of given non-encoded input string. If it <br>
> exceeds `MAXDNAME`, NULL will be returned, so `daemon->domain_suffix` <br>
> will be NULL.<br>
> <br>
> But what if encoded string exceeds `MAXDNAME`? I haven't found any check <br>
> for that. If this happens, it could cause overflow in the line<br>
> <br>
>     strcat(buff, daemon->domain_suffix);<br>
> <br>
> because `buff` may already contain (`MAXDNAME` + 1) bytes, leaving only <br>
> `MAXDNAME` bytes free. Do you think it will be useful to add a check of <br>
> encoded domain name length to prevent such an overflow? Any response <br>
> would be appreciated.<br>
> <br>
> I'm aware that such long domain names are unusual and described scenario <br>
> is unlikely in real life, but I still wanted to highlight it.<br>
> <br>
> Thank you in advance for your time and expertise!<br>
<br>
<br>
First, the code that appends the domain to SRV records. This is a good <br>
point, but note that the both the domain and the name of SRV records <br>
come from the configuration of dnsmasq, they are not untrusted data from <br>
the internet. The anyone who can manipulate them has root on the machine <br>
running dnsmasq, so this it not a security problem. The worst case <br>
scenario is that a somewhat bizarre configuration causes dnsmasq to crah <br>
on startup. The fix is to check for overflow, and exit with an suitable <br>
error message, so it's easy to find and correct.<br>
<br>
<br>
Aside: the buffer length is (MAXDNAME*2)+1 because dnsmasq uses an <br>
escape method to represent a few special characters in domain names. For <br>
instance '.' could appear in a valid label, but dnsmasq uses '.' in the <br>
obvious way as a label delimited. '.' in a label is therefore <br>
represented as the escape byte followed by a second byte. In theory, and <br>
name could require an escape character for each character, and a domain <br>
name is limited to MAXDNAME characters, hence the MAXDNAME * 2. The <br>
added one is for the zero terminator.<br>
<br>
Second, the problem with returned encoded names from libidn - the <br>
documentation for idn2 states that domain names are limited to 255 <br>
characters, which is less that MAXDNAME - I'm not sure why. libida is <br>
even more restrictive and 63 characters. I don't think that this a<br>
therefore  a problem.<br>
<br>
I've pushed a commit to make the check on the first issue.<br>
<br>
Cheers,<br>
<br>
Simon.<br>
<br>
> <br>
> _______________________________________________<br>
> Dnsmasq-discuss mailing list<br>
> <a href="mailto:Dnsmasq-discuss@lists.thekelleys.org.uk" target="_blank">Dnsmasq-discuss@lists.thekelleys.org.uk</a><br>
> <a href="https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss" rel="noreferrer" target="_blank">https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss</a><br>
<br>
<br>
_______________________________________________<br>
Dnsmasq-discuss mailing list<br>
<a href="mailto:Dnsmasq-discuss@lists.thekelleys.org.uk" target="_blank">Dnsmasq-discuss@lists.thekelleys.org.uk</a><br>
<a href="https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss" rel="noreferrer" target="_blank">https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss</a><br>
</blockquote></div></div>