Re: [PATCH 0/18] hardening allocations against integer overflow

From: Jeff King <peff@peff.net>
Date: Feb 15, 2016, 22:02 UTC
Message-ID: <20160215220212.GA10461@sigill.intra.peff.net>
In-Reply-To: <20160215214516.GA4015@sigill.intra.peff.net>

On Mon, Feb 15, 2016 at 04:45:16PM -0500, Jeff King wrote:

Show 10 quoted lines

> 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