Re: [hackers] [sbase] [PATCH 02/10] od: Fix buffer overflow if -N flag is larger than BUFSIZ

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Mon, 5 Dec 2016 13:47:02 +0100

Hi

On Mon, Dec 5, 2016 at 6:55 AM, Michael Forney <mforney_AT_mforney.org> wrote:
> Previously, if max was specified, od will call read with that size,
> potentially overflowing buf with data read from the file.
> ---
> od.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/od.c b/od.c
> index 9b83501..b5884e7 100644
> --- a/od.c
> +++ b/od.c
> _AT_@ -132,20 +132,19 @@ od(FILE *fp, char *fname, int last)
> size_t i;
> unsigned char buf[BUFSIZ];
> static off_t addr;
> - size_t buflen;
> + size_t n;
>
> while (skip - addr > 0) {
> - buflen = fread(buf, 1, MIN(skip - addr, BUFSIZ), fp);
> - addr += buflen;
> + n = fread(buf, 1, MIN(skip - addr, sizeof(buf)), fp);
> + addr += n;
> if (feof(fp) || ferror(fp))
> return;
> }
> if (!line)
> line = emalloc(linelen);
>
> - while ((buflen = fread(buf, 1, max >= 0 ?
> - max - (addr - skip) : BUFSIZ, fp))) {
> - for (i = 0; i < buflen; i++, addr++) {
> + while ((n = fread(buf, 1, MIN((size_t)max - (addr - skip), sizeof(buf)), fp))) {

From what I understand, max is an off_t which is signed and set to -1
(if not changed by a command line flag). If we cast this to the
unsigned size_t we get a very big number in the case where 'max' is
not set by a flag and the buffer size is used instead. Looks correct
to me.

The brackets around 'addr - skip' are not needed but I assume they are
there for documentation purposes (and they were present before the
patch too) so

LGTM


Cheers,

Silvan


> + for (i = 0; i < n; i++, addr++) {
> line[lineoff++] = buf[i];
> if (lineoff == linelen) {
> printline(line, lineoff, addr - lineoff + 1);
> --
> 2.11.0
>
>
Received on Mon Dec 05 2016 - 13:47:02 CET