From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Karthik Nayak <karthik.188@gmail.com>,
Gustavo Grieco <gustavo.grieco@imag.fr>
Subject: Re: [PATCH] unpack_sha1_header(): detect malformed object header
Date: Mon, 26 Sep 2016 10:34:32 -0700 [thread overview]
Message-ID: <xmqqoa3atviv.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <xmqqfuomvdqe.fsf@gitster.mtv.corp.google.com> (Junio C. Hamano's message of "Mon, 26 Sep 2016 09:15:53 -0700")
And the second one, that no longer checks SP in unpacker, looks like
this.
-- >8 --
Subject: [PATCH] unpack_sha1_header(): detect malformed object header
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 <typename> SP <length> NUL
Because the parsing function parse_sha1_header_extended() is not
given the number of bytes inflated into the header buffer, it you
craft a file whose early part inflates a garbage sequence without SP
or NUL, and replace a loose object with it, it will end up reading
past the end of the inflated data.
To correct this, do the following four 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 inflated data has NUL. 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 NUL for the same effect.
- update parse_sha1_header_extended() to make sure that its loop to
find the SP that terminates the <typename> stops at NUL.
Essentially, this makes unpack_*() functions that are asked to
unpack a loose object header to be a bit more strict and detect an
input that cannot possibly be a valid object header, even before the
parsing function kicks in.
Reported-by: Gustavo Grieco <gustavo.grieco@imag.fr>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
sha1_file.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 17262e1..f7054d3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1566,7 +1566,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));
@@ -1579,13 +1581,31 @@ 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)
+{
+ int status = unpack_sha1_short_header(stream, map, mapsize,
+ buffer, bufsiz);
+
+ if (status < Z_OK)
+ return status;
+
+ /* Make sure we have the terminating NUL */
+ if (!memchr(buffer, '\0', stream->next_out - (unsigned 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)
{
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.
@@ -1676,6 +1696,8 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
*/
for (;;) {
char c = *hdr++;
+ if (!c)
+ return -1;
if (c == ' ')
break;
type_len++;
--
2.10.0-533-ga18d90d
next prev parent reply other threads:[~2016-09-26 17:34 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 ` [PATCH] unpack_sha1_header(): detect malformed object header Junio C Hamano
2016-09-26 14:03 ` 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 [this message]
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=xmqqoa3atviv.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).