Date: Wed, 24 Apr 2002 21:46:51 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: "M. Warner Losh" <imp@village.org> Cc: ken@kdm.org, <obrien@FreeBSD.org>, <cvs-committers@FreeBSD.org>, <cvs-all@FreeBSD.org> Subject: Re: cvs commit: src/lib/libcam camlib.h Message-ID: <20020424212147.Y16026-100000@gamplex.bde.org> In-Reply-To: <20020423.235310.117915219.imp@village.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 23 Apr 2002, M. Warner Losh wrote:
> In message: <20020423220257.A39867@panzer.kdm.org>
> "Kenneth D. Merry" <ken@kdm.org> writes:
> : On Tue, Apr 23, 2002 at 16:58:21 -0700, David E. O'Brien wrote:
> : > obrien 2002/04/23 16:58:21 PDT
> : >
> : > Modified files:
> : > lib/libcam camlib.h
> : > Log:
> : > Do not +1 with MAXPATHLEN.
> :
> : Why not? What about the terminating NUL?
>
> Because MAXPATHLEN *INCLUDES* the terminating NUL. See lots of
> discussions on this topic in various lists in the past.
Nevertheless, the commit is unreviewed and quite broken. It only
changes one instance of MAXPATHLEN+1. This bogotifies cam.3 and
and introduces a 100% deterministic buffer overrun in:
/*
* If the user passed in a path, save it for him.
*/
if (given_path != NULL)
strncpy(device->device_path, given_path, MAXPATHLEN + 1);
else
device->device_path[0] = '\0';
This used to have an off-by-1 error. Now it has an off-by-2 error. The
apparently more serious buffer overrun turns out to be equivalent to the
previous bug, because the buffer is followed by another buffer (or struct
padding), and the other buffer is initialized next:
/*
* If the user passed in a device name and unit number pair, save
* those as well.
*/
if (given_dev_name != NULL)
strncpy(device->given_dev_name, given_dev_name, DEV_IDLEN);
else
device->given_dev_name[0] = '\0';
The worst that can happen is the first buffer is nonterminated (when
strlen(given_path) == MAXPATHLEN && given_dev_name != NULL &&
*given_dev_name != '\0').
Note that the initialization of the second buffer is missing the
off-by-1-error. Both buffers are 1 byte longer than the desired maximum
string length, to leave room for the NUL terminator and presumably to
permit copying strings into them using strncpy() with a size equal to
the maximum string length (the NUL terminator having been initialized
elswhere). The desired maximum string length is just wrong for
device_path. It should be {PATH_MAX} -1, not MAXPATHLEN.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020424212147.Y16026-100000>