[Dnsmasq-discuss] possible overflow while constructing SRV record name

[Mikhail Dmitrichenko]

Simon, thank you for such a quick and detailed reply! I understood
everything. Your commit is also very appreciated.
Have a nice day!

On Tue, Jul 15, 2025 at 5:45 PM Simon Kelley <simon at thekelleys.org.uk>
wrote:

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