mbox series

[v4,0/3] Add commit-graph fuzzer and fix buffer overflow

Message ID cover.1544729841.git.steadmon@google.com (mailing list archive)
Headers show
Series Add commit-graph fuzzer and fix buffer overflow | expand

Message

Josh Steadmon Dec. 13, 2018, 7:43 p.m. UTC
Add a new fuzz test for the commit graph and fix a buffer read-overflow
that it discovered. Additionally, fix the Makefile instructions for
building fuzzers.

Changes since V3:
  * Improve portability of the new test functionality.
  * Fix broken &&-chains in tests.

Changes since V2:
  * Avoid pointer arithmetic overflow when checking the graph's chunk
    count.
  * Merge the corrupt_graph_and_verify and
    corrupt_and_zero_graph_then_verify test functions.

Josh Steadmon (3):
  commit-graph, fuzz: Add fuzzer for commit-graph
  commit-graph: fix buffer read-overflow
  Makefile: correct example fuzz build

 .gitignore              |  1 +
 Makefile                |  3 +-
 commit-graph.c          | 67 +++++++++++++++++++++++++++++------------
 commit-graph.h          |  3 ++
 fuzz-commit-graph.c     | 16 ++++++++++
 t/t5318-commit-graph.sh | 16 ++++++++--
 6 files changed, 83 insertions(+), 23 deletions(-)
 create mode 100644 fuzz-commit-graph.c

Range-diff against v3:
1:  675d58ecea ! 1:  80b5662f30 commit-graph: fix buffer read-overflow
    @@ -55,29 +55,26 @@
      	pos=$1
      	data="${2:-\0}"
      	grepstr=$3
    -+	orig_size=$(stat --format=%s $objdir/info/commit-graph)
    -+	zero_pos=${4:-${orig_size}}
    ++	orig_size=$(wc -c < $objdir/info/commit-graph) &&
    ++	zero_pos=${4:-${orig_size}} &&
      	cd "$TRASH_DIRECTORY/full" &&
      	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
      	cp $objdir/info/commit-graph commit-graph-backup &&
      	printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
    -+	truncate --size=$zero_pos $objdir/info/commit-graph &&
    -+	truncate --size=$orig_size $objdir/info/commit-graph &&
    ++	dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 &&
    ++	dd if=/dev/zero of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=$(($orig_size - $zero_pos)) &&
      	test_must_fail git commit-graph verify 2>test_err &&
    - 	grep -v "^+" test_err >err
    +-	grep -v "^+" test_err >err
    ++	grep -v "^+" test_err >err &&
      	test_i18ngrep "$grepstr" err
      }
      
    -+
    - test_expect_success 'detect bad signature' '
    - 	corrupt_graph_and_verify 0 "\0" \
    - 		"graph signature"
     @@
      		"incorrect checksum"
      '
      
     +test_expect_success 'detect incorrect chunk count' '
    -+	corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
    ++	corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\377" \
     +		"chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET
     +'
     +
2:  06a32bfe8b = 2:  21101b961a Makefile: correct example fuzz build

Comments

Jeff King Dec. 18, 2018, 5:35 p.m. UTC | #1
On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:

> Add a new fuzz test for the commit graph and fix a buffer read-overflow
> that it discovered. Additionally, fix the Makefile instructions for
> building fuzzers.
> 
> Changes since V3:
>   * Improve portability of the new test functionality.

I thought there was some question about /dev/zero, which I think is
in this version (I don't actually know whether there are portability
issues or not, but somebody did mention it).

-Peff
Josh Steadmon Dec. 18, 2018, 9:05 p.m. UTC | #2
On 2018.12.18 12:35, Jeff King wrote:
> On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:
> 
> > Add a new fuzz test for the commit graph and fix a buffer read-overflow
> > that it discovered. Additionally, fix the Makefile instructions for
> > building fuzzers.
> > 
> > Changes since V3:
> >   * Improve portability of the new test functionality.
> 
> I thought there was some question about /dev/zero, which I think is
> in this version (I don't actually know whether there are portability
> issues or not, but somebody did mention it).
> 
> -Peff

I've only found one reference [1] (from 1999) of OS X Server not having
a /dev/zero. It appears to be present as of 2010 though [2].

