Subject: shred: buffer overlap, some data not wiped
From: Bogdan <[email-masked]>
To: [email-masked]
Date: Mon, 17 Apr 2017 20:53:47 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

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.

 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.

-- 
Pozdrawiam/Regards - Bogdan                     (GNU/Linux & FreeDOS)
Kurs asemblera x86 (DOS, GNU/Linux):            http://bogdro.evai.pl
Grupy dyskusyjne o asm:  pl.comp.lang.asm alt.pl.asm alt.pl.asm.win32
www.Xiph.org www.TorProject.org  Soft(EN): http://bogdro.evai.pl/soft
--- src/shred-orig.c	2017-04-17 17:49:51.877401171 +0200
+++ src/shred.c	2017-04-17 17:50:39.875382422 +0200
@@ -287,7 +287,7 @@
   r[0] = (bits >> 4) & 255;
   r[1] = (bits >> 8) & 255;
   r[2] = bits & 255;
-  for (i = 3; i < size / 2; i *= 2)
+  for (i = 3; (i << 1) < size; i <<= 1)
     memcpy (r + i, r, i);
   if (i < size)
     memcpy (r + i, r, size - i);