From unknown Fri Aug 15 16:19:52 2025 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-Mailer: MIME-tools 5.509 (Entity 5.509) Content-Type: text/plain; charset=utf-8 From: bug#26545 <26545@debbugs.gnu.org> To: bug#26545 <26545@debbugs.gnu.org> Subject: Status: shred: buffer overlap, some data not wiped Reply-To: bug#26545 <26545@debbugs.gnu.org> Date: Fri, 15 Aug 2025 23:19:52 +0000 retitle 26545 shred: buffer overlap, some data not wiped reassign 26545 coreutils submitter 26545 Bogdan severity 26545 normal thanks From debbugs-submit-bounces@debbugs.gnu.org Mon Apr 17 15:10:23 2017 Received: (at submit) by debbugs.gnu.org; 17 Apr 2017 19:10:24 +0000 Received: from localhost ([127.0.0.1]:52907 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d0C2W-0006eq-Gg for submit@debbugs.gnu.org; Mon, 17 Apr 2017 15:10:23 -0400 Received: from eggs.gnu.org ([208.118.235.92]:43914) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d0BnF-0006H0-LC for submit@debbugs.gnu.org; Mon, 17 Apr 2017 14:54:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d0Bn9-0001dj-11 for submit@debbugs.gnu.org; Mon, 17 Apr 2017 14:54:28 -0400 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on eggs.gnu.org X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=BAYES_50,T_DKIM_INVALID autolearn=disabled version=3.3.2 Received: from lists.gnu.org ([2001:4830:134:3::11]:54081) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1d0Bn8-0001dY-UD for submit@debbugs.gnu.org; Mon, 17 Apr 2017 14:54:26 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34864) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d0Bn5-0000b3-3R for bug-coreutils@gnu.org; Mon, 17 Apr 2017 14:54:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d0Bn1-0001bC-Dv for bug-coreutils@gnu.org; Mon, 17 Apr 2017 14:54:23 -0400 Received: from smtpo53.poczta.onet.pl ([213.180.142.184]:34673) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1d0Bn1-0001Zm-2O for bug-coreutils@gnu.org; Mon, 17 Apr 2017 14:54:19 -0400 Received: from orion.wszechswiat.org (unknown [185.165.168.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: bogdandr@op.pl) by smtp.poczta.onet.pl (Onet) with ESMTPSA id 3w6HVQ6Xddz18VTtg for ; Mon, 17 Apr 2017 20:54:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=op.pl; s=2011; t=1492455250; bh=Fj3pyLnSg8nPFefA2mNFRvZkoglTRi2h/NfWZQU9B3E=; h=To:From:Subject:Date:From; b=I5vA6EJzDFwmosUd3CEroOLuC/WGMkVrzn9cZ58D8ZNTZdRmPpQ0U6ffzZl9ip5Pn d3NWz6REhGF0mlSZO2Ehp+Xw76geL3i9AHA47cCRckzukJSYLWnYcYfwVNTMtdGlSJ TJKbN4GCZaW9mqMks49+G/jMrDjtRSshFfuzkyRI= To: bug-coreutils@gnu.org From: Bogdan Subject: shred: buffer overlap, some data not wiped Message-ID: Date: Mon, 17 Apr 2017 20:53:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------49A1CC4AD148B5139B520F20" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 2001:4830:134:3::11 X-Spam-Score: -4.1 (----) X-Debbugs-Envelope-To: submit X-Mailman-Approved-At: Mon, 17 Apr 2017 15:10:19 -0400 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -4.1 (----) This is a multi-part message in MIME format. --------------49A1CC4AD148B5139B520F20 Content-Type: text/plain; charset=iso-8859-2 Content-Transfer-Encoding: 7bit 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 --------------49A1CC4AD148B5139B520F20 Content-Type: text/x-patch; name="shred-buffer-fix.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="shred-buffer-fix.diff" --- 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); --------------49A1CC4AD148B5139B520F20-- From debbugs-submit-bounces@debbugs.gnu.org Mon Apr 17 22:50:22 2017 Received: (at 26545-done) by debbugs.gnu.org; 18 Apr 2017 02:50:22 +0000 Received: from localhost ([127.0.0.1]:53306 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d0JDi-0002g4-H5 for submit@debbugs.gnu.org; Mon, 17 Apr 2017 22:50:22 -0400 Received: from midir.magicbluesmoke.com ([82.195.144.46]:53564 helo=mail.magicbluesmoke.com) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d0JDg-0002fv-Ay for 26545-done@debbugs.gnu.org; Mon, 17 Apr 2017 22:50:21 -0400 Received: from localhost.localdomain (unknown [166.170.41.148]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.magicbluesmoke.com (Postfix) with ESMTPSA id 2B0739BF4; Tue, 18 Apr 2017 03:50:17 +0100 (IST) Subject: Re: bug#26545: shred: buffer overlap, some data not wiped To: Bogdan , 26545-done@debbugs.gnu.org References: From: =?UTF-8?Q?P=c3=a1draig_Brady?= Message-ID: <55f0917c-3966-410d-7135-e6598f6830f6@draigBrady.com> Date: Mon, 17 Apr 2017 19:50:14 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------089FF14DA27C74BAD5D1BBDE" X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 26545-done X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: 0.0 (/) This is a multi-part message in MIME format. --------------089FF14DA27C74BAD5D1BBDE Content-Type: text/plain; charset=iso-8859-2 Content-Transfer-Encoding: 8bit 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 --------------089FF14DA27C74BAD5D1BBDE Content-Type: text/x-patch; name="shred-pattern-umr.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="shred-pattern-umr.patch" =46rom f4570a9ed6a4a3d9698093d5c9388c59d454d1dd Mon Sep 17 00:00:00 2001 From: Bogdan Drozdowski Date: Mon, 17 Apr 2017 19:04:01 -0700 Subject: [PATCH] shred: fix invalid pattern generation for certain sizes * src/shred.c (fillpattern): Fix the "off by one" issue when testing whether we have enough space to copy the already written portion of the buffer to the remainder of the buffer. Specifically for buffer sizes that are (3*(2^x))+1, i.e. 7,13,... we both use an uninitialized byte and invoke undefined behavior in memcpy() operation on overlapping memory regions. * tests/misc/shred-passes.sh: Add an invocation that will trigger either valgrind UMR, or ASAN like: ERROR: AddressSanitizer: memcpy-param-overlap: memory ranges #1 0x403065 in fillpattern src/shred.c:293 A direct test is awkward due to the random writes surrounding the problematic pattern writes. Fixes http://bugs.gnu.org/26545 --- src/shred.c | 2 +- tests/misc/shred-passes.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/shred.c b/src/shred.c index a317c44..7926e7a 100644 --- a/src/shred.c +++ b/src/shred.c @@ -287,7 +287,7 @@ fillpattern (int type, unsigned char *r, size_t size)= r[0] =3D (bits >> 4) & 255; r[1] =3D (bits >> 8) & 255; r[2] =3D bits & 255; - for (i =3D 3; i < size / 2; i *=3D 2) + for (i =3D 3; i <=3D size / 2; i *=3D 2) memcpy (r + i, r, i); if (i < size) memcpy (r + i, r, size - i); diff --git a/tests/misc/shred-passes.sh b/tests/misc/shred-passes.sh index e8fcb4f..1990115 100755 --- a/tests/misc/shred-passes.sh +++ b/tests/misc/shred-passes.sh @@ -79,5 +79,13 @@ shred: f: removed" > exp || framework_failure_ shred -v -u -n20 -s4096 --random-source=3DUs f 2>out || fail=3D1 compare exp out || fail=3D1 =20 +# Trigger an issue in shred before v8.27 where single +# bytes in the pattern space were not initialized correctly +# for particular sizes, like 7,13,... +# This failed under both valgrind and ASAN. +for size in 1 2 6 7 8; do + touch shred.pattern.umr.size + shred -n4 -s$size shred.pattern.umr.size || fail=3D1 +done =20 Exit $fail --=20 2.9.3 --------------089FF14DA27C74BAD5D1BBDE-- From unknown Fri Aug 15 16:19:52 2025 Received: (at fakecontrol) by fakecontrolmessage; To: internal_control@debbugs.gnu.org From: Debbugs Internal Request Subject: Internal Control Message-Id: bug archived. Date: Tue, 16 May 2017 11:24:04 +0000 User-Agent: Fakemail v42.6.9 # This is a fake control message. # # The action: # bug archived. thanks # This fakemail brought to you by your local debbugs # administrator