[1]: https://macosx-admin.omnigroup.narkive.com/sAxj0XeP/dev-zero-equivalent-in-mac-os-x
[2]: https://serverfault.com/questions/143248/zeroing-a-disk-with-dd-vs-disk-utility
Jeff King Dec. 19, 2018, 3:51 p.m. UTC | #3
On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote:

> On 2018.12.18 12:35, Jeff King wrote:
> > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:
> > 
> > > Add a new fuzz test for the commit graph and fix a buffer read-overflow
> > > that it discovered. Additionally, fix the Makefile instructions for
> > > building fuzzers.
> > > 
> > > Changes since V3:
> > >   * Improve portability of the new test functionality.
> > 
> > I thought there was some question about /dev/zero, which I think is
> > in this version (I don't actually know whether there are portability
> > issues or not, but somebody did mention it).
> > 
> > -Peff
> 
> I've only found one reference [1] (from 1999) of OS X Server not having
> a /dev/zero. It appears to be present as of 2010 though [2].

Thanks for digging. That seems like enough to assume we should try it
and see if any macOS people complain.

I do wonder if we'll run into problems on Windows, though.

-Peff
Johannes Schindelin Dec. 20, 2018, 7:35 p.m. UTC | #4
Hi Peff,


On Wed, 19 Dec 2018, Jeff King wrote:

> On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote:
> 
> > On 2018.12.18 12:35, Jeff King wrote:
> > > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:
> > > 
> > > > Add a new fuzz test for the commit graph and fix a buffer read-overflow
> > > > that it discovered. Additionally, fix the Makefile instructions for
> > > > building fuzzers.
> > > > 
> > > > Changes since V3:
> > > >   * Improve portability of the new test functionality.
> > > 
> > > I thought there was some question about /dev/zero, which I think is
> > > in this version (I don't actually know whether there are portability
> > > issues or not, but somebody did mention it).
> > > 
> > > -Peff
> > 
> > I've only found one reference [1] (from 1999) of OS X Server not having
> > a /dev/zero. It appears to be present as of 2010 though [2].
> 
> Thanks for digging. That seems like enough to assume we should try it
> and see if any macOS people complain.
> 
> I do wonder if we'll run into problems on Windows, though.

As long as we're talking about Unix shell scripts, /dev/zero should be
fine, as we are essentially running in a variant of Cygwin.

If you try to pass /dev/zero as an argument to a Git command, that's an
entirely different thing: this most likely won't work.

Ciao,
Dscho
Jeff King Dec. 20, 2018, 8:11 p.m. UTC | #5
On Thu, Dec 20, 2018 at 08:35:57PM +0100, Johannes Schindelin wrote:

> > I do wonder if we'll run into problems on Windows, though.
> 
> As long as we're talking about Unix shell scripts, /dev/zero should be
> fine, as we are essentially running in a variant of Cygwin.
> 
> If you try to pass /dev/zero as an argument to a Git command, that's an
> entirely different thing: this most likely won't work.

Thanks for confirming. We're talking about passing it to dd here, so I
think it should be OK.

-Peff
Junio C Hamano Dec. 26, 2018, 10:29 p.m. UTC | #6
Jeff King <peff@peff.net> writes:

> On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote:
>
>> On 2018.12.18 12:35, Jeff King wrote:
>> > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:
>> > 
>> > > Add a new fuzz test for the commit graph and fix a buffer read-overflow
>> > > that it discovered. Additionally, fix the Makefile instructions for
>> > > building fuzzers.
>> > > 
>> > > Changes since V3:
>> > >   * Improve portability of the new test functionality.
>> > 
>> > I thought there was some question about /dev/zero, which I think is
>> > in this version (I don't actually know whether there are portability
>> > issues or not, but somebody did mention it).
>> > 
>> > -Peff
>> 
>> I've only found one reference [1] (from 1999) of OS X Server not having
>> a /dev/zero. It appears to be present as of 2010 though [2].
>
> Thanks for digging. That seems like enough to assume we should try it
> and see if any macOS people complain.

Our tests have been relying on /dev/zero since 852a1710 ("am: let
command-line options override saved options", 2015-08-04) that
appeared in v2.6.0.  Anybody who has trouble with /dev/zero now has
kept silent for about a dozen major releases, I think, and will be
silent with this one, too ;-)

>
> I do wonder if we'll run into problems on Windows, though.
>
> -Peff