Subject: Re: bin/21205: Potential buffer overrun in debug code of named
To: None <netbsd-bugs@netbsd.org>
From: David Laight <david@l8s.co.uk>
List: netbsd-bugs
Date: 04/16/2003 22:28:38
> >Synopsis: Potential buffer overrun in debug code of named
> dist/bind/bin/named/ns_maint.c:
>
> 905 int len;
> 906
> 907 curr = buffer;
> 908 last = &buffer[sizeof buffer - 1]; /* leave room for \0 */
> 909 for (i = 0; i < argc; i++) {
> 910 len = strlen(argv[i]);
> 911 if (curr + len + 1 >= last) {
>
> I don't think this check is sufficient. Is there any guarantee that
> `curr + len + 1' doesn't overflow the pointer and point to 0x0000CAFE?
> This would probably lead to a crash during the strncpy() later. Even
> a second check for >= curr (or buffer) isn't correct. The latter might
> be far more theoritical than the first issue, though.
>
> 912 ns_debug(ns_log_xfer_in, 1,
> 913 "xfer args debug printout truncated");
> 914 break;
> 915 }
> 916 strncpy(curr, argv[i], len);
> 917 curr += len;
> 918 *curr = ' ';
> 919 curr++;
> 920 }
> 921 *curr = '\0';
Looks safe to me....
For it to go wrong the sum 'curr + len + 1' would have to wrap.
Now the top of the processes address space is used for kernel
addresses (typically 0.5GB or 1GB) and 'len' is limited by the
amount of space available for process arguments.
Provided the single argument is less that the amount of address
space reserved for the kernel then everything is fine.
OTOH a strlcpy() based function would be simpler.
curr = buffer;
left = sizeof buffer - 1; /* -1 for the spaces */
for (i = 0; i < argc; i++) {
len = strlcpy(curr, argv[i], left);
if (len > left) {
ns_debug(...);
break;
}
curr += len;
left -= len;
*cur++ = ' ';
}
*curr = 0;
David
--
David Laight: david@l8s.co.uk