# [PATCH 0/4] Fix various integer overflows 13 messages from 2010-01-26 to 2010-01-27. Participants: Ilari Liusvaara, Junio C Hamano, Bill Lear, Stephen R. van den Berg. Thread: https://gitlist.dev/t/22397 ## Ilari Liusvaara, 2010-01-26 18:24 Subject: [PATCH 0/4] Fix various integer overflows Message-ID: <1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi> URL: https://gitlist.dev/e/1264530255-4682-1-git-send-email-ilari.liusvaara%40elisanet.fi ``` Fix integer overflows in patch_delta(), unpack_sha1_rest() and unpack_compressed_entry(). These at least can cause git to segfault, possibly worse. Operations that cause integer overflow are not possible to do (even whole virtual memory space would not be sufficient), so die() instead. Ilari Liusvaara (4): Add xmallocz() Fix integer overflow in patch_delta() Fix integer overflow in unpack_sha1_rest() Fix integer overflow in unpack_compressed_entry() git-compat-util.h | 1 + patch-delta.c | 3 +-- sha1_file.c | 5 ++--- wrapper.c | 12 +++++++++++- 4 files changed, 15 insertions(+), 6 deletions(-) ``` ## Ilari Liusvaara, 2010-01-26 18:24 Subject: [PATCH 1/4] Add xmallocz() Message-ID: <1264530255-4682-2-git-send-email-ilari.liusvaara@elisanet.fi> URL: https://gitlist.dev/e/1264530255-4682-2-git-send-email-ilari.liusvaara%40elisanet.fi In-Reply-To: <1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi> ``` Add routine for allocating NUL-terminated memory block without risking integer overflow in addition of +1 for NUL byte. Signed-off-by: Ilari Liusvaara --- git-compat-util.h | 1 + wrapper.c | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index 620a7c6..a3c4537 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -348,6 +348,7 @@ extern void release_pack_memory(size_t, int); extern char *xstrdup(const char *str); extern void *xmalloc(size_t size); +extern void *xmallocz(size_t size); extern void *xmemdupz(const void *data, size_t len); extern char *xstrndup(const char *str, size_t len); extern void *xrealloc(void *ptr, size_t size); diff --git a/wrapper.c b/wrapper.c index c9be140..dd7b6ee 100644 --- a/wrapper.c +++ b/wrapper.c @@ -34,6 +34,16 @@ void *xmalloc(size_t size) return ret; } +void *xmallocz(size_t size) +{ + void *ret; + if (size + 1 < size) + die("Data too large to fit into virtual memory space."); + ret = xmalloc(size + 1); + ((char*)ret)[size] = 0; + return ret; +} + /* * xmemdupz() allocates (len + 1) bytes of memory, duplicates "len" bytes of * "data" to the allocated memory, zero terminates the allocated memory, @@ -42,7 +52,7 @@ void *xmalloc(size_t size) */ void *xmemdupz(const void *data, size_t len) { - char *p = xmalloc(len + 1); + char *p = xmallocz(len); memcpy(p, data, len); p[len] = '\0'; return p; -- 1.6.6.1.439.gf06b6 ``` ## Ilari Liusvaara, 2010-01-26 18:24 Subject: [PATCH 2/4] Fix integer overflow in patch_delta() Message-ID: <1264530255-4682-3-git-send-email-ilari.liusvaara@elisanet.fi> URL: https://gitlist.dev/e/1264530255-4682-3-git-send-email-ilari.liusvaara%40elisanet.fi In-Reply-To: <1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi> ``` Signed-off-by: Ilari Liusvaara --- patch-delta.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/patch-delta.c b/patch-delta.c index e02e13b..d218faa 100644 --- a/patch-delta.c +++ b/patch-delta.c @@ -33,8 +33,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size, /* now the result size */ size = get_delta_hdr_size(&data, top); - dst_buf = xmalloc(size + 1); - dst_buf[size] = 0; + dst_buf = xmallocz(size); out = dst_buf; while (data < top) { -- 1.6.6.1.439.gf06b6 ``` ## Ilari Liusvaara, 2010-01-26 18:24 Subject: [PATCH 3/4] Fix integer overflow in unpack_sha1_rest() Message-ID: <1264530255-4682-4-git-send-email-ilari.liusvaara@elisanet.fi> URL: https://gitlist.dev/e/1264530255-4682-4-git-send-email-ilari.liusvaara%40elisanet.fi In-Reply-To: <1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi> ``` Signed-off-by: Ilari Liusvaara --- sha1_file.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 12478a3..39f0844 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1166,7 +1166,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size, const unsigned char *sha1) { int bytes = strlen(buffer) + 1; - unsigned char *buf = xmalloc(1+size); + unsigned char *buf = xmallocz(size); unsigned long n; int status = Z_OK; -- 1.6.6.1.439.gf06b6 ``` ## Ilari Liusvaara, 2010-01-26 18:24 Subject: [PATCH 4/4] Fix integer overflow in unpack_compressed_entry() Message-ID: <1264530255-4682-5-git-send-email-ilari.liusvaara@elisanet.fi> URL: https://gitlist.dev/e/1264530255-4682-5-git-send-email-ilari.liusvaara%40elisanet.fi In-Reply-To: <1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi> ``` Signed-off-by: Ilari Liusvaara --- sha1_file.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 39f0844..ea2ea75 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1517,8 +1517,7 @@ static void *unpack_compressed_entry(struct packed_git *p, z_stream stream; unsigned char *buffer, *in; - buffer = xmalloc(size + 1); - buffer[size] = 0; + buffer = xmallocz(size); memset(&stream, 0, sizeof(stream)); stream.next_out = buffer; stream.avail_out = size + 1; -- 1.6.6.1.439.gf06b6 ``` ## Junio C Hamano, 2010-01-26 19:58 Subject: Re: [PATCH 0/4] Fix various integer overflows Message-ID: <7vk4v4zlhg.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk4v4zlhg.fsf%40alter.siamese.dyndns.org In-Reply-To: <1264530255-4682-1-git-send-email-ilari.liusvaara@elisanet.fi> ``` Looks trivially correct; thanks. ``` ## Bill Lear, 2010-01-26 20:37 Subject: Re: [PATCH 1/4] Add xmallocz() Message-ID: <19295.21148.182245.516321@blake.zopyra.com> URL: https://gitlist.dev/e/19295.21148.182245.516321%40blake.zopyra.com In-Reply-To: <1264530255-4682-2-git-send-email-ilari.liusvaara@elisanet.fi> ``` On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes: >Add routine for allocating NUL-terminated memory block without risking >integer overflow in addition of +1 for NUL byte. >... > void *xmemdupz(const void *data, size_t len) > { >- char *p = xmalloc(len + 1); >+ char *p = xmallocz(len); > memcpy(p, data, len); > p[len] = '\0'; > return p; Do you need the statement p[len] = '\0'; any longer in the above? If not, could you just do this: void *xmemdupz(const void *data, size_t len) { return memcpy(xmallocz(len), data, len); } ?? Bill ``` ## Junio C Hamano, 2010-01-26 20:47 Subject: Re: [PATCH 1/4] Add xmallocz() Message-ID: <7v1vhczj95.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1vhczj95.fsf%40alter.siamese.dyndns.org In-Reply-To: <19295.21148.182245.516321@blake.zopyra.com> ``` Bill Lear writes: > On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes: >>Add routine for allocating NUL-terminated memory block without risking >>integer overflow in addition of +1 for NUL byte. >>... >> void *xmemdupz(const void *data, size_t len) >> { >>- char *p = xmalloc(len + 1); >>+ char *p = xmallocz(len); >> memcpy(p, data, len); >> p[len] = '\0'; >> return p; > > Do you need the statement > > p[len] = '\0'; > > any longer in the above? If not, could you just do this: > > void *xmemdupz(const void *data, size_t len) > { > return memcpy(xmallocz(len), data, len); > } I think the intention to name it xmallocz() was "This is to allocate buffer to hold 'len' bytes worth of stuff, and between the caller and this function the buffer is arranged to be NUL terminated". Even though none of the existing callers of xmalloc() expected the function to do the NUL termination (hence they do NUL termination themselves), I _think_ Ilari made the function to do this because its name now ends with "z" that hints the callers such a NUL-termination might happen inside the function. I'd rather see the function lose the NUL termination; if that makes the behaviour inconsistent with its name, perhaps it is better to rename the function; perhaps xmalloc1() to denote that it overallocates by one? ``` ## Junio C Hamano, 2010-01-26 20:52 Subject: Re: [PATCH 1/4] Add xmallocz() Message-ID: <7vwrz4y4gf.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwrz4y4gf.fsf%40alter.siamese.dyndns.org In-Reply-To: <7v1vhczj95.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano writes: > I'd rather see the function lose the NUL termination; if that makes the > behaviour inconsistent with its name, perhaps it is better to rename the > function; perhaps xmalloc1() to denote that it overallocates by one? Actually I take that back---all the callers do benefit from the allocator giving a buffer that is pre-terminated with NUL. We can also lose "buf[size] = 0" from unpack_sha1_rest() patch. ``` ## Bill Lear, 2010-01-26 20:56 Subject: Re: [PATCH 1/4] Add xmallocz() Message-ID: <19295.22246.788034.159299@blake.zopyra.com> URL: https://gitlist.dev/e/19295.22246.788034.159299%40blake.zopyra.com In-Reply-To: <7v1vhczj95.fsf@alter.siamese.dyndns.org> ``` On Tuesday, January 26, 2010 at 12:47:02 (-0800) Junio C Hamano writes: >Bill Lear writes: > >> On Tuesday, January 26, 2010 at 20:24:12 (+0200) Ilari Liusvaara writes: >>>Add routine for allocating NUL-terminated memory block without risking >>>integer overflow in addition of +1 for NUL byte. >>>... >>> void *xmemdupz(const void *data, size_t len) >>> { >>>- char *p = xmalloc(len + 1); >>>+ char *p = xmallocz(len); >>> memcpy(p, data, len); >>> p[len] = '\0'; >>> return p; >> >> Do you need the statement >> >> p[len] = '\0'; >> >> any longer in the above? If not, could you just do this: >> >> void *xmemdupz(const void *data, size_t len) >> { >> return memcpy(xmallocz(len), data, len); >> } > >I think the intention to name it xmallocz() was "This is to allocate >buffer to hold 'len' bytes worth of stuff, and between the caller and this >function the buffer is arranged to be NUL terminated". Even though none >of the existing callers of xmalloc() expected the function to do the NUL >termination (hence they do NUL termination themselves), I _think_ Ilari >made the function to do this because its name now ends with "z" that hints >the callers such a NUL-termination might happen inside the function. > >I'd rather see the function lose the NUL termination; if that makes the >behaviour inconsistent with its name, perhaps it is better to rename the >function; perhaps xmalloc1() to denote that it overallocates by one? Why have xmallocz/xmalloc1 lose the NUL termination? Is it because some call sites don't need the NUL termination? [I spotted one, I think, in the patch series...]. Bill ``` ## Ilari Liusvaara, 2010-01-26 21:13 Subject: Re: [PATCH 1/4] Add xmallocz() Message-ID: <20100126211347.GA9194@Knoppix> URL: https://gitlist.dev/e/20100126211347.GA9194%40Knoppix In-Reply-To: <7vwrz4y4gf.fsf@alter.siamese.dyndns.org> ``` On Tue, Jan 26, 2010 at 12:52:00PM -0800, Junio C Hamano wrote: > Junio C Hamano writes: > > > I'd rather see the function lose the NUL termination; if that makes the > > behaviour inconsistent with its name, perhaps it is better to rename the > > function; perhaps xmalloc1() to denote that it overallocates by one? > > Actually I take that back---all the callers do benefit from the allocator > giving a buffer that is pre-terminated with NUL. > > We can also lose "buf[size] = 0" from unpack_sha1_rest() patch. If that line would be next to xmalloc() line, I would have removed it, but because it was beyound loop, I was worried that something might be done to it (and was not in right mood to analyze the logic properly). And yeah, that nul-termination in xmemdupz is not needed. Oops, missed that. -Ilari ``` ## Stephen R. van den Berg, 2010-01-27 08:59 Subject: Re: [PATCH 0/4] Fix various integer overflows Message-ID: <20100127085952.GA21535@cuci.nl> URL: https://gitlist.dev/e/20100127085952.GA21535%40cuci.nl In-Reply-To: <7vk4v4zlhg.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: >Looks trivially correct; thanks. I'm just curious, but is this based on an actual bug which someone experienced, or is this just based on mere theoretical code analysis? -- Sincerely, Stephen R. van den Berg. "Am I paying for this abuse or is it extra?" ``` ## Ilari Liusvaara, 2010-01-27 09:57 Subject: Re: [PATCH 0/4] Fix various integer overflows Message-ID: <20100127095748.GA9992@Knoppix> URL: https://gitlist.dev/e/20100127095748.GA9992%40Knoppix In-Reply-To: <20100127085952.GA21535@cuci.nl> ``` On Wed, Jan 27, 2010 at 09:59:52AM +0100, Stephen R. van den Berg wrote: > Junio C Hamano wrote: > >Looks trivially correct; thanks. > > I'm just curious, but is this based on an actual bug which someone > experienced, or is this just based on mere theoretical code analysis? Theoretical at first, but I did construct packfile that hits one of those overflows (the one in patch_delta(), 32 bits only). In real world, hitting this bug would require hitting exactly 2^32-1 byte file, and that is quite rare size for file. And what can happen with them in real world git usage is different than what can happen with them if packs are suitably manipulated ("transport streams" and bundles both contain packs in them). -Ilari ```