bug#26545: shred: buffer overlap, some data not wiped

[Pádraig Brady]

On 17/04/17 11:53, Bogdan wrote:
>  Hi.
> 
>  I believe I've found a bug in shred.c, function fillpattern(): there
> is code that says:
> 
>   for (i = 3; i < size / 2; i *= 2)
>     memcpy (r + i, r, i);
>   if (i < size)
>     memcpy (r + i, r, size - i);
> 
> The problem occurs for specific values of the "size" variable.
> Example: size = 7:
> 1) size / 2 = 3,
> 2) the "for" loop sets "i" to 3 and never runs (the condition is "3 <
> 3"), even though it could (there are 4 bytes left to be wiped now)
> 3) the "if" condition is true (3 < 7)
> 4) the memcpy call evaluates to
>       memcpy (r + 3, r, 7 - 3)
>    in other words:
>       memcpy (r + 3, r, 4)
> 
> Now, this poses a few problems:
> 
> 1) copying 4 bytes between areas separated by 3 bytes means that the
> areas overlap, which is forbidden for memcpy (results are not guaranteed),
> 
> 2) copying 4 bytes with only 3 of them wiped means potentially copying
> 1 byte of the original data (from byte 4 to byte 7) and leaving it there.
> 
>  I don't know if it's possible to get "size = 7" with the current code
> shape, but there may be other "problematic" values. May look like a
> small bug now, may become bigger later.

Very well spotted!
It's easy enough to trigger:

  touch blah
  shred -n4 -s7 blah

valgrind or ASAN will trigger failures due to this issue.

>  Anyway, I'm attaching a simple patch to fix this. The key change is
> to write "i * 2 < size" instead of "i < size / 2". Although
> mathematically equivalent, with C's integer arithmetic the latter one
> will truncate the results.
>  You may change left shifts to multiplications, if you wish, but if
> overflow happens, it will happen in both versions.
> 
>  Things to keep in mind for later:
> - size_t may be 32 bits wide, so watch out for buffers of 4GB or more
> (may happen one day? :) ),
> - if one day "size" could be any value (including 1 or 2), buffer
> overflow will happen.

These overflow issues can be avoided by just changing the < to <=.
I've not noted the issue in NEWS since it really is edge case stuff
not practically relevant to users, especially considering there
is always a random pass written after the patterns.

I've updated your patch in the attached and will push later.

thanks,
Pádraig

shred-pattern-umr.patch
Description: Text Data