upsd: Misprint of bound socket address and possible buffer overflow #2914
Description
Background
In server/upsd.c there is a warning log statement that is issued in the case that the daemon has been asked to bind to a network interface by way of a hostname that resolves to more than one IPv4/IPv6 address.
Summary
The code that constructs the warning message has two issues:
- It incorrectly calculates the available buffer size when assembling the string, causing a possible buffer overflow.
- It attempts to produce a human-readable representation of the address it has bound to, but produces garbled results on some operating systems.
Details
The code in question is at
Line 509 in 0264b1b
At the sake of brevity, here is a copy of it for easier discussion:
509 if (ai->ai_next) {
510 char ipaddrbuf[SMALLBUF];
511 const char *ipaddr;
512 snprintf(ipaddrbuf, sizeof(ipaddrbuf), " as ");
513 ipaddr = inet_ntop(ai->ai_family, ai->ai_addr,
514 ipaddrbuf + strlen(ipaddrbuf),
515 sizeof(ipaddrbuf));
516 upslogx(LOG_WARNING,
517 "setuptcp: bound to %s%s but there seem to be "
518 "further (ignored) addresses resolved for this name",
519 server->addr,
520 ipaddr == NULL ? "" : ipaddrbuf);
521 }Buffer overflow
The buffer overflow issue occurs at line 515, where the remaining buffer size is computed as sizeof(ipaddrbuf) when it should be sizeof(ipaddrbuf) - strlen(ipaddrbuf). The latter expression adjusts for the fact that the starting address of the position in the buffer that will receive result has been increased by the same amount.
Malformed result
The malformed output issue is caused by a mismatch between the type that is being passed to inet_ntop versus what type it actually is designed to take.
inet_ntop is designed to take a pointer to a network address, (e.g. struct in_addr or struct in6_addr) not a pointer to a full socket address (struct sockaddr), which is the type of ai->ai_addr in this call. As such, the call to inet_ntop on line 513 produces garbled results on systems where the network address portion of a socket address is not the first member of the structure.
To Reproduce
Consider the following scenario, on a FreeBSD 13.5 system. With a network interface igb0 with addresses:
2600:1234:5678:aaaa::1
192.168.1.1
And an /etc/hosts file with the contents:
2600:1234:5678:aaaa::1 router.inside
192.168.1.1 router.inside
Expected Result
Asking upsd to LISTEN router.inside should produce the warning:
router upsd[9915]: setuptcp: bound to router.inside as 2600:1234:5678:aaaa::1 but there seem to be further (ignored) addresses resolved for this name
Actual Result
Instead, the daemon produces this actual warning, with a confusing address:
router upsd[9915]: setuptcp: bound to router.inside as 1c1c:da5::2600:1234:5678:aaaa but there seem to be further (ignored) addresses resolved for this name
TL;DR: 2600:1234:5678:aaaa::1 (good) versus 1c1c:da5::2600:1234:5678:aaaa (bad).
Deeper discussion
This is garbled because inet_ntop is picking up unintended address material from the front of FreeBSD's sockaddr_in6 structure, namely, the members, sin6_len, sin6_family, sin6_port and sin6_flowinfo from the structure below. Instead, it should have only been passed a pointer to the sin6_addr member itself.
struct sockaddr_in6 {
uint8_t sin6_len; /* length of this struct */
sa_family_t sin6_family; /* AF_INET6 */
in_port_t sin6_port; /* Transport layer port # */
uint32_t sin6_flowinfo; /* IP6 flow information */
struct in6_addr sin6_addr; /* IP6 address */
...
Possible Solutions
server/upsd.c already contains a helper function, inet_ntopW which performs the correct logic to get the address printed correctly. Perhaps that function should be used instead, and the string logic be cleaned up to perform the concatenation entirely within the call to upslogx(). In fact, I have a patch which does just this.
