Potential Bug while parsing port from the "stub-host" string

[hardik-tharad]

Describe the bug
Race condition while trying to replace '@' with '0' character and back again with '@' in stub-host string which is of the form "nameserver@port"

This is what the bug looks like -

1. Unbound creates background workers (threads) on starting up with a pointer to ctx (call by reference) -

   unbound/libunbound/libworker.h

   Line 101 in 0756196

   int libworker_bg(struct ub_ctx* ctx);

   .
2. Each background worker reads the ctx to parse the stub-host in forward stub zones.
3. The code flow ultimately reaches here where the parsing logic is present -

   unbound/util/net_help.c

   Line 481 in 0756196

   uint8_t* authextstrtodname(char* str, int* port, char** auth_name)

4. In this implementation it replaces the '@' in the original 'stub-host' string with a null character and then parses for the domain name and again replaces null with '@' -

   unbound/util/net_help.c

   Line 502 in 0756196

   *s = 0;

5. Now consider a scenario where 2 threads are doing the same thing.
6. In a particular race condition where the stub-host string is ''xyz@7171" - Thread 1 will replace '@' with null, parse domain as "xyz". Thread 2 will also replace '@' with null, but by the time it tries to parse the domain name, Thread 1 has already replaced null with '@' again, therefore Thread 2 parses domain name as "xyz@7171".

To reproduce
Steps to reproduce the behavior:

I have attached a snippet of the unbound config with this issue - unbound-conf.txt

Number of threads on which I am running unbound is 4.

1. Keep on triggering "unbound-control reload" and randomly wait for the above race condition to occur.

Expected behavior
Unbound trying to query nameserver with port attached to it rather than just domain string.

System:

• Unbound version: 1.17.1
• OS: Ubuntu 20.04.6 LTS (GNU/Linux 5.15.0-1055-aws aarch64)
• unbound -V output:

Version 1.17.1

Configure line: --build=aarch64-linux-gnu --prefix=/usr --includedir=${prefix}/include --mandir=${prefix}/share/man --infodir=${prefix}/share/info --sysconfdir=/etc --localstatedir=/var --disable-silent-rules --libdir=${prefix}/lib/aarch64-linux-gnu --runstatedir=/run --disable-maintainer-mode --disable-dependency-tracking --with-pythonmodule --with-pyunbound --enable-subnet --enable-dnstap --enable-systemd --with-libnghttp2 --with-chroot-dir= --with-dnstap-socket-path=/run/dnstap.sock --disable-rpath --with-pidfile=/run/unbound.pid --with-libevent --enable-tfo-client --with-rootkey-file=/usr/share/dns/root.key --disable-flto --enable-tfo-server
Linked libs: libevent 2.1.11-stable (it uses epoll), OpenSSL 1.1.1f 31 Mar 2020
Linked modules: dns64 python subnetcache respip validator iterator
TCP Fastopen feature available

BSD licensed, see LICENSE in source package for details.
Report bugs to unbound-bugs@nlnetlabs.nl or https://github.com/NLnetLabs/unbound/issues

Additional information
Add any other information that you may have gathered about the issue here.

[gthess]

The commit should fix the issue. It uses a safe copy of the data before manipulating the buffer; same way as similar functions in the net_help.c file. At least I can't reproduce it anymore locally.
Thanks for reporting!