Date: Wed, 24 Apr 2002 22:41:22 -0600 From: "Kenneth D. Merry" <ken@kdm.org> To: Bruce Evans <bde@zeta.org.au> Cc: "M. Warner Losh" <imp@village.org>, obrien@FreeBSD.org, cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/lib/libcam camlib.h Message-ID: <20020424224122.A48194@panzer.kdm.org> In-Reply-To: <20020424212147.Y16026-100000@gamplex.bde.org>; from bde@zeta.org.au on Wed, Apr 24, 2002 at 09:46:51PM %2B1000 References: <20020423.235310.117915219.imp@village.org> <20020424212147.Y16026-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--k+w/mQv8wyuph6w0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Wed, Apr 24, 2002 at 21:46:51 +1000, Bruce Evans wrote:
> 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.
Thanks for pointing this out!
The only problem is, the NUL terminator isn't put in either string, and the
structure isn't bzeroed, I think.
So what needs to be done is make sure the strncpy() instance above doesn't
overflow, and make sure the strings are NUL terminated.
I think the attached patch will fix the problem, let me know what you
think. strncpy() could be used instead of strlcpy(), with the addition of
an extra line to NUL terminate the string in case the string copied into
the buffer is as long as the buffer.
Thanks,
Ken
--
Kenneth Merry
ken@kdm.org
--k+w/mQv8wyuph6w0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="camlib.c.20020424"
==== //depot/FreeBSD-ken/src/lib/libcam/camlib.c#4 - /usr/home/ken/perforce/FreeBSD-ken/src/lib/libcam/camlib.c ====
*** /tmp/tmp.1173.0 Wed Apr 24 22:28:56 2002
--- /usr/home/ken/perforce/FreeBSD-ken/src/lib/libcam/camlib.c Wed Apr 24 22:27:25 2002
***************
*** 576,582 ****
* 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';
--- 576,583 ----
* If the user passed in a path, save it for him.
*/
if (given_path != NULL)
! strlcpy(device->device_path, given_path,
! sizeof(device->device_path));
else
device->device_path[0] = '\0';
***************
*** 585,591 ****
* 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';
device->given_unit_number = given_unit_number;
--- 586,593 ----
* those as well.
*/
if (given_dev_name != NULL)
! strlcpy(device->given_dev_name, given_dev_name,
! sizeof(device->given_dev_name));
else
device->given_dev_name[0] = '\0';
device->given_unit_number = given_unit_number;
***************
*** 648,654 ****
"%s: %s", func_name, func_name, strerror(errno));
goto crod_bailout;
}
! strncpy(device->sim_name, ccb.cpi.dev_name, SIM_IDLEN);
device->sim_unit_number = ccb.cpi.unit_number;
device->bus_id = ccb.cpi.bus_id;
--- 650,656 ----
"%s: %s", func_name, func_name, strerror(errno));
goto crod_bailout;
}
! strlcpy(device->sim_name, ccb.cpi.dev_name, sizeof(device->sim_name));
device->sim_unit_number = ccb.cpi.unit_number;
device->bus_id = ccb.cpi.bus_id;
--k+w/mQv8wyuph6w0--
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?20020424224122.A48194>