{"thread":{"id":"58687","subject":"[PATCH] apply: reject patches larger than ~1 GiB","startedAt":"2022-10-25T18:24:37Z","lastAt":"2022-10-26T05:55:14Z","messageCount":2,"participants":["Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465723","messageId":"70f5763834dff373a5573a99ec4cdfa36cadf34c.1666722251.git.me@ttaylorr.com","threadId":"58687","inReplyTo":null,"subject":"[PATCH] apply: reject patches larger than ~1 GiB","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-25T18:24:31Z","receivedAt":"2022-10-25T18:24:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The apply code is not prepared to handle extremely large files. It uses\n\"int\" in some places, and \"unsigned long\" in others.\n\nThis combination leads to unfortunate problems when switching between\nthe two types. Using \"int\" prevents us from handling large files, since\nlarge offsets will wrap around and spill into small negative values,\nwhich can result in wrong behavior (like accessing the patch buffer with\na negative offset).\n\nConverting from \"unsigned long\" to \"int\" also has truncation problems\neven on LLP64 platforms where \"long\" is the same size as \"int\", since\nthe former is unsigned but the latter is not.\n\nTo avoid potential overflow and truncation issues in `git apply`, apply\nsimilar treatment as in dcd1742e56 (xdiff: reject files larger than\n~1GB, 2015-09-24), where the xdiff code was taught to reject large\nfiles for similar reasons.\n\nThe maximum size was chosen somewhat arbitrarily, but picking a value\njust shy of a gigabyte allows us to double it without overflowing 2^31-1\n(after which point our value would wrap around to a negative number).\nTo give ourselves a bit of extra margin, the maximum patch size is a MiB\nsmaller than a full GiB, which gives us some slop in case we allocate\n\"(records + 1) * sizeof(int)\" or similar.\n\nLuckily, the security implications of these conversion issues are\nrelatively uninteresting, because a victim needs to be convinced to\napply a malicious patch.\n\nReported-by: 정재우 <thebound7@gmail.com>\nSuggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n apply.c                    | 12 +++++++++++-\n t/t4141-apply-too-large.sh | 23 +++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 1 deletion(-)\n create mode 100755 t/t4141-apply-too-large.sh\n\ndiff --git a/apply.c b/apply.c\nindex 2b7cd930ef..850604c191 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -386,9 +386,19 @@ static void say_patch_name(FILE *output, const char *fmt, struct patch *patch)\n \n #define SLOP (16)\n \n+/*\n+ * apply.c isn't equipped to handle arbitrarily large patches, because\n+ * it intermingles `unsigned long` with `int` for the type used to store\n+ * buffer lengths.\n+ *\n+ * Only process patches that are just shy of 1 GiB large in order to\n+ * avoid any truncation or overflow issues.\n+ */\n+#define MAX_APPLY_SIZE (1024UL * 1024 * 1023)\n+\n static int read_patch_file(struct strbuf *sb, int fd)\n {\n-\tif (strbuf_read(sb, fd, 0) < 0)\n+\tif (strbuf_read(sb, fd, 0) < 0 || sb->len >= MAX_APPLY_SIZE)\n \t\treturn error_errno(\"git apply: failed to read\");\n \n \t/*\ndiff --git a/t/t4141-apply-too-large.sh b/t/t4141-apply-too-large.sh\nnew file mode 100755\nindex 0000000000..58742d4fc5\n--- /dev/null\n+++ b/t/t4141-apply-too-large.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='git apply with too-large patch'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success EXPENSIVE 'git apply rejects patches that are too large' '\n+\tsz=$((1024 * 1024 * 1023)) &&\n+\t{\n+\t\tcat <<-\\EOF &&\n+\t\tdiff --git a/file b/file\n+\t\tnew file mode 100644\n+\t\t--- /dev/null\n+\t\t+++ b/file\n+\t\t@@ -0,0 +1 @@\n+\t\tEOF\n+\t\ttest-tool genzeros\n+\t} | test_copy_bytes $sz | test_must_fail git apply 2>err &&\n+\tgrep \"git apply: failed to read\" err\n+'\n+\n+test_done\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"465755","messageId":"Y1jLuMN6biwR87ga@coredump.intra.peff.net","threadId":"58687","inReplyTo":"70f5763834dff373a5573a99ec4cdfa36cadf34c.1666722251.git.me@ttaylorr.com","subject":"Re: [PATCH] apply: reject patches larger than ~1 GiB","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-26T05:55:04Z","receivedAt":"2022-10-26T05:55:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 25, 2022 at 02:24:31PM -0400, Taylor Blau wrote:\n\n> To avoid potential overflow and truncation issues in `git apply`, apply\n> similar treatment as in dcd1742e56 (xdiff: reject files larger than\n> ~1GB, 2015-09-24), where the xdiff code was taught to reject large\n> files for similar reasons.\n\nI think this is a reasonable stopgap measure. In the case of xdiff, we\nknew that we were working with text files. Is that always true here,\nthough? I.e., after this patch are we blocked from handling a 1GB binary\ndiff patch?\n\nI'm mostly asking from a devil's advocate position. Even if the answer\nis yes, I think this may still be the right thing to do, at least in the\nshort term.\n\n> The maximum size was chosen somewhat arbitrarily, but picking a value\n> just shy of a gigabyte allows us to double it without overflowing 2^31-1\n> (after which point our value would wrap around to a negative number).\n> To give ourselves a bit of extra margin, the maximum patch size is a MiB\n> smaller than a full GiB, which gives us some slop in case we allocate\n> \"(records + 1) * sizeof(int)\" or similar.\n\nThis was eerily familiar, and I wondered what \"records\" meant in\napply.c. But that is just the example from xdiff. :) I agree that the\nsame \"extra margin\" argument makes sense here, just out of caution.\n\n> +/*\n> + * apply.c isn't equipped to handle arbitrarily large patches, because\n> + * it intermingles `unsigned long` with `int` for the type used to store\n> + * buffer lengths.\n> + *\n> + * Only process patches that are just shy of 1 GiB large in order to\n> + * avoid any truncation or overflow issues.\n> + */\n> +#define MAX_APPLY_SIZE (1024UL * 1024 * 1023)\n> +\n>  static int read_patch_file(struct strbuf *sb, int fd)\n>  {\n> -\tif (strbuf_read(sb, fd, 0) < 0)\n> +\tif (strbuf_read(sb, fd, 0) < 0 || sb->len >= MAX_APPLY_SIZE)\n>  \t\treturn error_errno(\"git apply: failed to read\");\n\nThe patch itself looks reasonable. We'll potentially allocate an\nunbounded amount of RAM before rejecting the patch, but there's not an\neasy fix there without teaching strbuf_read() to accept a maximum.\n\nIt's probably not worth worrying about given the attack surface here\n(it's not like anybody is reading patches from a socket that will cause\na DoS).\n\n-Peff\n"}]}