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 <[email protected]> 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 > > [email protected] > > https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss > > > _______________________________________________ > Dnsmasq-discuss mailing list > [email protected] > https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss >
_______________________________________________ Dnsmasq-discuss mailing list [email protected] https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
