On Tue, Oct 25, 2022 at 02:24:31PM -0400, Taylor Blau wrote:
> To avoid potential overflow and truncation issues in `git apply`, apply
> similar treatment as in dcd1742e56 (xdiff: reject files larger than
> ~1GB, 2015-09-24), where the xdiff code was taught to reject large
> files for similar reasons.
I think this is a reasonable stopgap measure. In the case of xdiff, we
knew that we were working with text files. Is that always true here,
though? I.e., after this patch are we blocked from handling a 1GB binary
diff patch?
I'm mostly asking from a devil's advocate position. Even if the answer
is yes, I think this may still be the right thing to do, at least in the
short term.
> The maximum size was chosen somewhat arbitrarily, but picking a value
> just shy of a gigabyte allows us to double it without overflowing 2^31-1
> (after which point our value would wrap around to a negative number).
> To give ourselves a bit of extra margin, the maximum patch size is a MiB
> smaller than a full GiB, which gives us some slop in case we allocate
> "(records + 1) * sizeof(int)" or similar.
This was eerily familiar, and I wondered what "records" meant in
apply.c. But that is just the example from xdiff. :) I agree that the
same "extra margin" argument makes sense here, just out of caution.
> +/*
> + * apply.c isn't equipped to handle arbitrarily large patches, because
> + * it intermingles `unsigned long` with `int` for the type used to store
> + * buffer lengths.
> + *
> + * Only process patches that are just shy of 1 GiB large in order to
> + * avoid any truncation or overflow issues.
> + */
> +#define MAX_APPLY_SIZE (1024UL * 1024 * 1023)
> +
> static int read_patch_file(struct strbuf *sb, int fd)
> {
> - if (strbuf_read(sb, fd, 0) < 0)
> + if (strbuf_read(sb, fd, 0) < 0 || sb->len >= MAX_APPLY_SIZE)
> return error_errno("git apply: failed to read");
The patch itself looks reasonable. We'll potentially allocate an
unbounded amount of RAM before rejecting the patch, but there's not an
easy fix there without teaching strbuf_read() to accept a maximum.
It's probably not worth worrying about given the attack surface here
(it's not like anybody is reading patches from a socket that will cause
a DoS).
-Peff
