From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org, Karthik Nayak <karthik.188@gmail.com>,
Jeff King <peff@peff.net>
Cc: Gustavo Grieco <gustavo.grieco@imag.fr>
Subject: [PATCH] unpack_sha1_header(): detect malformed object header
Date: Sun, 25 Sep 2016 21:29:04 -0700 [thread overview]
Message-ID: <xmqqshsnuvvz.fsf_-_@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com> (Junio C. Hamano's message of "Sun, 25 Sep 2016 17:10:31 -0700")
When opening a loose object file, we often do this sequence:
- prepare a short buffer for the object header (on stack)
- call unpack_sha1_header() and have early part of the object data
inflated, enough to fill the buffer
- parse that data in the short buffer, assuming that the first part
of the object is <type> SP <length> NUL
Nobody in this sequence however actually verifies that the loop that
tries to find SP that must come after the typename or NUL that must
come after the length exist in the inflated data. Because the
parsing function parse_sha1_header_extended() is not even given the
number of bytes inflated into the header buffer, it can easily read
past it, looking for the SP byte that may not even exist.
A variant recently introduced to support "--allow-unknown-type"
option of "git cat-file -t" changes the second step to use
unpack_sha1_header_to_strbuf(), but the story is essentially the
same. It did check to see if it saw enough to include NUL, but
nobody checked for SP before calling the parsing function.
To correct this, do these three things:
- rename unpack_sha1_header() to unpack_sha1_short_header() and
have unpack_sha1_header_to_strbuf() keep calling that as its
helper function. This will detect and report zlib errors, but is
not aware of the format of a loose object (as before).
- introduce unpack_sha1_header() that calls the same helper
function, and when zlib reports it inflated OK into the buffer,
check if the buffer has both SP and NUL in this order. This
would ensure that parsing function will terminate within the
buffer that holds the inflated header.
- update unpack_sha1_header_to_strbuf() to check if the resulting
buffer has both SP and NUL in this order for the same effect.
Reported-by: Gustavo Grieco <gustavo.grieco@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Unlike the "something like this" version, this does the "we got
some data, does it look like an object header, safely parseable
by our parser?" check in the unpack code, without touching the
parser, as I think that division of labor between the unpacker
and the parser makes more sense.
The strbuf codepath came in 46f03448 ("sha1_file: support reading
from a loose object of unknown type", 2015-05-03) by Karthik,
whose log says it was written by me, and helped by Peff, so I'm
asking these two to lend their eyes.
sha1_file.c | 40 +++++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index b9c1fa3..445e763 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1646,7 +1646,9 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
return used;
}
-int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz)
+static int unpack_sha1_short_header(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, unsigned long bufsiz)
{
/* Get the data stream */
memset(stream, 0, sizeof(*stream));
@@ -1659,13 +1661,37 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma
return git_inflate(stream, 0);
}
+int unpack_sha1_header(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, unsigned long bufsiz)
+{
+ const char *eoh;
+ int status = unpack_sha1_short_header(stream, map, mapsize,
+ buffer, bufsiz);
+
+ if (status < Z_OK)
+ return status;
+
+ /* Make sure we have the terminating NUL */
+ eoh = memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer);
+ if (!eoh)
+ return -1;
+ /* Make sure we have ' ' at the end of type */
+ if (!memchr(buffer, ' ', eoh - (const char *)buffer))
+ return -1;
+ return 0;
+}
+
static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
unsigned long mapsize, void *buffer,
unsigned long bufsiz, struct strbuf *header)
{
+ const char *eoh;
int status;
- status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
+ status = unpack_sha1_short_header(stream, map, mapsize, buffer, bufsiz);
+ if (status < Z_OK)
+ return -1;
/*
* Check if entire header is unpacked in the first iteration.
@@ -1686,11 +1712,19 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
status = git_inflate(stream, 0);
strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);
if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
- return 0;
+ goto enough;
stream->next_out = buffer;
stream->avail_out = bufsiz;
} while (status != Z_STREAM_END);
return -1;
+
+enough:
+ eoh = memchr(header->buf, '\0', header->len);
+ if (!eoh)
+ die("BUG: the NUL we earlier saw is gone???");
+ if (!memchr(header->buf, ' ', eoh - header->buf))
+ return -1;
+ return 0;
}
static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
next prev parent reply other threads:[~2016-09-26 4:29 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1399913289.8224468.1474810664933.JavaMail.zimbra@imag.fr>
2016-09-25 14:12 ` Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0 Gustavo Grieco
2016-09-26 0:10 ` Junio C Hamano
2016-09-26 4:29 ` Junio C Hamano [this message]
2016-09-26 14:03 ` [PATCH] unpack_sha1_header(): detect malformed object header Jeff King
2016-09-26 16:15 ` Junio C Hamano
2016-09-26 17:33 ` Junio C Hamano
2016-09-26 17:35 ` Jeff King
2016-09-26 17:39 ` Junio C Hamano
2016-09-26 17:34 ` Junio C Hamano
2016-09-26 17:38 ` Jeff King
2016-09-26 13:50 ` Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0 Jeff King
2016-09-26 17:48 ` Gustavo Grieco
2016-09-26 17:55 ` Junio C Hamano
2016-09-26 18:01 ` Gustavo Grieco
2016-09-26 18:06 ` Junio C Hamano
2016-09-26 18:10 ` Junio C Hamano
2016-09-27 2:13 ` Gustavo Grieco
2016-09-27 7:19 ` Jeff King
2016-09-27 2:30 ` Possible integer overflow parsing malformed objects in " Gustavo Grieco
2016-09-27 8:07 ` Jeff King
2016-09-27 15:57 ` Junio C Hamano
2016-09-27 19:14 ` Gustavo Grieco
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqshsnuvvz.fsf_-_@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gustavo.grieco@imag.fr \
--cc=karthik.188@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).