Skip to content

Commit

Permalink
Add missing sanity-check on mdat->argc.
Browse files Browse the repository at this point in the history
This is a scary bug, since it allows an attacker who can store validly
HMACed tarsnap blocks to make tarsnap -t, -x, or --list-archives have
an integer overflow in a memory allocation, resulting in a buffer
overflow when the allocated buffer is used.  However, it turns out to
be mostly harmless.

If the attacker provides a value of argc which is negative, the
allocated buffer will never be used: Every instance where argc is
referenced is a "for (i = 0; i < argc; i++)" loop.  Consequentially,
while a negative value of argc can result in a bogus malloc length,
there will be no buffer overflow.  (Note that this case cannot occur
if sizeof(int) > 4; but if sizeof(int) == 4 and sizeof(size_t) > 4,
we will call malloc on a size_t which has its MSB set, due to the way
that the conversion from signed to unsigned is performed.)

Assuming that sizeof(size_t) >= sizeof(int), the computation of
argc * sizeof(char *) will be done by first converting argc to a
size_t, due to the C language's "usual arithmetic conversion" rules.
I'm not aware of any systems, even the most bizarre embedded ones,
where sizeof(size_t) < sizeof(int), so this should be a safe thing
to assume.

If the attacker provides a value of argc between 0 and SIZE_MAX / X,
where X = sizeof(char *), no integer overflow occurs, and thus no buffer
overflow occurs.  The memory allocation requested might however be as
large as the minimum of SIZE_MAX & -X and (2^32 - 1) * X; but any such
large allocation would be freed when tarsnap runs out of metadata buffer
while copying the individual strings which make up the command line.

If the attacker provides a value of argc between SIZE_MAX / X and 2^32
(which is only possible on 32-bit or smaller machines), the calculation
of the malloc size will overflow and a short memory allocation may
occur.  However, the immediately following code proceeds to fill the
buffer -- which wraps all the way around address space -- with NULLs,
which is guaranteed to cause a crash.

To summarize: On 32-bit and smaller machines, an attacker who can
store a validly HMACed tarsnap block can make tarsnap crash upon
listing archives or reading an archive the attacker created; on larger
machines, that attacker can make tarsnap waste a lot of memory.

Reported by:	Ryan Govostes
Bur bounty:	$100
  • Loading branch information
cperciva committed Jun 29, 2015
1 parent 9fa670c commit 77751c1
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions tar/multitape/multitape_metadata.c
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,10 @@ multitape_metadata_dec(struct tapemetadata * mdat, uint8_t * buf,
buflen -= 4;
p += 4;

/* Sanity-check argc. */
if ((mdat->argc < 0) || ((size_t)(mdat->argc) > buflen))
goto bad1;

/* Allocate space for argv. */
if ((mdat->argv = malloc(mdat->argc * sizeof(char *))) == NULL)
goto err1;
Expand Down

0 comments on commit 77751c1

Please sign in to comment.