Re: [PATCH 2/4] write_sha1_file_prepare: fix buffer overrun with extra-long object type

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 5, 2015, 00:13 UTC
Message-ID: <CAPig+cS3f2XggxqbvX6Z2Da24QKLOg915Bf-bTVa+4oVzfhA1A@mail.gmail.com>
In-Reply-To: <1430775451-31130-3-git-send-email-gitster@pobox.com>

On Mon, May 4, 2015 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

> From: Eric Sunshine <sunshine@sunshineco.com>

Thanks for re-rerolling this series. Considering that the only bits left from me are the diagnosis and the (mostly intact) commit message, perhaps the authorship should be changed, or at the very least a big "Helped-by: Junio" added? Anyhow, a few minor comments below...

> write_sha1_file_prepare: fix buffer overrun with extra-long object type

Although the overrun happened in write_sha1_file_prepare(), that function is no longer the focus of the patch. Would make sense to rephrase the subject more generally as:

    sha1_file: fix buffer overrun with extra-long object type

or something.

More below.

Show 21 quoted lines

> git-hash-object learned --literally in 5ba9a93 (hash-object: add
> --literally option, 2014-09-11) which can be used to craft a
> corrupt/broken object of unknown type.
>
> When the user-provided type is particularly long, however, it can
> overflow the relatively small stack-based character array handed to
> write_sha1_file_prepare() by hash_sha1_file() and write_sha1_file(),
> leading to stack corruption (and crash).
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/cache.h b/cache.h
> index dfa1a56..2da7740 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -888,6 +888,7 @@ static inline const unsigned char *lookup_replace_object_extended(const unsigned
>  extern int sha1_object_info(const unsigned char *, unsigned long *);
>  extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
>  extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
> +extern int hash_sha1_file_literally(struct strbuf *buf, const char *type, unsigned char *return_sha1, unsigned flags);

A few questions:

What's the value of making the first argument of hash_sha1_file_literally() a strbuf rather than the two-argument buf & len accepted by hash_sha1_file() and write_sha1_file()? Is the inconsistency warranted?

Would it make sense to name the third argument "sha1" instead of "return_sha1" to match the argument name of hash_sha1_file()?

And, as an aside, should your new patch 4/4 rename "return_sha1" to "sha1" in the write_sha1_file() prototype also?

Show 19 quoted lines

>  extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
>  extern int force_object_loose(const unsigned char *sha1, time_t mtime);
>  extern int git_open_noatime(const char *name);
> diff --git a/sha1_file.c b/sha1_file.c
> index c08c0cb..0fe3f29 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2962,6 +2962,31 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
>         return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
>  }
>
> +int hash_sha1_file_literally(struct strbuf *buf, const char *type,
> +                            unsigned char *sha1, unsigned flags)
> +{
> +       struct strbuf header = STRBUF_INIT;
> +       int hdrlen, status = 0;
> +
> +       /* type string, SP, %lu of the length plus NUL must fit this */
> +       strbuf_grow(&header, strlen(type) + 20);

A couple comments:

First, given that the largest 64-bit unsigned long value (18,446,744,073,709,551,615) is 20 characters, do we want to be really pedantic and add 22 instead of 20?

Second, is strbuf overkill in this situation when a simple xmalloc()/free() would do?

Show 21 quoted lines

> +       write_sha1_file_prepare(buf->buf, buf->len, type, sha1,
> +                               header.buf, &hdrlen);
> +
> +       if (!(flags & HASH_WRITE_OBJECT))
> +               goto cleanup;
> +
> +       if (has_sha1_file(sha1))
> +               goto cleanup;
> +       status = write_loose_object(sha1, header.buf, hdrlen,
> +                                   buf->buf, buf->len, 0);
> +
> +cleanup:
> +       strbuf_release(&header);
> +       return status;
> +}
> +
>  int force_object_loose(const unsigned char *sha1, time_t mtime)
>  {
>         void *buf;
> --
> 2.4.0-302-g6743426