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