Re: [PATCH 0/18] hardening allocations against integer overflow
On Mon, Feb 15, 2016 at 04:45:16PM -0500, Jeff King wrote: > The only bug I have actually confirmed in practice here is fixed by > patch 2 (which is why it's at the front). There's another one in > path_name(), but that function is already dropped by the nearby > jk/lose-name-path topic. > > The rest are cleanups of spots which _might_ be buggy, but I didn't dig > too far to find out. As with the earlier audit, I tried to refactor > using helpers that make the code clearer and less error-prone. So maybe > they're fixing bugs or not, but they certainly make it easier to audit > the result for problems. After this, looking for /[cm]alloc.*\+/ is pretty clean. I _didn't_ fix any sites in xdiff, but those are already protected by dcd1742 (xdiff: reject files larger than ~1GB, 2015-09-24). That's not necessarily complete coverage, though, as you can always screw up the computation outside of the xmalloc call, and pass in the truncated version. E.g.: int alloc = a + b; char *foo = xmalloc(alloc); However, this is only a big problem if you then copy "a" and "b" separately. If you use "alloc" later as the limit, like: xsnprintf(foo, alloc, "%s%s", a, b); that's only a minor bug (we notice the overflow and complain, rather than smashing the heap). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/18] hardening allocations against integer overflow
About 6 months or so ago, I did an audit of git's code base for uses of
strcpy and sprintf that could overflow, fixing any bugs and cleaning up
any suspect spots to make further audits simpler. This is a
continuation of that work, for size computations which can overflow and
cause us to allocate a too-small buffer. E.g., something like:
char *concat(const char *a, const char *b)
{
unsigned len_a = strlen(a);
unsigned len_b = strlen(b);
char *ret = xmalloc(len_a + len_b);
memcpy(ret, a, len_a);
memcpy(ret, b, len_b);
}
will behave badly if the sum of "a" and "b" overflows "unsigned". There
are other variants, too (we are also truncating the return value from
strlen, and we'd frequently use "int" here, so the lengths can actually
be negative!). It also varies based on platform. If the sites use size_t
instead of int, then 64-bit systems are typically hard to trigger in
practice (just because you'd need petabytes to store "a" and "b" in the
first place).
The only bug I have actually confirmed in practice here is fixed by
patch 2 (which is why it's at the front). There's another one in
path_name(), but that function is already dropped by the nearby
jk/lose-name-path topic.
The rest are cleanups of spots which _might_ be buggy, but I didn't dig
too far to find out. As with the earlier audit, I tried to refactor
using helpers that make the code clearer and less error-prone. So maybe
they're fixing bugs or not, but they certainly make it easier to audit
the result for problems.
[01/18]: add helpers for detecting size_t overflow
[02/18]: tree-diff: catch integer overflow in combine_diff_path allocation
[03/18]: harden REALLOC_ARRAY and xcalloc against size_t overflow
[04/18]: add helpers for allocating flex-array structs
[05/18]: convert trivial cases to ALLOC_ARRAY
[06/18]: use xmallocz to avoid size arithmetic
[07/18]: convert trivial cases to FLEX_ARRAY macros
[08/18]: use st_add and st_mult for allocation size computation
[09/18]: write_untracked_extension: use FLEX_ALLOC helper
[10/18]: fast-import: simplify allocation in start_packfile
[11/18]: fetch-pack: simplify add_sought_entry
[12/18]: test-path-utils: fix normalize_path_copy output buffer size
[13/18]: sequencer: simplify memory allocation of get_message
[14/18]: git-compat-util: drop mempcpy compat code
[15/18]: transport_anonymize_url: use xstrfmt
[16/18]: diff_populate_gitlink: use a strbuf
[17/18]: convert ewah/bitmap code to use xmalloc
[18/18]: ewah: convert to REALLOC_ARRAY, etc
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
