Re: [PATCH 1/5] avoid parse_sha1_header() accessing memory out of bound

From: Shawn O. Pearce <spearce@spearce.org>
Date: Dec 2, 2008, 15:42 UTC
Message-ID: <20081202154257.GK23984@spearce.org>
In-Reply-To: <4934949B.70307@gmail.com>

Liu Yubao <yubao.liu@gmail.com> wrote:

Show 20 quoted lines

> diff --git a/sha1_file.c b/sha1_file.c
> index 6c0e251..efe6967 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1254,10 +1255,10 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>  	/*
>  	 * The type can be at most ten bytes (including the
>  	 * terminating '\0' that we add), and is followed by
> -	 * a space.
> +	 * a space, at least one byte for size, and a '\0'.
>  	 */
>  	i = 0;
> -	for (;;) {
> +	while (hdr < hdr_end - 2) {
>  		char c = *hdr++;
>  		if (c == ' ')
>  			break;
> @@ -1265,6 +1266,8 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>  		if (i >= sizeof(type))
>  			return -1;

That first hunk I am citing is unnecessary, because of the lines right above. All of the callers of this function pass in a buffer that is at least 32 bytes in size; this loop aborts if it does not find a ' ' within the first 10 bytes of the buffer. We'll never access memory outside of the buffer during this loop.

So IMHO your first three hunks here aren't necessary.

Show 9 quoted lines

> @@ -1275,7 +1278,7 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>  	if (size > 9)
>  		return -1;
>  	if (size) {
> -		for (;;) {
> +		while (hdr < hdr_end - 1) {
>  			unsigned long c = *hdr - '0';
>  			if (c > 9)
>  				break;

OK, there's no promise here that we don't roll off the buffer.

This can be fixed in the caller, ensuring we always have the '\0' at some point in the initial header buffer we were asked to parse:

diff --git a/sha1_file.c b/sha1_file.c
index 6c0e251..26c6ffb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1162,9 +1162,10 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
 	stream->next_in = map;
 	stream->avail_in = mapsize;
 	stream->next_out = buffer;
-	stream->avail_out = bufsiz;
 
 	if (legacy_loose_object(map)) {
+		stream->avail_out = bufsiz - 1;
+		buffer[bufsiz - 1] = '\0';
 		inflateInit(stream);
 		return inflate(stream, 0);
 	}
@@ -1186,6 +1187,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
 	/* Set up the stream for the rest.. */
 	stream->next_in = map;
 	stream->avail_in = mapsize;
+	stream->avail_out = bufsiz;
 	inflateInit(stream);
 
 	/* And generate the fake traditional header */

-- 
Shawn.