Subject: Re: bug#20998: Out of bounds global read in shred / genpattern()
From: Pádraig Brady <[email-masked]>
To: Hanno Böck <[email-masked]>, [email-masked]
Date: Tue, 07 Jul 2015 03:28:32 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

On 07/07/15 01:45, Pádraig Brady wrote:
> On 07/07/15 00:29, Hanno Böck wrote:
>> Hi,
>>
>> There is an out of bounds read error in the function genpattern() in
>> shred (coreutils 8.23). This issue only appears randomly.
>>
>> To test:
>> a) recompile coreutils 8.23 with address sanitizer

> Nice one!
> 
> It looks like the restriction to the k patterns available
> was lost with v5.92-1462-g65533e1 and that this should
> fix it up.
> 
> diff --git a/src/shred.c b/src/shred.c
> index 63bcd6f..74f7ad9 100644
> --- a/src/shred.c
> +++ b/src/shred.c
> @@ -785,6 +785,7 @@ genpattern (int *dest, size_t num, struct randint_source *s)
>                    n--;
>                  }
>                p++;
> +              k--;
>              }
>            while (n);
>            break;

Attached is the full patch including a test.
Marking this as done.

thanks!
Pádraig.
From 5e5d454037df549cc914f45891957181aa3b0a45 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Tue, 7 Jul 2015 01:46:54 +0100
Subject: [PATCH] shred: fix pattern selection for certain iteration counts

This was detected in about 25% of runs with gcc -fsanitize=address

  ERROR: AddressSanitizer: global-buffer-overflow on address ...
  READ of size 4 at 0x000000416628 thread T0
    #0 0x40479f in genpattern src/shred.c:782
    #1 0x4050d9 in do_wipefd src/shred.c:921
    #2 0x406203 in wipefile src/shred.c:1175
    #3 0x406b84 in main src/shred.c:1316
    #4 0x7f3454a1ef9f in __libc_start_main (/lib64/libc.so.6+0x1ff9f)
    #5 0x4025d8 (/tmp/coreutils-8.23/src/shred+0x4025d8)
  0x000000416628 is located 56 bytes to the left of
  global variable '*.LC49' from 'src/shred.c' (0x416660) of size 17
  0x000000416628 is located 12 bytes to the right of
  global variable 'patterns' from 'src/shred.c' (0x416540) of size 220
  SUMMARY: AddressSanitizer: global-buffer-overflow src/shred.c:782

* src/shred.c (gen_patterns): Restrict pattern selection
to the K available, which regressed due to v5.92-1462-g65533e1.
* tests/misc/shred-passes.sh: Add a deterministic test case.
* NEWS: Mention the bug fix.
Fixes http://bugs.gnu.org/20998
---
 NEWS                       |  5 +++++
 src/shred.c                |  5 +++--
 tests/misc/shred-passes.sh | 34 +++++++++++++++++++++++++++++++++-
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index 7e213fd..54a0ab6 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,11 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 * Noteworthy changes in release ?.? (????-??-??) [?]
 
+** Bug fixes
+
+  shred again uses defined patterns for all iteration counts.
+  [bug introduced in coreutils-5.93]
+
 
 * Noteworthy changes in release 8.24 (2015-07-03) [stable]
 
diff --git a/src/shred.c b/src/shred.c
index 63bcd6f..52c93ef 100644
--- a/src/shred.c
+++ b/src/shred.c
@@ -712,7 +712,7 @@ static int const
   12, 0x111, 0x222, 0x333, 0x444, 0x666, 0x777,
   0x888, 0x999, 0xBBB, 0xCCC, 0xDDD, 0xEEE,	/* 4-bit */
   -1,				/* 1 random pass */
-        /* The following patterns have the frst bit per block flipped */
+        /* The following patterns have the first bit per block flipped */
   8, 0x1000, 0x1249, 0x1492, 0x16DB, 0x1924, 0x1B6D, 0x1DB6, 0x1FFF,
   14, 0x1111, 0x1222, 0x1333, 0x1444, 0x1555, 0x1666, 0x1777,
   0x1888, 0x1999, 0x1AAA, 0x1BBB, 0x1CCC, 0x1DDD, 0x1EEE,
@@ -776,7 +776,7 @@ genpattern (int *dest, size_t num, struct randint_source *s)
           break;
         }
       else
-        {			/* Pad out with k of the n available */
+        {			/* Pad out with n of the k available */
           do
             {
               if (n == (size_t) k || randint_choose (s, k) < n)
@@ -785,6 +785,7 @@ genpattern (int *dest, size_t num, struct randint_source *s)
                   n--;
                 }
               p++;
+              k--;
             }
           while (n);
           break;
diff --git a/tests/misc/shred-passes.sh b/tests/misc/shred-passes.sh
index 0fa63be..64216fd 100755
--- a/tests/misc/shred-passes.sh
+++ b/tests/misc/shred-passes.sh
@@ -32,9 +32,9 @@ shred: f: renamed to 0
 shred: f: removed" > exp || framework_failure_
 
 shred -v -u f 2>out || fail=1
-
 compare exp out || fail=1
 
+
 # Likewise but for a zero length file
 # to bypass the data passes
 touch f || framework_failure_
@@ -44,7 +44,39 @@ shred: f: renamed to 0
 shred: f: removed" > exp || framework_failure_
 
 shred -v -u f 2>out || fail=1
+compare exp out || fail=1
+
+
+# shred data 20 times and verify the passes used.
+# This would consume all random data between 5.93 and 8.24 inclusive.
+dd bs=100K count=1 if=/dev/zero | tr '\0' 'U' > Us || framework_failure_
+printf 1 > f || framework_failure_
+echo "\
+shred: f: pass 1/20 (random)...
+shred: f: pass 2/20 (ffffff)...
+shred: f: pass 3/20 (924924)...
+shred: f: pass 4/20 (888888)...
+shred: f: pass 5/20 (db6db6)...
+shred: f: pass 6/20 (777777)...
+shred: f: pass 7/20 (492492)...
+shred: f: pass 8/20 (bbbbbb)...
+shred: f: pass 9/20 (555555)...
+shred: f: pass 10/20 (aaaaaa)...
+shred: f: pass 11/20 (random)...
+shred: f: pass 12/20 (6db6db)...
+shred: f: pass 13/20 (249249)...
+shred: f: pass 14/20 (999999)...
+shred: f: pass 15/20 (111111)...
+shred: f: pass 16/20 (000000)...
+shred: f: pass 17/20 (b6db6d)...
+shred: f: pass 18/20 (eeeeee)...
+shred: f: pass 19/20 (333333)...
+shred: f: pass 20/20 (random)...
+shred: f: removing
+shred: f: renamed to 0
+shred: f: removed" > exp || framework_failure_
 
+shred -v -u -n20 --random-source=Us f 2>out || fail=1
 compare exp out || fail=1

-- 
2.4.1