GNU bug report logs -
#26545
shred: buffer overlap, some data not wiped
Previous Next
Reported by: Bogdan <bogdandr <at> op.pl>
Date: Mon, 17 Apr 2017 19:11:01 UTC
Severity: normal
Done: Pádraig Brady <P <at> draigBrady.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 26545 in the body.
You can then email your comments to 26545 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-coreutils <at> gnu.org
:
bug#26545
; Package
coreutils
.
(Mon, 17 Apr 2017 19:11:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Bogdan <bogdandr <at> op.pl>
:
New bug report received and forwarded. Copy sent to
bug-coreutils <at> gnu.org
.
(Mon, 17 Apr 2017 19:11:01 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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
[shred-buffer-fix.diff (text/x-patch, attachment)]
Reply sent
to
Pádraig Brady <P <at> draigBrady.com>
:
You have taken responsibility.
(Tue, 18 Apr 2017 02:51:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Bogdan <bogdandr <at> op.pl>
:
bug acknowledged by developer.
(Tue, 18 Apr 2017 02:51:02 GMT)
Full text and
rfc822 format available.
Message #10 received at 26545-done <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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 (text/x-patch, attachment)]
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Tue, 16 May 2017 11:24:04 GMT)
Full text and
rfc822 format available.
This bug report was last modified 6 years and 340 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.