diff --git a/backport-CVE-2022-23521-attr-fix-integer-overflow-when-parsing-huge-attribut.patch b/backport-CVE-2022-23521-attr-fix-integer-overflow-when-parsing-huge-attribut.patch new file mode 100644 index 0000000000000000000000000000000000000000..08b24cf622f7b8a49d784603526c6a952b151335 --- /dev/null +++ b/backport-CVE-2022-23521-attr-fix-integer-overflow-when-parsing-huge-attribut.patch @@ -0,0 +1,55 @@ +From 24557209500e6ed618f04a8795a111a0c491a29c Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:23 +0100 +Subject: [PATCH] attr: fix integer overflow when parsing huge attribute names + +It is possible to trigger an integer overflow when parsing attribute +names that are longer than 2^31 bytes because we assign the result of +strlen(3P) to an `int` instead of to a `size_t`. This can lead to an +abort in vsnprintf(3P) with the following reproducer: + + blob=$(perl -e 'print "A " . "B"x2147483648 . "\n"' | git hash-object -w --stdin) + git update-index --add --cacheinfo 100644,$blob,.gitattributes + git check-attr --all path + + BUG: strbuf.c:400: your vsnprintf is broken (returned -1) + +But furthermore, assuming that the attribute name is even longer than +that, it can cause us to silently truncate the attribute and thus lead +to wrong results. + +Fix this integer overflow by using a `size_t` instead. This fixes the +silent truncation of attribute names, but it only partially fixes the +BUG we hit: even though the initial BUG is fixed, we can still hit a BUG +when parsing invalid attribute lines via `report_invalid_attr()`. + +This is due to an underlying design issue in vsnprintf(3P) which only +knows to return an `int`, and thus it may always overflow with large +inputs. This issue is benign though: the worst that can happen is that +the error message is misreported to be either truncated or too long, but +due to the buffer being NUL terminated we wouldn't ever do an +out-of-bounds read here. + +Reported-by: Markus Vervier +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/attr.c b/attr.c +index 9d42bc1721..4a10ba4d94 100644 +--- a/attr.c ++++ b/attr.c +@@ -289,7 +289,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp, + struct attr_state *e) + { + const char *ep, *equals; +- int len; ++ size_t len; + + ep = cp + strcspn(cp, blank); + equals = strchr(cp, '='); +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-fix-integer-overflow-with-more-than-INT_MAX-mac.patch b/backport-CVE-2022-23521-attr-fix-integer-overflow-with-more-than-INT_MAX-mac.patch new file mode 100644 index 0000000000000000000000000000000000000000..4485df03222e435682c1360b8f5f18ed7b356a2f --- /dev/null +++ b/backport-CVE-2022-23521-attr-fix-integer-overflow-with-more-than-INT_MAX-mac.patch @@ -0,0 +1,71 @@ +From e1e12e97ac73ded85f7d000da1063a774b3cc14f Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:36 +0100 +Subject: [PATCH] attr: fix integer overflow with more than INT_MAX macros + +Attributes have a field that tracks the position in the `all_attrs` +array they're stored inside. This field gets set via `hashmap_get_size` +when adding the attribute to the global map of attributes. But while the +field is of type `int`, the value returned by `hashmap_get_size` is an +`unsigned int`. It can thus happen that the value overflows, where we +would now dereference teh `all_attrs` array at an out-of-bounds value. + +We do have a sanity check for this overflow via an assert that verifies +the index matches the new hashmap's size. But asserts are not a proper +mechanism to detect against any such overflows as they may not in fact +be compiled into production code. + +Fix this by using an `unsigned int` to track the index and convert the +assert to a call `die()`. + +Reported-by: Jeff King +Signed-off-by: Junio C Hamano +--- + attr.c | 10 +++++----- + 1 file changed, 5 insertions(+), 5 deletions(-) + +diff --git a/attr.c b/attr.c +index 98c231d675..d1faf69083 100644 +--- a/attr.c ++++ b/attr.c +@@ -28,7 +28,7 @@ static const char git_attr__unknown[] = "(builtin)unknown"; + #endif + + struct git_attr { +- int attr_nr; /* unique attribute number */ ++ unsigned int attr_nr; /* unique attribute number */ + char name[FLEX_ARRAY]; /* attribute name */ + }; + +@@ -226,8 +226,8 @@ static const struct git_attr *git_attr_internal(const char *name, size_t namelen + a->attr_nr = hashmap_get_size(&g_attr_hashmap.map); + + attr_hashmap_add(&g_attr_hashmap, a->name, namelen, a); +- assert(a->attr_nr == +- (hashmap_get_size(&g_attr_hashmap.map) - 1)); ++ if (a->attr_nr != hashmap_get_size(&g_attr_hashmap.map) - 1) ++ die(_("unable to add additional attribute")); + } + + hashmap_unlock(&g_attr_hashmap); +@@ -1064,7 +1064,7 @@ static void determine_macros(struct all_attrs_item *all_attrs, + for (i = stack->num_matches; i > 0; i--) { + const struct match_attr *ma = stack->attrs[i - 1]; + if (ma->is_macro) { +- int n = ma->u.attr->attr_nr; ++ unsigned int n = ma->u.attr->attr_nr; + if (!all_attrs[n].macro) { + all_attrs[n].macro = ma; + } +@@ -1116,7 +1116,7 @@ void git_check_attr(const struct index_state *istate, + collect_some_attrs(istate, path, check); + + for (i = 0; i < check->nr; i++) { +- size_t n = check->items[i].attr->attr_nr; ++ unsigned int n = check->items[i].attr->attr_nr; + const char *value = check->all_attrs[n].value; + if (value == ATTR__UNKNOWN) + value = ATTR__UNSET; +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-fix-out-of-bounds-read-with-huge-attribute-name.patch b/backport-CVE-2022-23521-attr-fix-out-of-bounds-read-with-huge-attribute-name.patch new file mode 100644 index 0000000000000000000000000000000000000000..7ea8d9d0457495f2fd05e1377ff5c5b6438900d5 --- /dev/null +++ b/backport-CVE-2022-23521-attr-fix-out-of-bounds-read-with-huge-attribute-name.patch @@ -0,0 +1,68 @@ +From 8d0d48cf2157cfb914db1f53b3fe40785b86f3aa Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:19 +0100 +Subject: [PATCH] attr: fix out-of-bounds read with huge attribute names + +There is an out-of-bounds read possible when parsing gitattributes that +have an attribute that is 2^31+1 bytes long. This is caused due to an +integer overflow when we assign the result of strlen(3P) to an `int`, +where we use the wrapped-around value in a subsequent call to +memcpy(3P). The following code reproduces the issue: + + blob=$(perl -e 'print "a" x 2147483649 . " attr"' | git hash-object -w --stdin) + git update-index --add --cacheinfo 100644,$blob,.gitattributes + git check-attr --all file + + AddressSanitizer:DEADLYSIGNAL + ================================================================= + ==8451==ERROR: AddressSanitizer: SEGV on unknown address 0x7f93efa00800 (pc 0x7f94f1f8f082 bp 0x7ffddb59b3a0 sp 0x7ffddb59ab28 T0) + ==8451==The signal is caused by a READ memory access. + #0 0x7f94f1f8f082 (/usr/lib/libc.so.6+0x176082) + #1 0x7f94f2047d9c in __interceptor_strspn /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:752 + #2 0x560e190f7f26 in parse_attr_line attr.c:375 + #3 0x560e190f9663 in handle_attr_line attr.c:660 + #4 0x560e190f9ddd in read_attr_from_index attr.c:769 + #5 0x560e190f9f14 in read_attr attr.c:797 + #6 0x560e190fa24e in bootstrap_attr_stack attr.c:867 + #7 0x560e190fa4a5 in prepare_attr_stack attr.c:902 + #8 0x560e190fb5dc in collect_some_attrs attr.c:1097 + #9 0x560e190fb93f in git_all_attrs attr.c:1128 + #10 0x560e18e6136e in check_attr builtin/check-attr.c:67 + #11 0x560e18e61c12 in cmd_check_attr builtin/check-attr.c:183 + #12 0x560e18e15993 in run_builtin git.c:466 + #13 0x560e18e16397 in handle_builtin git.c:721 + #14 0x560e18e16b2b in run_argv git.c:788 + #15 0x560e18e17991 in cmd_main git.c:926 + #16 0x560e190ae2bd in main common-main.c:57 + #17 0x7f94f1e3c28f (/usr/lib/libc.so.6+0x2328f) + #18 0x7f94f1e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) + #19 0x560e18e110e4 in _start ../sysdeps/x86_64/start.S:115 + + AddressSanitizer can not provide additional info. + SUMMARY: AddressSanitizer: SEGV (/usr/lib/libc.so.6+0x176082) + ==8451==ABORTING + +Fix this bug by converting the variable to a `size_t` instead. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/attr.c b/attr.c +index 39ce0eb95e..9d42bc1721 100644 +--- a/attr.c ++++ b/attr.c +@@ -333,7 +333,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp, + static struct match_attr *parse_attr_line(const char *line, const char *src, + int lineno, int macro_ok) + { +- int namelen; ++ size_t namelen; + int num_attr, i; + const char *cp, *name, *states; + struct match_attr *res = NULL; +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-fix-out-of-bounds-read-with-unreasonable-amount.patch b/backport-CVE-2022-23521-attr-fix-out-of-bounds-read-with-unreasonable-amount.patch new file mode 100644 index 0000000000000000000000000000000000000000..ca73d5c88ee646aafec8c5c83c58cdfcdc35ed6a --- /dev/null +++ b/backport-CVE-2022-23521-attr-fix-out-of-bounds-read-with-unreasonable-amount.patch @@ -0,0 +1,84 @@ +From 447ac906e189535e77dcb1f4bbe3f1bc917d4c12 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:31 +0100 +Subject: [PATCH] attr: fix out-of-bounds read with unreasonable amount of + patterns + +The `struct attr_stack` tracks the stack of all patterns together with +their attributes. When parsing a gitattributes file that has more than +2^31 such patterns though we may trigger multiple out-of-bounds reads on +64 bit platforms. This is because while the `num_matches` variable is an +unsigned integer, we always use a signed integer to iterate over them. + +I have not been able to reproduce this issue due to memory constraints +on my systems. But despite the out-of-bounds reads, the worst thing that +can seemingly happen is to call free(3P) with a garbage pointer when +calling `attr_stack_free()`. + +Fix this bug by using unsigned integers to iterate over the array. While +this makes the iteration somewhat awkward when iterating in reverse, it +is at least better than knowingly running into an out-of-bounds read. +While at it, convert the call to `ALLOC_GROW` to use `ALLOC_GROW_BY` +instead. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 18 +++++++++--------- + 1 file changed, 9 insertions(+), 9 deletions(-) + +diff --git a/attr.c b/attr.c +index 525f6da201..98c231d675 100644 +--- a/attr.c ++++ b/attr.c +@@ -446,7 +446,7 @@ struct attr_stack { + + static void attr_stack_free(struct attr_stack *e) + { +- int i; ++ unsigned i; + free(e->origin); + for (i = 0; i < e->num_matches; i++) { + struct match_attr *a = e->attrs[i]; +@@ -660,8 +660,8 @@ static void handle_attr_line(struct attr_stack *res, + a = parse_attr_line(line, src, lineno, macro_ok); + if (!a) + return; +- ALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc); +- res->attrs[res->num_matches++] = a; ++ ALLOC_GROW_BY(res->attrs, res->num_matches, 1, res->alloc); ++ res->attrs[res->num_matches - 1] = a; + } + + static struct attr_stack *read_attr_from_array(const char **list) +@@ -1025,11 +1025,11 @@ static int fill(const char *path, int pathlen, int basename_offset, + struct all_attrs_item *all_attrs, int rem) + { + for (; rem > 0 && stack; stack = stack->prev) { +- int i; ++ unsigned i; + const char *base = stack->origin ? stack->origin : ""; + +- for (i = stack->num_matches - 1; 0 < rem && 0 <= i; i--) { +- const struct match_attr *a = stack->attrs[i]; ++ for (i = stack->num_matches; 0 < rem && 0 < i; i--) { ++ const struct match_attr *a = stack->attrs[i - 1]; + if (a->is_macro) + continue; + if (path_matches(path, pathlen, basename_offset, +@@ -1060,9 +1060,9 @@ static void determine_macros(struct all_attrs_item *all_attrs, + const struct attr_stack *stack) + { + for (; stack; stack = stack->prev) { +- int i; +- for (i = stack->num_matches - 1; i >= 0; i--) { +- const struct match_attr *ma = stack->attrs[i]; ++ unsigned i; ++ for (i = stack->num_matches; i > 0; i--) { ++ const struct match_attr *ma = stack->attrs[i - 1]; + if (ma->is_macro) { + int n = ma->u.attr->attr_nr; + if (!all_attrs[n].macro) { +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-fix-out-of-bounds-write-when-parsing-huge-numbe.patch b/backport-CVE-2022-23521-attr-fix-out-of-bounds-write-when-parsing-huge-numbe.patch new file mode 100644 index 0000000000000000000000000000000000000000..6864020048af65716b46d98fa27780607f171437 --- /dev/null +++ b/backport-CVE-2022-23521-attr-fix-out-of-bounds-write-when-parsing-huge-numbe.patch @@ -0,0 +1,203 @@ +From 34ace8bad02bb14ecc5b631f7e3daaa7a9bba7d9 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:27 +0100 +Subject: [PATCH] attr: fix out-of-bounds write when parsing huge number of + attributes + +It is possible to trigger an integer overflow when parsing attribute +names when there are more than 2^31 of them for a single pattern. This +can either lead to us dying due to trying to request too many bytes: + + blob=$(perl -e 'print "f" . " a=" x 2147483649' | git hash-object -w --stdin) + git update-index --add --cacheinfo 100644,$blob,.gitattributes + git attr-check --all file + + ================================================================= + ==1022==ERROR: AddressSanitizer: requested allocation size 0xfffffff800000032 (0xfffffff800001038 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0) + #0 0x7fd3efabf411 in __interceptor_calloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77 + #1 0x5563a0a1e3d3 in xcalloc wrapper.c:150 + #2 0x5563a058d005 in parse_attr_line attr.c:384 + #3 0x5563a058e661 in handle_attr_line attr.c:660 + #4 0x5563a058eddb in read_attr_from_index attr.c:769 + #5 0x5563a058ef12 in read_attr attr.c:797 + #6 0x5563a058f24c in bootstrap_attr_stack attr.c:867 + #7 0x5563a058f4a3 in prepare_attr_stack attr.c:902 + #8 0x5563a05905da in collect_some_attrs attr.c:1097 + #9 0x5563a059093d in git_all_attrs attr.c:1128 + #10 0x5563a02f636e in check_attr builtin/check-attr.c:67 + #11 0x5563a02f6c12 in cmd_check_attr builtin/check-attr.c:183 + #12 0x5563a02aa993 in run_builtin git.c:466 + #13 0x5563a02ab397 in handle_builtin git.c:721 + #14 0x5563a02abb2b in run_argv git.c:788 + #15 0x5563a02ac991 in cmd_main git.c:926 + #16 0x5563a05432bd in main common-main.c:57 + #17 0x7fd3ef82228f (/usr/lib/libc.so.6+0x2328f) + + ==1022==HINT: if you don't care about these errors you may set allocator_may_return_null=1 + SUMMARY: AddressSanitizer: allocation-size-too-big /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77 in __interceptor_calloc + ==1022==ABORTING + +Or, much worse, it can lead to an out-of-bounds write because we +underallocate and then memcpy(3P) into an array: + + perl -e ' + print "A " . "\rh="x2000000000; + print "\rh="x2000000000; + print "\rh="x294967294 . "\n" + ' >.gitattributes + git add .gitattributes + git commit -am "evil attributes" + + $ git clone --quiet /path/to/repo + ================================================================= + ==15062==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000002550 at pc 0x5555559884d5 bp 0x7fffffffbc60 sp 0x7fffffffbc58 + WRITE of size 8 at 0x602000002550 thread T0 + #0 0x5555559884d4 in parse_attr_line attr.c:393 + #1 0x5555559884d4 in handle_attr_line attr.c:660 + #2 0x555555988902 in read_attr_from_index attr.c:784 + #3 0x555555988902 in read_attr_from_index attr.c:747 + #4 0x555555988a1d in read_attr attr.c:800 + #5 0x555555989b0c in bootstrap_attr_stack attr.c:882 + #6 0x555555989b0c in prepare_attr_stack attr.c:917 + #7 0x555555989b0c in collect_some_attrs attr.c:1112 + #8 0x55555598b141 in git_check_attr attr.c:1126 + #9 0x555555a13004 in convert_attrs convert.c:1311 + #10 0x555555a95e04 in checkout_entry_ca entry.c:553 + #11 0x555555d58bf6 in checkout_entry entry.h:42 + #12 0x555555d58bf6 in check_updates unpack-trees.c:480 + #13 0x555555d5eb55 in unpack_trees unpack-trees.c:2040 + #14 0x555555785ab7 in checkout builtin/clone.c:724 + #15 0x555555785ab7 in cmd_clone builtin/clone.c:1384 + #16 0x55555572443c in run_builtin git.c:466 + #17 0x55555572443c in handle_builtin git.c:721 + #18 0x555555727872 in run_argv git.c:788 + #19 0x555555727872 in cmd_main git.c:926 + #20 0x555555721fa0 in main common-main.c:57 + #21 0x7ffff73f1d09 in __libc_start_main ../csu/libc-start.c:308 + #22 0x555555723f39 in _start (git+0x1cff39) + + 0x602000002552 is located 0 bytes to the right of 2-byte region [0x602000002550,0x602000002552) allocated by thread T0 here: + #0 0x7ffff768c037 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154 + #1 0x555555d7fff7 in xcalloc wrapper.c:150 + #2 0x55555598815f in parse_attr_line attr.c:384 + #3 0x55555598815f in handle_attr_line attr.c:660 + #4 0x555555988902 in read_attr_from_index attr.c:784 + #5 0x555555988902 in read_attr_from_index attr.c:747 + #6 0x555555988a1d in read_attr attr.c:800 + #7 0x555555989b0c in bootstrap_attr_stack attr.c:882 + #8 0x555555989b0c in prepare_attr_stack attr.c:917 + #9 0x555555989b0c in collect_some_attrs attr.c:1112 + #10 0x55555598b141 in git_check_attr attr.c:1126 + #11 0x555555a13004 in convert_attrs convert.c:1311 + #12 0x555555a95e04 in checkout_entry_ca entry.c:553 + #13 0x555555d58bf6 in checkout_entry entry.h:42 + #14 0x555555d58bf6 in check_updates unpack-trees.c:480 + #15 0x555555d5eb55 in unpack_trees unpack-trees.c:2040 + #16 0x555555785ab7 in checkout builtin/clone.c:724 + #17 0x555555785ab7 in cmd_clone builtin/clone.c:1384 + #18 0x55555572443c in run_builtin git.c:466 + #19 0x55555572443c in handle_builtin git.c:721 + #20 0x555555727872 in run_argv git.c:788 + #21 0x555555727872 in cmd_main git.c:926 + #22 0x555555721fa0 in main common-main.c:57 + #23 0x7ffff73f1d09 in __libc_start_main ../csu/libc-start.c:308 + + SUMMARY: AddressSanitizer: heap-buffer-overflow attr.c:393 in parse_attr_line + Shadow bytes around the buggy address: + 0x0c047fff8450: fa fa 00 02 fa fa 00 07 fa fa fd fd fa fa 00 00 + 0x0c047fff8460: fa fa 02 fa fa fa fd fd fa fa 00 06 fa fa 05 fa + 0x0c047fff8470: fa fa fd fd fa fa 00 02 fa fa 06 fa fa fa 05 fa + 0x0c047fff8480: fa fa 07 fa fa fa fd fd fa fa 00 01 fa fa 00 02 + 0x0c047fff8490: fa fa 00 03 fa fa 00 fa fa fa 00 01 fa fa 00 03 + =>0x0c047fff84a0: fa fa 00 01 fa fa 00 02 fa fa[02]fa fa fa fa fa + 0x0c047fff84b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c047fff84c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c047fff84d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c047fff84e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c047fff84f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + Shadow byte legend (one shadow byte represents 8 application bytes): + Addressable: 00 + Partially addressable: 01 02 03 04 05 06 07 + Heap left redzone: fa + Freed heap region: fd + Stack left redzone: f1 + Stack mid redzone: f2 + Stack right redzone: f3 + Stack after return: f5 + Stack use after scope: f8 + Global redzone: f9 + Global init order: f6 + Poisoned by user: f7 + Container overflow: fc + Array cookie: ac + Intra object redzone: bb + ASan internal: fe + Left alloca redzone: ca + Right alloca redzone: cb + Shadow gap: cc + ==15062==ABORTING + +Fix this bug by using `size_t` instead to count the number of attributes +so that this value cannot reasonably overflow without running out of +memory before already. + +Reported-by: Markus Vervier +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 16 ++++++++-------- + 1 file changed, 8 insertions(+), 8 deletions(-) + +diff --git a/attr.c b/attr.c +index 4a10ba4d94..525f6da201 100644 +--- a/attr.c ++++ b/attr.c +@@ -272,7 +272,7 @@ struct match_attr { + const struct git_attr *attr; + } u; + char is_macro; +- unsigned num_attr; ++ size_t num_attr; + struct attr_state state[FLEX_ARRAY]; + }; + +@@ -333,8 +333,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp, + static struct match_attr *parse_attr_line(const char *line, const char *src, + int lineno, int macro_ok) + { +- size_t namelen; +- int num_attr, i; ++ size_t namelen, num_attr, i; + const char *cp, *name, *states; + struct match_attr *res = NULL; + int is_macro; +@@ -451,7 +450,8 @@ static void attr_stack_free(struct attr_stack *e) + free(e->origin); + for (i = 0; i < e->num_matches; i++) { + struct match_attr *a = e->attrs[i]; +- int j; ++ size_t j; ++ + for (j = 0; j < a->num_attr; j++) { + const char *setto = a->state[j].setto; + if (setto == ATTR__TRUE || +@@ -1001,12 +1001,12 @@ static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem); + static int fill_one(const char *what, struct all_attrs_item *all_attrs, + const struct match_attr *a, int rem) + { +- int i; ++ size_t i; + +- for (i = a->num_attr - 1; rem > 0 && i >= 0; i--) { +- const struct git_attr *attr = a->state[i].attr; ++ for (i = a->num_attr; rem > 0 && i > 0; i--) { ++ const struct git_attr *attr = a->state[i - 1].attr; + const char **n = &(all_attrs[attr->attr_nr].value); +- const char *v = a->state[i].setto; ++ const char *v = a->state[i - 1].setto; + + if (*n == ATTR__UNKNOWN) { + debug_set(what, +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-fix-overflow-when-upserting-attribute-with-over.patch b/backport-CVE-2022-23521-attr-fix-overflow-when-upserting-attribute-with-over.patch new file mode 100644 index 0000000000000000000000000000000000000000..5887c74d9cbd409c4fae1f9da0088035ca1cc228 --- /dev/null +++ b/backport-CVE-2022-23521-attr-fix-overflow-when-upserting-attribute-with-over.patch @@ -0,0 +1,45 @@ +From eb22e7dfa23da6bd9aed9bd1dad69e1e8e167d24 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:15 +0100 +Subject: [PATCH] attr: fix overflow when upserting attribute with overly long + name + +The function `git_attr_internal()` is called to upsert attributes into +the global map. And while all callers pass a `size_t`, the function +itself accepts an `int` as the attribute name's length. This can lead to +an integer overflow in case the attribute name is longer than `INT_MAX`. + +Now this overflow seems harmless as the first thing we do is to call +`attr_name_valid()`, and that function only succeeds in case all chars +in the range of `namelen` match a certain small set of chars. We thus +can't do an out-of-bounds read as NUL is not part of that set and all +strings passed to this function are NUL-terminated. And furthermore, we +wouldn't ever read past the current attribute name anyway due to the +same reason. And if validation fails we will return early. + +On the other hand it feels fragile to rely on this behaviour, even more +so given that we pass `namelen` to `FLEX_ALLOC_MEM()`. So let's instead +just do the correct thing here and accept a `size_t` as line length. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/attr.c b/attr.c +index 4ef85d668b..39ce0eb95e 100644 +--- a/attr.c ++++ b/attr.c +@@ -210,7 +210,7 @@ static void report_invalid_attr(const char *name, size_t len, + * dictionary. If no entry is found, create a new attribute and store it in + * the dictionary. + */ +-static const struct git_attr *git_attr_internal(const char *name, int namelen) ++static const struct git_attr *git_attr_internal(const char *name, size_t namelen) + { + struct git_attr *a; + +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-fix-silently-splitting-up-lines-longer-than-204.patch b/backport-CVE-2022-23521-attr-fix-silently-splitting-up-lines-longer-than-204.patch new file mode 100644 index 0000000000000000000000000000000000000000..2061af64233dffff631b1b3cf587647994a1988d --- /dev/null +++ b/backport-CVE-2022-23521-attr-fix-silently-splitting-up-lines-longer-than-204.patch @@ -0,0 +1,91 @@ +From d74b1fd54fdbc45966d12ea907dece11e072fb2b Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:44 +0100 +Subject: [PATCH] attr: fix silently splitting up lines longer than 2048 bytes + +When reading attributes from a file we use fgets(3P) with a buffer size +of 2048 bytes. This means that as soon as a line exceeds the buffer size +we split it up into multiple parts and parse each of them as a separate +pattern line. This is of course not what the user intended, and even +worse the behaviour is inconsistent with how we read attributes from the +index. + +Fix this bug by converting the code to use `strbuf_getline()` instead. +This will indeed read in the whole line, which may theoretically lead to +an out-of-memory situation when the gitattributes file is huge. We're +about to reject any gitattributes files larger than 100MB in the next +commit though, which makes this less of a concern. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 13 +++++++------ + t/t0003-attributes.sh | 21 +++++++++++++++++++++ + 2 files changed, 28 insertions(+), 6 deletions(-) + +diff --git a/attr.c b/attr.c +index a9f7063cfc..41657479ff 100644 +--- a/attr.c ++++ b/attr.c +@@ -699,21 +699,22 @@ void git_attr_set_direction(enum git_attr_direction new_direction) + + static struct attr_stack *read_attr_from_file(const char *path, int macro_ok) + { ++ struct strbuf buf = STRBUF_INIT; + FILE *fp = fopen_or_warn(path, "r"); + struct attr_stack *res; +- char buf[2048]; + int lineno = 0; + + if (!fp) + return NULL; + res = xcalloc(1, sizeof(*res)); +- while (fgets(buf, sizeof(buf), fp)) { +- char *bufp = buf; +- if (!lineno) +- skip_utf8_bom(&bufp, strlen(bufp)); +- handle_attr_line(res, bufp, path, ++lineno, macro_ok); ++ while (strbuf_getline(&buf, fp) != EOF) { ++ if (!lineno && starts_with(buf.buf, utf8_bom)) ++ strbuf_remove(&buf, 0, strlen(utf8_bom)); ++ handle_attr_line(res, buf.buf, path, ++lineno, macro_ok); + } ++ + fclose(fp); ++ strbuf_release(&buf); + return res; + } + +diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh +index b660593c20..416386ce2f 100755 +--- a/t/t0003-attributes.sh ++++ b/t/t0003-attributes.sh +@@ -339,4 +339,25 @@ test_expect_success 'query binary macro directly' ' + test_cmp expect actual + ' + ++test_expect_success 'large attributes line ignores trailing content in tree' ' ++ test_when_finished "rm .gitattributes" && ++ # older versions of Git broke lines at 2048 bytes; the 2045 bytes ++ # of 0-padding here is accounting for the three bytes of "a 1", which ++ # would knock "trailing" to the "next" line, where it would be ++ # erroneously parsed. ++ printf "a %02045dtrailing attribute\n" 1 >.gitattributes && ++ git check-attr --all trailing >actual 2>err && ++ test_must_be_empty err && ++ test_must_be_empty actual ++' ++ ++test_expect_success 'large attributes line ignores trailing content in index' ' ++ test_when_finished "git update-index --remove .gitattributes" && ++ blob=$(printf "a %02045dtrailing attribute\n" 1 | git hash-object -w --stdin) && ++ git update-index --add --cacheinfo 100644,$blob,.gitattributes && ++ git check-attr --cached --all trailing >actual 2>err && ++ test_must_be_empty err && ++ test_must_be_empty actual ++' ++ + test_done +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-harden-allocation-against-integer-overflows.patch b/backport-CVE-2022-23521-attr-harden-allocation-against-integer-overflows.patch new file mode 100644 index 0000000000000000000000000000000000000000..b07a02f5b6f5eeeda667a7dd67accced39f9d436 --- /dev/null +++ b/backport-CVE-2022-23521-attr-harden-allocation-against-integer-overflows.patch @@ -0,0 +1,40 @@ +From a60a66e409c265b2944f18bf43581c146812586d Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:40 +0100 +Subject: [PATCH] attr: harden allocation against integer overflows + +When parsing an attributes line, we need to allocate an array that holds +all attributes specified for the given file pattern. The calculation to +determine the number of bytes that need to be allocated was prone to an +overflow though when there was an unreasonable amount of attributes. + +Harden the allocation by instead using the `st_` helper functions that +cause us to die when we hit an integer overflow. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 7 +++---- + 1 file changed, 3 insertions(+), 4 deletions(-) + +diff --git a/attr.c b/attr.c +index d1faf69083..a9f7063cfc 100644 +--- a/attr.c ++++ b/attr.c +@@ -380,10 +380,9 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, + goto fail_return; + } + +- res = xcalloc(1, +- sizeof(*res) + +- sizeof(struct attr_state) * num_attr + +- (is_macro ? 0 : namelen + 1)); ++ res = xcalloc(1, st_add3(sizeof(*res), ++ st_mult(sizeof(struct attr_state), num_attr), ++ is_macro ? 0 : namelen + 1)); + if (is_macro) { + res->u.attr = git_attr_internal(name, namelen); + } else { +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-ignore-attribute-lines-exceeding-2048-bytes.patch b/backport-CVE-2022-23521-attr-ignore-attribute-lines-exceeding-2048-bytes.patch new file mode 100644 index 0000000000000000000000000000000000000000..17c58c81aa5b5ca350bd068ecfc1308044396a64 --- /dev/null +++ b/backport-CVE-2022-23521-attr-ignore-attribute-lines-exceeding-2048-bytes.patch @@ -0,0 +1,120 @@ +From dfa6b32b5e599d97448337ed4fc18dd50c90758f Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:48 +0100 +Subject: [PATCH] attr: ignore attribute lines exceeding 2048 bytes + +There are two different code paths to read gitattributes: once via a +file, and once via the index. These two paths used to behave differently +because when reading attributes from a file, we used fgets(3P) with a +buffer size of 2kB. Consequentially, we silently truncate line lengths +when lines are longer than that and will then parse the remainder of the +line as a new pattern. It goes without saying that this is entirely +unexpected, but it's even worse that the behaviour depends on how the +gitattributes are parsed. + +While this is simply wrong, the silent truncation saves us with the +recently discovered vulnerabilities that can cause out-of-bound writes +or reads with unreasonably long lines due to integer overflows. As the +common path is to read gitattributes via the worktree file instead of +via the index, we can assume that any gitattributes file that had lines +longer than that is already broken anyway. So instead of lifting the +limit here, we can double down on it to fix the vulnerabilities. + +Introduce an explicit line length limit of 2kB that is shared across all +paths that read attributes and ignore any line that hits this limit +while printing a warning. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 5 +++++ + attr.h | 6 ++++++ + t/t0003-attributes.sh | 25 +++++++++++++++++++++++-- + 3 files changed, 34 insertions(+), 2 deletions(-) + +diff --git a/attr.c b/attr.c +index 41657479ff..38ecd2fff3 100644 +--- a/attr.c ++++ b/attr.c +@@ -344,6 +344,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, + return NULL; + name = cp; + ++ if (strlen(line) >= ATTR_MAX_LINE_LENGTH) { ++ warning(_("ignoring overly long attributes line %d"), lineno); ++ return NULL; ++ } ++ + if (*cp == '"' && !unquote_c_style(&pattern, name, &states)) { + name = pattern.buf; + namelen = pattern.len; +diff --git a/attr.h b/attr.h +index 404548f028..df9a75da55 100644 +--- a/attr.h ++++ b/attr.h +@@ -107,6 +107,12 @@ + * - Free the `attr_check` struct by calling `attr_check_free()`. + */ + ++/** ++ * The maximum line length for a gitattributes file. If the line exceeds this ++ * length we will ignore it. ++ */ ++#define ATTR_MAX_LINE_LENGTH 2048 ++ + struct index_state; + + /** +diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh +index 416386ce2f..7d68e6a56e 100755 +--- a/t/t0003-attributes.sh ++++ b/t/t0003-attributes.sh +@@ -339,6 +339,15 @@ test_expect_success 'query binary macro directly' ' + test_cmp expect actual + ' + ++test_expect_success 'large attributes line ignored in tree' ' ++ test_when_finished "rm .gitattributes" && ++ printf "path %02043d" 1 >.gitattributes && ++ git check-attr --all path >actual 2>err && ++ echo "warning: ignoring overly long attributes line 1" >expect && ++ test_cmp expect err && ++ test_must_be_empty actual ++' ++ + test_expect_success 'large attributes line ignores trailing content in tree' ' + test_when_finished "rm .gitattributes" && + # older versions of Git broke lines at 2048 bytes; the 2045 bytes +@@ -347,7 +356,18 @@ test_expect_success 'large attributes line ignores trailing content in tree' ' + # erroneously parsed. + printf "a %02045dtrailing attribute\n" 1 >.gitattributes && + git check-attr --all trailing >actual 2>err && +- test_must_be_empty err && ++ echo "warning: ignoring overly long attributes line 1" >expect && ++ test_cmp expect err && ++ test_must_be_empty actual ++' ++ ++test_expect_success 'large attributes line ignored in index' ' ++ test_when_finished "git update-index --remove .gitattributes" && ++ blob=$(printf "path %02043d" 1 | git hash-object -w --stdin) && ++ git update-index --add --cacheinfo 100644,$blob,.gitattributes && ++ git check-attr --cached --all path >actual 2>err && ++ echo "warning: ignoring overly long attributes line 1" >expect && ++ test_cmp expect err && + test_must_be_empty actual + ' + +@@ -356,7 +376,8 @@ test_expect_success 'large attributes line ignores trailing content in index' ' + blob=$(printf "a %02045dtrailing attribute\n" 1 | git hash-object -w --stdin) && + git update-index --add --cacheinfo 100644,$blob,.gitattributes && + git check-attr --cached --all trailing >actual 2>err && +- test_must_be_empty err && ++ echo "warning: ignoring overly long attributes line 1" >expect && ++ test_cmp expect err && + test_must_be_empty actual + ' + +-- +2.27.0 + diff --git a/backport-CVE-2022-23521-attr-ignore-overly-large-gitattributes-files.patch b/backport-CVE-2022-23521-attr-ignore-overly-large-gitattributes-files.patch new file mode 100644 index 0000000000000000000000000000000000000000..8352ce3b406386edb9c34edb26b0505173c585bb --- /dev/null +++ b/backport-CVE-2022-23521-attr-ignore-overly-large-gitattributes-files.patch @@ -0,0 +1,139 @@ +From 3c50032ff5289cc45659f21949c8d09e52164579 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:45:53 +0100 +Subject: [PATCH] attr: ignore overly large gitattributes files + +Similar as with the preceding commit, start ignoring gitattributes files +that are overly large to protect us against out-of-bounds reads and +writes caused by integer overflows. Unfortunately, we cannot just define +"overly large" in terms of any preexisting limits in the codebase. + +Instead, we choose a very conservative limit of 100MB. This is plenty of +room for specifying gitattributes, and incidentally it is also the limit +for blob sizes for GitHub. While we don't want GitHub to dictate limits +here, it is still sensible to use this fact for an informed decision +given that it is hosting a huge set of repositories. Furthermore, over +at GitLab we scanned a subset of repositories for their root-level +attribute files. We found that 80% of them have a gitattributes file +smaller than 100kB, 99.99% have one smaller than 1MB, and only a single +repository had one that was almost 3MB in size. So enforcing a limit of +100MB seems to give us ample of headroom. + +With this limit in place we can be reasonably sure that there is no easy +way to exploit the gitattributes file via integer overflows anymore. +Furthermore, it protects us against resource exhaustion caused by +allocating the in-memory data structures required to represent the +parsed attributes. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + attr.c | 24 ++++++++++++++++++++++-- + attr.h | 6 ++++++ + t/t0003-attributes.sh | 17 +++++++++++++++++ + 3 files changed, 45 insertions(+), 2 deletions(-) + +diff --git a/attr.c b/attr.c +index 38ecd2fff3..f9316d14ba 100644 +--- a/attr.c ++++ b/attr.c +@@ -708,10 +708,25 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok) + FILE *fp = fopen_or_warn(path, "r"); + struct attr_stack *res; + int lineno = 0; ++ int fd; ++ struct stat st; + + if (!fp) + return NULL; +- res = xcalloc(1, sizeof(*res)); ++ ++ fd = fileno(fp); ++ if (fstat(fd, &st)) { ++ warning_errno(_("cannot fstat gitattributes file '%s'"), path); ++ fclose(fp); ++ return NULL; ++ } ++ if (st.st_size >= ATTR_MAX_FILE_SIZE) { ++ warning(_("ignoring overly large gitattributes file '%s'"), path); ++ fclose(fp); ++ return NULL; ++ } ++ ++ CALLOC_ARRAY(res, 1); + while (strbuf_getline(&buf, fp) != EOF) { + if (!lineno && starts_with(buf.buf, utf8_bom)) + strbuf_remove(&buf, 0, strlen(utf8_bom)); +@@ -730,13 +745,18 @@ static struct attr_stack *read_attr_from_index(const struct index_state *istate, + struct attr_stack *res; + char *buf, *sp; + int lineno = 0; ++ size_t size; + + if (!istate) + return NULL; + +- buf = read_blob_data_from_index(istate, path, NULL); ++ buf = read_blob_data_from_index(istate, path, &size); + if (!buf) + return NULL; ++ if (size >= ATTR_MAX_FILE_SIZE) { ++ warning(_("ignoring overly large gitattributes blob '%s'"), path); ++ return NULL; ++ } + + res = xcalloc(1, sizeof(*res)); + for (sp = buf; *sp; ) { +diff --git a/attr.h b/attr.h +index df9a75da55..5970f930fd 100644 +--- a/attr.h ++++ b/attr.h +@@ -113,6 +113,12 @@ + */ + #define ATTR_MAX_LINE_LENGTH 2048 + ++ /** ++ * The maximum size of the giattributes file. If the file exceeds this size we ++ * will ignore it. ++ */ ++#define ATTR_MAX_FILE_SIZE (100 * 1024 * 1024) ++ + struct index_state; + + /** +diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh +index 7d68e6a56e..9d9aa2855d 100755 +--- a/t/t0003-attributes.sh ++++ b/t/t0003-attributes.sh +@@ -361,6 +361,14 @@ test_expect_success 'large attributes line ignores trailing content in tree' ' + test_must_be_empty actual + ' + ++test_expect_success EXPENSIVE 'large attributes file ignored in tree' ' ++ test_when_finished "rm .gitattributes" && ++ dd if=/dev/zero of=.gitattributes bs=101M count=1 2>/dev/null && ++ git check-attr --all path >/dev/null 2>err && ++ echo "warning: ignoring overly large gitattributes file ${SQ}.gitattributes${SQ}" >expect && ++ test_cmp expect err ++' ++ + test_expect_success 'large attributes line ignored in index' ' + test_when_finished "git update-index --remove .gitattributes" && + blob=$(printf "path %02043d" 1 | git hash-object -w --stdin) && +@@ -381,4 +389,13 @@ test_expect_success 'large attributes line ignores trailing content in index' ' + test_must_be_empty actual + ' + ++test_expect_success EXPENSIVE 'large attributes file ignored in index' ' ++ test_when_finished "git update-index --remove .gitattributes" && ++ blob=$(dd if=/dev/zero bs=101M count=1 2>/dev/null | git hash-object -w --stdin) && ++ git update-index --add --cacheinfo 100644,$blob,.gitattributes && ++ git check-attr --cached --all path >/dev/null 2>err && ++ echo "warning: ignoring overly large gitattributes blob ${SQ}.gitattributes${SQ}" >expect && ++ test_cmp expect err ++' ++ + test_done +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-pretty-fix-adding-linefeed-when-placeholder-is-not-e.patch b/backport-CVE-2022-41903-pretty-fix-adding-linefeed-when-placeholder-is-not-e.patch new file mode 100644 index 0000000000000000000000000000000000000000..cebdc8f1d879e34f5d9272587ec358530aad3fd4 --- /dev/null +++ b/backport-CVE-2022-41903-pretty-fix-adding-linefeed-when-placeholder-is-not-e.patch @@ -0,0 +1,94 @@ +From 1de69c0cdd388b0a5b7bdde0bfa0bda514a354b0 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:46:39 +0100 +Subject: [PATCH] pretty: fix adding linefeed when placeholder is not expanded + +When a formatting directive has a `+` or ` ` after the `%`, then we add +either a line feed or space if the placeholder expands to a non-empty +string. In specific cases though this logic doesn't work as expected, +and we try to add the character even in the case where the formatting +directive is empty. + +One such pattern is `%w(1)%+d%+w(2)`. `%+d` expands to reference names +pointing to a certain commit, like in `git log --decorate`. For a tagged +commit this would for example expand to `\n (tag: v1.0.0)`, which has a +leading newline due to the `+` modifier and a space added by `%d`. Now +the second wrapping directive will cause us to rewrap the text to +`\n(tag:\nv1.0.0)`, which is one byte shorter due to the missing leading +space. The code that handles the `+` magic now notices that the length +has changed and will thus try to insert a leading line feed at the +original posititon. But as the string was shortened, the original +position is past the buffer's boundary and thus we die with an error. + +Now there are two issues here: + + 1. We check whether the buffer length has changed, not whether it + has been extended. This causes us to try and add the character + past the string boundary. + + 2. The current logic does not make any sense whatsoever. When the + string got expanded due to the rewrap, putting the separator into + the original position is likely to put it somewhere into the + middle of the rewrapped contents. + +It is debatable whether `%+w()` makes any sense in the first place. +Strictly speaking, the placeholder never expands to a non-empty string, +and consequentially we shouldn't ever accept this combination. We thus +fix the bug by simply refusing `%+w()`. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + pretty.c | 14 +++++++++++++- + t/t4205-log-pretty-formats.sh | 8 ++++++++ + 2 files changed, 21 insertions(+), 1 deletion(-) + +diff --git a/pretty.c b/pretty.c +index d55b88607a..c6c757c0ce 100644 +--- a/pretty.c ++++ b/pretty.c +@@ -1597,9 +1597,21 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ + default: + break; + } +- if (magic != NO_MAGIC) ++ if (magic != NO_MAGIC) { + placeholder++; + ++ switch (placeholder[0]) { ++ case 'w': ++ /* ++ * `%+w()` cannot ever expand to a non-empty string, ++ * and it potentially changes the layout of preceding ++ * contents. We're thus not able to handle the magic in ++ * this combination and refuse the pattern. ++ */ ++ return 0; ++ }; ++ } ++ + orig_len = sb->len; + if (((struct format_commit_context *)context)->flush_type != no_flush) + consumed = format_and_pad_commit(sb, placeholder, context); +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index cdde37d325..1d768f7244 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -879,6 +879,14 @@ test_expect_success 'log --pretty with invalid padding format' ' + test_cmp expect actual + ' + ++test_expect_success 'log --pretty with magical wrapping directives' ' ++ commit_id=$(git commit-tree HEAD^{tree} -m "describe me") && ++ git tag describe-me $commit_id && ++ printf "\n(tag:\ndescribe-me)%%+w(2)" >expect && ++ git log -1 --pretty="format:%w(1)%+d%+w(2)" $commit_id >actual && ++ test_cmp expect actual ++' ++ + test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' ' + # We only assert that this command does not crash. This needs to be + # executed with the address sanitizer to demonstrate failure. +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-pretty-fix-integer-overflow-in-wrapping-format.patch b/backport-CVE-2022-41903-pretty-fix-integer-overflow-in-wrapping-format.patch new file mode 100644 index 0000000000000000000000000000000000000000..61ec001f7056b755a5b199fdda79b361ebb49f01 --- /dev/null +++ b/backport-CVE-2022-41903-pretty-fix-integer-overflow-in-wrapping-format.patch @@ -0,0 +1,86 @@ +From 48050c42c73c28b0c001d63d11dffac7e116847b Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:46:49 +0100 +Subject: [PATCH] pretty: fix integer overflow in wrapping format + +The `%w(width,indent1,indent2)` formatting directive can be used to +rewrap text to a specific width and is designed after git-shortlog(1)'s +`-w` parameter. While the three parameters are all stored as `size_t` +internally, `strbuf_add_wrapped_text()` accepts integers as input. As a +result, the casted integers may overflow. As these now-negative integers +are later on passed to `strbuf_addchars()`, we will ultimately run into +implementation-defined behaviour due to casting a negative number back +to `size_t` again. On my platform, this results in trying to allocate +9000 petabyte of memory. + +Fix this overflow by using `cast_size_t_to_int()` so that we reject +inputs that cannot be represented as an integer. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + git-compat-util.h | 8 ++++++++ + pretty.c | 4 +++- + t/t4205-log-pretty-formats.sh | 12 ++++++++++++ + 3 files changed, 23 insertions(+), 1 deletion(-) + +diff --git a/git-compat-util.h b/git-compat-util.h +index f505f817d5..0ac1b7f560 100644 +--- a/git-compat-util.h ++++ b/git-compat-util.h +@@ -918,6 +918,14 @@ static inline size_t st_sub(size_t a, size_t b) + return a - b; + } + ++static inline int cast_size_t_to_int(size_t a) ++{ ++ if (a > INT_MAX) ++ die("number too large to represent as int on this platform: %"PRIuMAX, ++ (uintmax_t)a); ++ return (int)a; ++} ++ + #ifdef HAVE_ALLOCA_H + # include + # define xalloca(size) (alloca(size)) +diff --git a/pretty.c b/pretty.c +index c6c757c0ce..7e649b1cec 100644 +--- a/pretty.c ++++ b/pretty.c +@@ -915,7 +915,9 @@ static void strbuf_wrap(struct strbuf *sb, size_t pos, + if (pos) + strbuf_add(&tmp, sb->buf, pos); + strbuf_add_wrapped_text(&tmp, sb->buf + pos, +- (int) indent1, (int) indent2, (int) width); ++ cast_size_t_to_int(indent1), ++ cast_size_t_to_int(indent2), ++ cast_size_t_to_int(width)); + strbuf_swap(&tmp, sb); + strbuf_release(&tmp); + } +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index 1d768f7244..c88b64d08b 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -887,6 +887,18 @@ test_expect_success 'log --pretty with magical wrapping directives' ' + test_cmp expect actual + ' + ++test_expect_success SIZE_T_IS_64BIT 'log --pretty with overflowing wrapping directive' ' ++ cat >expect <<-EOF && ++ fatal: number too large to represent as int on this platform: 2147483649 ++ EOF ++ test_must_fail git log -1 --pretty="format:%w(2147483649,1,1)%d" 2>error && ++ test_cmp expect error && ++ test_must_fail git log -1 --pretty="format:%w(1,2147483649,1)%d" 2>error && ++ test_cmp expect error && ++ test_must_fail git log -1 --pretty="format:%w(1,1,2147483649)%d" 2>error && ++ test_cmp expect error ++' ++ + test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' ' + # We only assert that this command does not crash. This needs to be + # executed with the address sanitizer to demonstrate failure. +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-pretty-fix-out-of-bounds-read-when-left-flushing-wit.patch b/backport-CVE-2022-41903-pretty-fix-out-of-bounds-read-when-left-flushing-wit.patch new file mode 100644 index 0000000000000000000000000000000000000000..b44aa71c4cd37f6345aea2d8170690669580a969 --- /dev/null +++ b/backport-CVE-2022-41903-pretty-fix-out-of-bounds-read-when-left-flushing-wit.patch @@ -0,0 +1,143 @@ +From b49f309aa16febeddb65e82526640a91bbba3be3 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:46:30 +0100 +Subject: [PATCH] pretty: fix out-of-bounds read when left-flushing with + stealing + +With the `%>>()` pretty formatter, you can ask git-log(1) et al to +steal spaces. To do so we need to look ahead of the next token to see +whether there are spaces there. This loop takes into account ANSI +sequences that end with an `m`, and if it finds any it will skip them +until it finds the first space. While doing so it does not take into +account the buffer's limits though and easily does an out-of-bounds +read. + +Add a test that hits this behaviour. While we don't have an easy way to +verify this, the test causes the following failure when run with +`SANITIZE=address`: + + ==37941==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000000baf at pc 0x55ba6f88e0d0 bp 0x7ffc84c50d20 sp 0x7ffc84c50d10 + READ of size 1 at 0x603000000baf thread T0 + #0 0x55ba6f88e0cf in format_and_pad_commit pretty.c:1712 + #1 0x55ba6f88e7b4 in format_commit_item pretty.c:1801 + #2 0x55ba6f9b1ae4 in strbuf_expand strbuf.c:429 + #3 0x55ba6f88f020 in repo_format_commit_message pretty.c:1869 + #4 0x55ba6f890ccf in pretty_print_commit pretty.c:2161 + #5 0x55ba6f7884c8 in show_log log-tree.c:781 + #6 0x55ba6f78b6ba in log_tree_commit log-tree.c:1117 + #7 0x55ba6f40fed5 in cmd_log_walk_no_free builtin/log.c:508 + #8 0x55ba6f41035b in cmd_log_walk builtin/log.c:549 + #9 0x55ba6f4131a2 in cmd_log builtin/log.c:883 + #10 0x55ba6f2ea993 in run_builtin git.c:466 + #11 0x55ba6f2eb397 in handle_builtin git.c:721 + #12 0x55ba6f2ebb07 in run_argv git.c:788 + #13 0x55ba6f2ec8a7 in cmd_main git.c:923 + #14 0x55ba6f581682 in main common-main.c:57 + #15 0x7f2d08c3c28f (/usr/lib/libc.so.6+0x2328f) + #16 0x7f2d08c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) + #17 0x55ba6f2e60e4 in _start ../sysdeps/x86_64/start.S:115 + + 0x603000000baf is located 1 bytes to the left of 24-byte region [0x603000000bb0,0x603000000bc8) + allocated by thread T0 here: + #0 0x7f2d08ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85 + #1 0x55ba6fa5b494 in xrealloc wrapper.c:136 + #2 0x55ba6f9aefdc in strbuf_grow strbuf.c:99 + #3 0x55ba6f9b0a06 in strbuf_add strbuf.c:298 + #4 0x55ba6f9b1a25 in strbuf_expand strbuf.c:418 + #5 0x55ba6f88f020 in repo_format_commit_message pretty.c:1869 + #6 0x55ba6f890ccf in pretty_print_commit pretty.c:2161 + #7 0x55ba6f7884c8 in show_log log-tree.c:781 + #8 0x55ba6f78b6ba in log_tree_commit log-tree.c:1117 + #9 0x55ba6f40fed5 in cmd_log_walk_no_free builtin/log.c:508 + #10 0x55ba6f41035b in cmd_log_walk builtin/log.c:549 + #11 0x55ba6f4131a2 in cmd_log builtin/log.c:883 + #12 0x55ba6f2ea993 in run_builtin git.c:466 + #13 0x55ba6f2eb397 in handle_builtin git.c:721 + #14 0x55ba6f2ebb07 in run_argv git.c:788 + #15 0x55ba6f2ec8a7 in cmd_main git.c:923 + #16 0x55ba6f581682 in main common-main.c:57 + #17 0x7f2d08c3c28f (/usr/lib/libc.so.6+0x2328f) + #18 0x7f2d08c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) + #19 0x55ba6f2e60e4 in _start ../sysdeps/x86_64/start.S:115 + + SUMMARY: AddressSanitizer: heap-buffer-overflow pretty.c:1712 in format_and_pad_commit + Shadow bytes around the buggy address: + 0x0c067fff8120: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd + 0x0c067fff8130: fd fd fa fa fd fd fd fd fa fa fd fd fd fa fa fa + 0x0c067fff8140: fd fd fd fa fa fa fd fd fd fa fa fa fd fd fd fa + 0x0c067fff8150: fa fa fd fd fd fd fa fa 00 00 00 fa fa fa fd fd + 0x0c067fff8160: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa + =>0x0c067fff8170: fd fd fd fa fa[fa]00 00 00 fa fa fa 00 00 00 fa + 0x0c067fff8180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c067fff8190: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c067fff81a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c067fff81b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c067fff81c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + Shadow byte legend (one shadow byte represents 8 application bytes): + Addressable: 00 + Partially addressable: 01 02 03 04 05 06 07 + Heap left redzone: fa + Freed heap region: fd + Stack left redzone: f1 + Stack mid redzone: f2 + Stack right redzone: f3 + Stack after return: f5 + Stack use after scope: f8 + Global redzone: f9 + Global init order: f6 + Poisoned by user: f7 + Container overflow: fc + Array cookie: ac + Intra object redzone: bb + ASan internal: fe + Left alloca redzone: ca + Right alloca redzone: cb + +Luckily enough, this would only cause us to copy the out-of-bounds data +into the formatted commit in case we really had an ANSI sequence +preceding our buffer. So this bug likely has no security consequences. + +Fix it regardless by not traversing past the buffer's start. + +Reported-by: Patrick Steinhardt +Reported-by: Eric Sesterhenn +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + pretty.c | 2 +- + t/t4205-log-pretty-formats.sh | 6 ++++++ + 2 files changed, 7 insertions(+), 1 deletion(-) + +diff --git a/pretty.c b/pretty.c +index a1a01492c1..692a6382a1 100644 +--- a/pretty.c ++++ b/pretty.c +@@ -1514,7 +1514,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ + if (*ch != 'm') + break; + p = ch - 1; +- while (ch - p < 10 && *p != '\033') ++ while (p > sb->buf && ch - p < 10 && *p != '\033') + p--; + if (*p != '\033' || + ch + 1 - p != display_mode_esc_sequence_len(p)) +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index fff3e05615..126dc20f23 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -867,6 +867,12 @@ test_expect_success 'log --pretty=reference is colored appropriately' ' + test_cmp expect actual + ' + ++test_expect_success 'log --pretty with space stealing' ' ++ printf mm0 >expect && ++ git log -1 --pretty="format:mm%>>|(1)%x30" >actual && ++ test_cmp expect actual ++' ++ + test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' ' + # We only assert that this command does not crash. This needs to be + # executed with the address sanitizer to demonstrate failure. +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-pretty-fix-out-of-bounds-read-when-parsing-invalid-p.patch b/backport-CVE-2022-41903-pretty-fix-out-of-bounds-read-when-parsing-invalid-p.patch new file mode 100644 index 0000000000000000000000000000000000000000..9131a063c1a8b39d215ba5d4cb7836dee3b2b1e9 --- /dev/null +++ b/backport-CVE-2022-41903-pretty-fix-out-of-bounds-read-when-parsing-invalid-p.patch @@ -0,0 +1,147 @@ +From f6e0b9f38987ad5e47bab551f8760b70689a5905 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:46:34 +0100 +Subject: [PATCH] pretty: fix out-of-bounds read when parsing invalid padding + format + +An out-of-bounds read can be triggered when parsing an incomplete +padding format string passed via `--pretty=format` or in Git archives +when files are marked with the `export-subst` gitattribute. + +This bug exists since we have introduced support for truncating output +via the `trunc` keyword a7f01c6b4d (pretty: support truncating in %>, %< +and %><, 2013-04-19). Before this commit, we used to find the end of the +formatting string by using strchr(3P). This function returns a `NULL` +pointer in case the character in question wasn't found. The subsequent +check whether any character was found thus simply checked the returned +pointer. After the commit we switched to strcspn(3P) though, which only +returns the offset to the first found character or to the trailing NUL +byte. As the end pointer is now computed by adding the offset to the +start pointer it won't be `NULL` anymore, and as a consequence the check +doesn't do anything anymore. + +The out-of-bounds data that is being read can in fact end up in the +formatted string. As a consequence, it is possible to leak memory +contents either by calling git-log(1) or via git-archive(1) when any of +the archived files is marked with the `export-subst` gitattribute. + + ==10888==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000398 at pc 0x7f0356047cb2 bp 0x7fff3ffb95d0 sp 0x7fff3ffb8d78 + READ of size 1 at 0x602000000398 thread T0 + #0 0x7f0356047cb1 in __interceptor_strchrnul /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:725 + #1 0x563b7cec9a43 in strbuf_expand strbuf.c:417 + #2 0x563b7cda7060 in repo_format_commit_message pretty.c:1869 + #3 0x563b7cda8d0f in pretty_print_commit pretty.c:2161 + #4 0x563b7cca04c8 in show_log log-tree.c:781 + #5 0x563b7cca36ba in log_tree_commit log-tree.c:1117 + #6 0x563b7c927ed5 in cmd_log_walk_no_free builtin/log.c:508 + #7 0x563b7c92835b in cmd_log_walk builtin/log.c:549 + #8 0x563b7c92b1a2 in cmd_log builtin/log.c:883 + #9 0x563b7c802993 in run_builtin git.c:466 + #10 0x563b7c803397 in handle_builtin git.c:721 + #11 0x563b7c803b07 in run_argv git.c:788 + #12 0x563b7c8048a7 in cmd_main git.c:923 + #13 0x563b7ca99682 in main common-main.c:57 + #14 0x7f0355e3c28f (/usr/lib/libc.so.6+0x2328f) + #15 0x7f0355e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) + #16 0x563b7c7fe0e4 in _start ../sysdeps/x86_64/start.S:115 + + 0x602000000398 is located 0 bytes to the right of 8-byte region [0x602000000390,0x602000000398) + allocated by thread T0 here: + #0 0x7f0356072faa in __interceptor_strdup /usr/src/debug/gcc/libsanitizer/asan/asan_interceptors.cpp:439 + #1 0x563b7cf7317c in xstrdup wrapper.c:39 + #2 0x563b7cd9a06a in save_user_format pretty.c:40 + #3 0x563b7cd9b3e5 in get_commit_format pretty.c:173 + #4 0x563b7ce54ea0 in handle_revision_opt revision.c:2456 + #5 0x563b7ce597c9 in setup_revisions revision.c:2850 + #6 0x563b7c9269e0 in cmd_log_init_finish builtin/log.c:269 + #7 0x563b7c927362 in cmd_log_init builtin/log.c:348 + #8 0x563b7c92b193 in cmd_log builtin/log.c:882 + #9 0x563b7c802993 in run_builtin git.c:466 + #10 0x563b7c803397 in handle_builtin git.c:721 + #11 0x563b7c803b07 in run_argv git.c:788 + #12 0x563b7c8048a7 in cmd_main git.c:923 + #13 0x563b7ca99682 in main common-main.c:57 + #14 0x7f0355e3c28f (/usr/lib/libc.so.6+0x2328f) + #15 0x7f0355e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) + #16 0x563b7c7fe0e4 in _start ../sysdeps/x86_64/start.S:115 + + SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:725 in __interceptor_strchrnul + Shadow bytes around the buggy address: + 0x0c047fff8020: fa fa fd fd fa fa 00 06 fa fa 05 fa fa fa fd fd + 0x0c047fff8030: fa fa 00 02 fa fa 06 fa fa fa 05 fa fa fa fd fd + 0x0c047fff8040: fa fa 00 07 fa fa 03 fa fa fa fd fd fa fa 00 00 + 0x0c047fff8050: fa fa 00 01 fa fa fd fd fa fa 00 00 fa fa 00 01 + 0x0c047fff8060: fa fa 00 06 fa fa 00 06 fa fa 05 fa fa fa 05 fa + =>0x0c047fff8070: fa fa 00[fa]fa fa fd fa fa fa fd fd fa fa fd fd + 0x0c047fff8080: fa fa fd fd fa fa 00 00 fa fa 00 fa fa fa fd fa + 0x0c047fff8090: fa fa fd fd fa fa 00 00 fa fa fa fa fa fa fa fa + 0x0c047fff80a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c047fff80b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c047fff80c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + Shadow byte legend (one shadow byte represents 8 application bytes): + Addressable: 00 + Partially addressable: 01 02 03 04 05 06 07 + Heap left redzone: fa + Freed heap region: fd + Stack left redzone: f1 + Stack mid redzone: f2 + Stack right redzone: f3 + Stack after return: f5 + Stack use after scope: f8 + Global redzone: f9 + Global init order: f6 + Poisoned by user: f7 + Container overflow: fc + Array cookie: ac + Intra object redzone: bb + ASan internal: fe + Left alloca redzone: ca + Right alloca redzone: cb + ==10888==ABORTING + +Fix this bug by checking whether `end` points at the trailing NUL byte. +Add a test which catches this out-of-bounds read and which demonstrates +that we used to write out-of-bounds data into the formatted message. + +Reported-by: Markus Vervier +Original-patch-by: Markus Vervier +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + pretty.c | 2 +- + t/t4205-log-pretty-formats.sh | 6 ++++++ + 2 files changed, 7 insertions(+), 1 deletion(-) + +diff --git a/pretty.c b/pretty.c +index 692a6382a1..d55b88607a 100644 +--- a/pretty.c ++++ b/pretty.c +@@ -1041,7 +1041,7 @@ static size_t parse_padding_placeholder(const char *placeholder, + const char *end = start + strcspn(start, ",)"); + char *next; + int width; +- if (!end || end == start) ++ if (!*end || end == start) + return 0; + width = strtol(start, &next, 10); + if (next == start || width == 0) +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index 126dc20f23..cdde37d325 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -873,6 +873,12 @@ test_expect_success 'log --pretty with space stealing' ' + test_cmp expect actual + ' + ++test_expect_success 'log --pretty with invalid padding format' ' ++ printf "%s%%<(20" "$(git rev-parse HEAD)" >expect && ++ git log -1 --pretty="format:%H%<(20" >actual && ++ test_cmp expect actual ++' ++ + test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' ' + # We only assert that this command does not crash. This needs to be + # executed with the address sanitizer to demonstrate failure. +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-pretty-fix-out-of-bounds-write-caused-by-integer-ove.patch b/backport-CVE-2022-41903-pretty-fix-out-of-bounds-write-caused-by-integer-ove.patch new file mode 100644 index 0000000000000000000000000000000000000000..94ce4e0716bd4a6e827dbeea4a68678b180142cb --- /dev/null +++ b/backport-CVE-2022-41903-pretty-fix-out-of-bounds-write-caused-by-integer-ove.patch @@ -0,0 +1,183 @@ +From 81dc898df9b4b4035534a927f3234a3839b698bf Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:46:25 +0100 +Subject: [PATCH] pretty: fix out-of-bounds write caused by integer overflow + +When using a padding specifier in the pretty format passed to git-log(1) +we need to calculate the string length in several places. These string +lengths are stored in `int`s though, which means that these can easily +overflow when the input lengths exceeds 2GB. This can ultimately lead to +an out-of-bounds write when these are used in a call to memcpy(3P): + + ==8340==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f1ec62f97fe at pc 0x7f2127e5f427 bp 0x7ffd3bd63de0 sp 0x7ffd3bd63588 + WRITE of size 1 at 0x7f1ec62f97fe thread T0 + #0 0x7f2127e5f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 + #1 0x5628e96aa605 in format_and_pad_commit pretty.c:1762 + #2 0x5628e96aa7f4 in format_commit_item pretty.c:1801 + #3 0x5628e97cdb24 in strbuf_expand strbuf.c:429 + #4 0x5628e96ab060 in repo_format_commit_message pretty.c:1869 + #5 0x5628e96acd0f in pretty_print_commit pretty.c:2161 + #6 0x5628e95a44c8 in show_log log-tree.c:781 + #7 0x5628e95a76ba in log_tree_commit log-tree.c:1117 + #8 0x5628e922bed5 in cmd_log_walk_no_free builtin/log.c:508 + #9 0x5628e922c35b in cmd_log_walk builtin/log.c:549 + #10 0x5628e922f1a2 in cmd_log builtin/log.c:883 + #11 0x5628e9106993 in run_builtin git.c:466 + #12 0x5628e9107397 in handle_builtin git.c:721 + #13 0x5628e9107b07 in run_argv git.c:788 + #14 0x5628e91088a7 in cmd_main git.c:923 + #15 0x5628e939d682 in main common-main.c:57 + #16 0x7f2127c3c28f (/usr/lib/libc.so.6+0x2328f) + #17 0x7f2127c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) + #18 0x5628e91020e4 in _start ../sysdeps/x86_64/start.S:115 + + 0x7f1ec62f97fe is located 2 bytes to the left of 4831838265-byte region [0x7f1ec62f9800,0x7f1fe62f9839) + allocated by thread T0 here: + #0 0x7f2127ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85 + #1 0x5628e98774d4 in xrealloc wrapper.c:136 + #2 0x5628e97cb01c in strbuf_grow strbuf.c:99 + #3 0x5628e97ccd42 in strbuf_addchars strbuf.c:327 + #4 0x5628e96aa55c in format_and_pad_commit pretty.c:1761 + #5 0x5628e96aa7f4 in format_commit_item pretty.c:1801 + #6 0x5628e97cdb24 in strbuf_expand strbuf.c:429 + #7 0x5628e96ab060 in repo_format_commit_message pretty.c:1869 + #8 0x5628e96acd0f in pretty_print_commit pretty.c:2161 + #9 0x5628e95a44c8 in show_log log-tree.c:781 + #10 0x5628e95a76ba in log_tree_commit log-tree.c:1117 + #11 0x5628e922bed5 in cmd_log_walk_no_free builtin/log.c:508 + #12 0x5628e922c35b in cmd_log_walk builtin/log.c:549 + #13 0x5628e922f1a2 in cmd_log builtin/log.c:883 + #14 0x5628e9106993 in run_builtin git.c:466 + #15 0x5628e9107397 in handle_builtin git.c:721 + #16 0x5628e9107b07 in run_argv git.c:788 + #17 0x5628e91088a7 in cmd_main git.c:923 + #18 0x5628e939d682 in main common-main.c:57 + #19 0x7f2127c3c28f (/usr/lib/libc.so.6+0x2328f) + #20 0x7f2127c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) + #21 0x5628e91020e4 in _start ../sysdeps/x86_64/start.S:115 + + SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy + Shadow bytes around the buggy address: + 0x0fe458c572a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0fe458c572b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0fe458c572c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0fe458c572d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0fe458c572e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + =>0x0fe458c572f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa] + 0x0fe458c57300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + 0x0fe458c57310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + 0x0fe458c57320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + 0x0fe458c57330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + 0x0fe458c57340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + Shadow byte legend (one shadow byte represents 8 application bytes): + Addressable: 00 + Partially addressable: 01 02 03 04 05 06 07 + Heap left redzone: fa + Freed heap region: fd + Stack left redzone: f1 + Stack mid redzone: f2 + Stack right redzone: f3 + Stack after return: f5 + Stack use after scope: f8 + Global redzone: f9 + Global init order: f6 + Poisoned by user: f7 + Container overflow: fc + Array cookie: ac + Intra object redzone: bb + ASan internal: fe + Left alloca redzone: ca + Right alloca redzone: cb + ==8340==ABORTING + +The pretty format can also be used in `git archive` operations via the +`export-subst` attribute. So this is what in our opinion makes this a +critical issue in the context of Git forges which allow to download an +archive of user supplied Git repositories. + +Fix this vulnerability by using `size_t` instead of `int` to track the +string lengths. Add tests which detect this vulnerability when Git is +compiled with the address sanitizer. + +Reported-by: Joern Schneeweisz +Original-patch-by: Joern Schneeweisz +Modified-by: Taylor Blau +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + pretty.c | 11 ++++++----- + t/t4205-log-pretty-formats.sh | 17 +++++++++++++++++ + 2 files changed, 23 insertions(+), 5 deletions(-) + +diff --git a/pretty.c b/pretty.c +index 7a7708a0ea..a1a01492c1 100644 +--- a/pretty.c ++++ b/pretty.c +@@ -1473,7 +1473,9 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ + struct format_commit_context *c) + { + struct strbuf local_sb = STRBUF_INIT; +- int total_consumed = 0, len, padding = c->padding; ++ size_t total_consumed = 0; ++ int len, padding = c->padding; ++ + if (padding < 0) { + const char *start = strrchr(sb->buf, '\n'); + int occupied; +@@ -1485,7 +1487,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ + } + while (1) { + int modifier = *placeholder == 'C'; +- int consumed = format_commit_one(&local_sb, placeholder, c); ++ size_t consumed = format_commit_one(&local_sb, placeholder, c); + total_consumed += consumed; + + if (!modifier) +@@ -1551,7 +1553,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ + } + strbuf_addbuf(sb, &local_sb); + } else { +- int sb_len = sb->len, offset = 0; ++ size_t sb_len = sb->len, offset = 0; + if (c->flush_type == flush_left) + offset = padding - len; + else if (c->flush_type == flush_both) +@@ -1574,8 +1576,7 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ + const char *placeholder, + void *context) + { +- int consumed; +- size_t orig_len; ++ size_t consumed, orig_len; + enum { + NO_MAGIC, + ADD_LF_BEFORE_NON_EMPTY, +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index 204c149d5a..fff3e05615 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -867,4 +867,21 @@ test_expect_success 'log --pretty=reference is colored appropriately' ' + test_cmp expect actual + ' + ++test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' ' ++ # We only assert that this command does not crash. This needs to be ++ # executed with the address sanitizer to demonstrate failure. ++ git log -1 --pretty="format:%>(2147483646)%x41%41%>(2147483646)%x41" >/dev/null ++' ++ ++test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'set up huge commit' ' ++ test-tool genzeros 2147483649 | tr "\000" "1" >expect && ++ huge_commit=$(git commit-tree -F expect HEAD^{tree}) ++' ++ ++test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' ' ++ git log -1 --format="%B%<(1)%x30" $huge_commit >actual && ++ echo 0 >>expect && ++ test_cmp expect actual ++' ++ + test_done +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-pretty-restrict-input-lengths-for-padding-and-wrappi.patch b/backport-CVE-2022-41903-pretty-restrict-input-lengths-for-padding-and-wrappi.patch new file mode 100644 index 0000000000000000000000000000000000000000..c69cd013be878a3215bf743050fa4f9e3e5a53d1 --- /dev/null +++ b/backport-CVE-2022-41903-pretty-restrict-input-lengths-for-padding-and-wrappi.patch @@ -0,0 +1,121 @@ +From 304a50adff6480ede46b68f7545baab542cbfb46 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:47:23 +0100 +Subject: [PATCH] pretty: restrict input lengths for padding and wrapping + formats + +Both the padding and wrapping formatting directives allow the caller to +specify an integer that ultimately leads to us adding this many chars to +the result buffer. As a consequence, it is trivial to e.g. allocate 2GB +of RAM via a single formatting directive and cause resource exhaustion +on the machine executing this logic. Furthermore, it is debatable +whether there are any sane usecases that require the user to pad data to +2GB boundaries or to indent wrapped data by 2GB. + +Restrict the input sizes to 16 kilobytes at a maximum to limit the +amount of bytes that can be requested by the user. This is not meant +as a fix because there are ways to trivially amplify the amount of +data we generate via formatting directives; the real protection is +achieved by the changes in previous steps to catch and avoid integer +wraparound that causes us to under-allocate and access beyond the +end of allocated memory reagions. But having such a limit +significantly helps fuzzing the pretty format, because the fuzzer is +otherwise quite fast to run out-of-memory as it discovers these +formatters. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + pretty.c | 26 ++++++++++++++++++++++++++ + t/t4205-log-pretty-formats.sh | 24 +++++++++++++++--------- + 2 files changed, 41 insertions(+), 9 deletions(-) + +diff --git a/pretty.c b/pretty.c +index aae6e792bc..e2285572c4 100644 +--- a/pretty.c ++++ b/pretty.c +@@ -13,6 +13,13 @@ + #include "gpg-interface.h" + #include "trailer.h" + ++/* ++ * The limit for formatting directives, which enable the caller to append ++ * arbitrarily many bytes to the formatted buffer. This includes padding ++ * and wrapping formatters. ++ */ ++#define FORMATTING_LIMIT (16 * 1024) ++ + static char *user_format; + static struct cmt_fmt_map { + const char *name; +@@ -1046,6 +1053,15 @@ static size_t parse_padding_placeholder(const char *placeholder, + if (!*end || end == start) + return 0; + width = strtol(start, &next, 10); ++ ++ /* ++ * We need to limit the amount of padding, or otherwise this ++ * would allow the user to pad the buffer by arbitrarily many ++ * bytes and thus cause resource exhaustion. ++ */ ++ if (width < -FORMATTING_LIMIT || width > FORMATTING_LIMIT) ++ return 0; ++ + if (next == start || width == 0) + return 0; + if (width < 0) { +@@ -1205,6 +1221,16 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ + if (*next != ')') + return 0; + } ++ ++ /* ++ * We need to limit the format here as it allows the ++ * user to prepend arbitrarily many bytes to the buffer ++ * when rewrapping. ++ */ ++ if (width > FORMATTING_LIMIT || ++ indent1 > FORMATTING_LIMIT || ++ indent2 > FORMATTING_LIMIT) ++ return 0; + rewrap_message_tail(sb, c, width, indent1, indent2); + return end - placeholder + 1; + } else +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index 5c5b56596e..84c61dfc48 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -888,15 +888,21 @@ test_expect_success 'log --pretty with magical wrapping directives' ' + ' + + test_expect_success SIZE_T_IS_64BIT 'log --pretty with overflowing wrapping directive' ' +- cat >expect <<-EOF && +- fatal: number too large to represent as int on this platform: 2147483649 +- EOF +- test_must_fail git log -1 --pretty="format:%w(2147483649,1,1)%d" 2>error && +- test_cmp expect error && +- test_must_fail git log -1 --pretty="format:%w(1,2147483649,1)%d" 2>error && +- test_cmp expect error && +- test_must_fail git log -1 --pretty="format:%w(1,1,2147483649)%d" 2>error && +- test_cmp expect error ++ printf "%%w(2147483649,1,1)0" >expect && ++ git log -1 --pretty="format:%w(2147483649,1,1)%x30" >actual && ++ test_cmp expect actual && ++ printf "%%w(1,2147483649,1)0" >expect && ++ git log -1 --pretty="format:%w(1,2147483649,1)%x30" >actual && ++ test_cmp expect actual && ++ printf "%%w(1,1,2147483649)0" >expect && ++ git log -1 --pretty="format:%w(1,1,2147483649)%x30" >actual && ++ test_cmp expect actual ++' ++ ++test_expect_success SIZE_T_IS_64BIT 'log --pretty with overflowing padding directive' ' ++ printf "%%<(2147483649)0" >expect && ++ git log -1 --pretty="format:%<(2147483649)%x30" >actual && ++ test_cmp expect actual + ' + + test_expect_success 'log --pretty with padding and preceding control chars' ' +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-test-lib-add-prerequisite-for-64-bit-platforms.patch b/backport-CVE-2022-41903-test-lib-add-prerequisite-for-64-bit-platforms.patch new file mode 100644 index 0000000000000000000000000000000000000000..73bec9a93af5aff4eb1de0feb97ed4940abf854b --- /dev/null +++ b/backport-CVE-2022-41903-test-lib-add-prerequisite-for-64-bit-platforms.patch @@ -0,0 +1,38 @@ +From a244dc5b0a629290881641467c7a545de7508ab2 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= +Date: Tue, 2 Nov 2021 15:46:06 +0000 +Subject: [PATCH] test-lib: add prerequisite for 64-bit platforms +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Allow tests that assume a 64-bit `size_t` to be skipped in 32-bit +platforms and regardless of the size of `long`. + +This imitates the `LONG_IS_64BIT` prerequisite. + +Signed-off-by: Carlo Marcelo Arenas Belón +Signed-off-by: Johannes Schindelin +Signed-off-by: Junio C Hamano +--- + t/test-lib.sh | 4 ++++ + 1 file changed, 4 insertions(+) + +diff --git a/t/test-lib.sh b/t/test-lib.sh +index 9fa7c1d0f6..7d6e0f89d1 100644 +--- a/t/test-lib.sh ++++ b/t/test-lib.sh +@@ -1686,6 +1686,10 @@ build_option () { + sed -ne "s/^$1: //p" + } + ++test_lazy_prereq SIZE_T_IS_64BIT ' ++ test 8 -eq "$(build_option sizeof-size_t)" ++' ++ + test_lazy_prereq LONG_IS_64BIT ' + test 8 -le "$(build_option sizeof-long)" + ' +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-utf8-fix-checking-for-glyph-width-in-strbuf_utf8_rep.patch b/backport-CVE-2022-41903-utf8-fix-checking-for-glyph-width-in-strbuf_utf8_rep.patch new file mode 100644 index 0000000000000000000000000000000000000000..ca8ec44db14bda993bb268453a9cf64ba241dafe --- /dev/null +++ b/backport-CVE-2022-41903-utf8-fix-checking-for-glyph-width-in-strbuf_utf8_rep.patch @@ -0,0 +1,95 @@ +From 81c2d4c3a5ba0e6ab8c348708441fed170e63a82 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:47:10 +0100 +Subject: [PATCH] utf8: fix checking for glyph width in `strbuf_utf8_replace()` + +In `strbuf_utf8_replace()`, we call `utf8_width()` to compute the width +of the current glyph. If the glyph is a control character though it can +be that `utf8_width()` returns `-1`, but because we assign this value to +a `size_t` the conversion will cause us to underflow. This bug can +easily be triggered with the following command: + + $ git log --pretty='format:xxx%<|(1,trunc)%x10' + +>From all I can see though this seems to be a benign underflow that has +no security-related consequences. + +Fix the bug by using an `int` instead. When we see a control character, +we now copy it into the target buffer but don't advance the current +width of the string. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + t/t4205-log-pretty-formats.sh | 7 +++++++ + utf8.c | 19 ++++++++++++++----- + 2 files changed, 21 insertions(+), 5 deletions(-) + +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index aac9e4ce6c..5c5b56596e 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -905,6 +905,13 @@ test_expect_success 'log --pretty with padding and preceding control chars' ' + test_cmp expect actual + ' + ++test_expect_success 'log --pretty truncation with control chars' ' ++ test_commit "$(printf "\20\20\20\20xxxx")" file contents commit-with-control-chars && ++ printf "\20\20\20\20x.." >expect && ++ git log -1 --pretty="format:%<(3,trunc)%s" commit-with-control-chars >actual && ++ test_cmp expect actual ++' ++ + test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' ' + # We only assert that this command does not crash. This needs to be + # executed with the address sanitizer to demonstrate failure. +diff --git a/utf8.c b/utf8.c +index 30c7787cfa..077daf4b20 100644 +--- a/utf8.c ++++ b/utf8.c +@@ -377,6 +377,7 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width, + dst = sb_dst.buf; + + while (src < end) { ++ int glyph_width; + char *old; + size_t n; + +@@ -390,21 +391,29 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width, + break; + + old = src; +- n = utf8_width((const char**)&src, NULL); +- if (!src) /* broken utf-8, do nothing */ ++ glyph_width = utf8_width((const char**)&src, NULL); ++ if (!src) /* broken utf-8, do nothing */ + goto out; +- if (n && w >= pos && w < pos + width) { ++ ++ /* ++ * In case we see a control character we copy it into the ++ * buffer, but don't add it to the width. ++ */ ++ if (glyph_width < 0) ++ glyph_width = 0; ++ ++ if (glyph_width && w >= pos && w < pos + width) { + if (subst) { + memcpy(dst, subst, subst_len); + dst += subst_len; + subst = NULL; + } +- w += n; ++ w += glyph_width; + continue; + } + memcpy(dst, old, src - old); + dst += src - old; +- w += n; ++ w += glyph_width; + } + strbuf_setlen(&sb_dst, dst - sb_dst.buf); + strbuf_swap(sb_src, &sb_dst); +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-utf8-fix-overflow-when-returning-string-width.patch b/backport-CVE-2022-41903-utf8-fix-overflow-when-returning-string-width.patch new file mode 100644 index 0000000000000000000000000000000000000000..1f1d10b261f62b20d4ae074f47f75ccb069443ec --- /dev/null +++ b/backport-CVE-2022-41903-utf8-fix-overflow-when-returning-string-width.patch @@ -0,0 +1,158 @@ +From 937b71cc8b5b998963a7f9a33312ba3549d55510 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:47:04 +0100 +Subject: [PATCH] utf8: fix overflow when returning string width + +The return type of both `utf8_strwidth()` and `utf8_strnwidth()` is +`int`, but we operate on string lengths which are typically of type +`size_t`. This means that when the string is longer than `INT_MAX`, we +will overflow and thus return a negative result. + +This can lead to an out-of-bounds write with `--pretty=format:%<1)%B` +and a commit message that is 2^31+1 bytes long: + + ================================================================= + ==26009==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000001168 at pc 0x7f95c4e5f427 bp 0x7ffd8541c900 sp 0x7ffd8541c0a8 + WRITE of size 2147483649 at 0x603000001168 thread T0 + #0 0x7f95c4e5f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 + #1 0x5612bbb1068c in format_and_pad_commit pretty.c:1763 + #2 0x5612bbb1087a in format_commit_item pretty.c:1801 + #3 0x5612bbc33bab in strbuf_expand strbuf.c:429 + #4 0x5612bbb110e7 in repo_format_commit_message pretty.c:1869 + #5 0x5612bbb12d96 in pretty_print_commit pretty.c:2161 + #6 0x5612bba0a4d5 in show_log log-tree.c:781 + #7 0x5612bba0d6c7 in log_tree_commit log-tree.c:1117 + #8 0x5612bb691ed5 in cmd_log_walk_no_free builtin/log.c:508 + #9 0x5612bb69235b in cmd_log_walk builtin/log.c:549 + #10 0x5612bb6951a2 in cmd_log builtin/log.c:883 + #11 0x5612bb56c993 in run_builtin git.c:466 + #12 0x5612bb56d397 in handle_builtin git.c:721 + #13 0x5612bb56db07 in run_argv git.c:788 + #14 0x5612bb56e8a7 in cmd_main git.c:923 + #15 0x5612bb803682 in main common-main.c:57 + #16 0x7f95c4c3c28f (/usr/lib/libc.so.6+0x2328f) + #17 0x7f95c4c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) + #18 0x5612bb5680e4 in _start ../sysdeps/x86_64/start.S:115 + + 0x603000001168 is located 0 bytes to the right of 24-byte region [0x603000001150,0x603000001168) + allocated by thread T0 here: + #0 0x7f95c4ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85 + #1 0x5612bbcdd556 in xrealloc wrapper.c:136 + #2 0x5612bbc310a3 in strbuf_grow strbuf.c:99 + #3 0x5612bbc32acd in strbuf_add strbuf.c:298 + #4 0x5612bbc33aec in strbuf_expand strbuf.c:418 + #5 0x5612bbb110e7 in repo_format_commit_message pretty.c:1869 + #6 0x5612bbb12d96 in pretty_print_commit pretty.c:2161 + #7 0x5612bba0a4d5 in show_log log-tree.c:781 + #8 0x5612bba0d6c7 in log_tree_commit log-tree.c:1117 + #9 0x5612bb691ed5 in cmd_log_walk_no_free builtin/log.c:508 + #10 0x5612bb69235b in cmd_log_walk builtin/log.c:549 + #11 0x5612bb6951a2 in cmd_log builtin/log.c:883 + #12 0x5612bb56c993 in run_builtin git.c:466 + #13 0x5612bb56d397 in handle_builtin git.c:721 + #14 0x5612bb56db07 in run_argv git.c:788 + #15 0x5612bb56e8a7 in cmd_main git.c:923 + #16 0x5612bb803682 in main common-main.c:57 + #17 0x7f95c4c3c28f (/usr/lib/libc.so.6+0x2328f) + + SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy + Shadow bytes around the buggy address: + 0x0c067fff81d0: fd fd fd fa fa fa fd fd fd fa fa fa fd fd fd fa + 0x0c067fff81e0: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd + 0x0c067fff81f0: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa + 0x0c067fff8200: fd fd fd fa fa fa fd fd fd fd fa fa 00 00 00 fa + 0x0c067fff8210: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd + =>0x0c067fff8220: fd fa fa fa fd fd fd fa fa fa 00 00 00[fa]fa fa + 0x0c067fff8230: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c067fff8240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c067fff8250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c067fff8260: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + 0x0c067fff8270: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa + Shadow byte legend (one shadow byte represents 8 application bytes): + Addressable: 00 + Partially addressable: 01 02 03 04 05 06 07 + Heap left redzone: fa + Freed heap region: fd + Stack left redzone: f1 + Stack mid redzone: f2 + Stack right redzone: f3 + Stack after return: f5 + Stack use after scope: f8 + Global redzone: f9 + Global init order: f6 + Poisoned by user: f7 + Container overflow: fc + Array cookie: ac + Intra object redzone: bb + ASan internal: fe + Left alloca redzone: ca + Right alloca redzone: cb + ==26009==ABORTING + +Now the proper fix for this would be to convert both functions to return +an `size_t` instead of an `int`. But given that this commit may be part +of a security release, let's instead do the minimal viable fix and die +in case we see an overflow. + +Add a test that would have previously caused us to crash. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + t/t4205-log-pretty-formats.sh | 8 ++++++++ + utf8.c | 12 +++++++++--- + 2 files changed, 17 insertions(+), 3 deletions(-) + +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index e3905baa3c..aac9e4ce6c 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -922,4 +922,12 @@ test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit mes + test_cmp expect actual + ' + ++test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message does not cause allocation failure' ' ++ test_must_fail git log -1 --format="%<(1)%B" $huge_commit 2>error && ++ cat >expect <<-EOF && ++ fatal: number too large to represent as int on this platform: 2147483649 ++ EOF ++ test_cmp expect error ++' ++ + test_done +diff --git a/utf8.c b/utf8.c +index 6a21fd6a7b..30c7787cfa 100644 +--- a/utf8.c ++++ b/utf8.c +@@ -208,11 +208,12 @@ int utf8_width(const char **start, size_t *remainder_p) + */ + int utf8_strnwidth(const char *string, size_t len, int skip_ansi) + { +- int width = 0; + const char *orig = string; ++ size_t width = 0; + + while (string && string < orig + len) { +- int glyph_width, skip; ++ int glyph_width; ++ size_t skip; + + while (skip_ansi && + (skip = display_mode_esc_sequence_len(string)) != 0) +@@ -222,7 +223,12 @@ int utf8_strnwidth(const char *string, size_t len, int skip_ansi) + if (glyph_width > 0) + width += glyph_width; + } +- return string ? width : len; ++ ++ /* ++ * TODO: fix the interface of this function and `utf8_strwidth()` to ++ * return `size_t` instead of `int`. ++ */ ++ return cast_size_t_to_int(string ? width : len); + } + + int utf8_strwidth(const char *string) +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-utf8-fix-returning-negative-string-width.patch b/backport-CVE-2022-41903-utf8-fix-returning-negative-string-width.patch new file mode 100644 index 0000000000000000000000000000000000000000..d298077481c7247b8d6a7d9c09cb4360650aa6c7 --- /dev/null +++ b/backport-CVE-2022-41903-utf8-fix-returning-negative-string-width.patch @@ -0,0 +1,63 @@ +From 17d23e8a3812a5ca3dd6564e74d5250f22e5d76d Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:47:00 +0100 +Subject: [PATCH] utf8: fix returning negative string width + +The `utf8_strnwidth()` function calls `utf8_width()` in a loop and adds +its returned width to the end result. `utf8_width()` can return `-1` +though in case it reads a control character, which means that the +computed string width is going to be wrong. In the worst case where +there are more control characters than non-control characters, we may +even return a negative string width. + +Fix this bug by treating control characters as having zero width. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + t/t4205-log-pretty-formats.sh | 6 ++++++ + utf8.c | 8 ++++++-- + 2 files changed, 12 insertions(+), 2 deletions(-) + +diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh +index c88b64d08b..e3905baa3c 100755 +--- a/t/t4205-log-pretty-formats.sh ++++ b/t/t4205-log-pretty-formats.sh +@@ -899,6 +899,12 @@ test_expect_success SIZE_T_IS_64BIT 'log --pretty with overflowing wrapping dire + test_cmp expect error + ' + ++test_expect_success 'log --pretty with padding and preceding control chars' ' ++ printf "\20\20 0" >expect && ++ git log -1 --pretty="format:%x10%x10%>|(4)%x30" >actual && ++ test_cmp expect actual ++' ++ + test_expect_success EXPENSIVE,SIZE_T_IS_64BIT 'log --pretty with huge commit message' ' + # We only assert that this command does not crash. This needs to be + # executed with the address sanitizer to demonstrate failure. +diff --git a/utf8.c b/utf8.c +index 504e517c34..6a21fd6a7b 100644 +--- a/utf8.c ++++ b/utf8.c +@@ -212,11 +212,15 @@ int utf8_strnwidth(const char *string, size_t len, int skip_ansi) + const char *orig = string; + + while (string && string < orig + len) { +- int skip; ++ int glyph_width, skip; ++ + while (skip_ansi && + (skip = display_mode_esc_sequence_len(string)) != 0) + string += skip; +- width += utf8_width(&string, NULL); ++ ++ glyph_width = utf8_width(&string, NULL); ++ if (glyph_width > 0) ++ width += glyph_width; + } + return string ? width : len; + } +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-utf8-fix-truncated-string-lengths-in-utf8_strnwidth.patch b/backport-CVE-2022-41903-utf8-fix-truncated-string-lengths-in-utf8_strnwidth.patch new file mode 100644 index 0000000000000000000000000000000000000000..fcd6078433548e10e6f8e2df91cdb7fa0ddb10c9 --- /dev/null +++ b/backport-CVE-2022-41903-utf8-fix-truncated-string-lengths-in-utf8_strnwidth.patch @@ -0,0 +1,119 @@ +From 522cc87fdc25449222a5894a428eebf4b8d5eaa9 Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:46:53 +0100 +Subject: [PATCH] utf8: fix truncated string lengths in `utf8_strnwidth()` + +The `utf8_strnwidth()` function accepts an optional string length as +input parameter. This parameter can either be set to `-1`, in which case +we call `strlen()` on the input. Or it can be set to a positive integer +that indicates a precomputed length, which callers typically compute by +calling `strlen()` at some point themselves. + +The input parameter is an `int` though, whereas `strlen()` returns a +`size_t`. This can lead to implementation-defined behaviour though when +the `size_t` cannot be represented by the `int`. In the general case +though this leads to wrap-around and thus to negative string sizes, +which is sure enough to not lead to well-defined behaviour. + +Fix this by accepting a `size_t` instead of an `int` as string length. +While this takes away the ability of callers to simply pass in `-1` as +string length, it really is trivial enough to convert them to instead +pass in `strlen()` instead. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + column.c | 2 +- + pretty.c | 4 ++-- + utf8.c | 8 +++----- + utf8.h | 2 +- + 4 files changed, 7 insertions(+), 9 deletions(-) + +diff --git a/column.c b/column.c +index 1261e18a72..fbf88639aa 100644 +--- a/column.c ++++ b/column.c +@@ -23,7 +23,7 @@ struct column_data { + /* return length of 's' in letters, ANSI escapes stripped */ + static int item_length(const char *s) + { +- return utf8_strnwidth(s, -1, 1); ++ return utf8_strnwidth(s, strlen(s), 1); + } + + /* +diff --git a/pretty.c b/pretty.c +index 7e649b1cec..aae6e792bc 100644 +--- a/pretty.c ++++ b/pretty.c +@@ -1483,7 +1483,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ + int occupied; + if (!start) + start = sb->buf; +- occupied = utf8_strnwidth(start, -1, 1); ++ occupied = utf8_strnwidth(start, strlen(start), 1); + occupied += c->pretty_ctx->graph_width; + padding = (-padding) - occupied; + } +@@ -1501,7 +1501,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */ + placeholder++; + total_consumed++; + } +- len = utf8_strnwidth(local_sb.buf, -1, 1); ++ len = utf8_strnwidth(local_sb.buf, local_sb.len, 1); + + if (c->flush_type == flush_left_and_steal) { + const char *ch = sb->buf + sb->len - 1; +diff --git a/utf8.c b/utf8.c +index 5b39361ada..504e517c34 100644 +--- a/utf8.c ++++ b/utf8.c +@@ -206,13 +206,11 @@ int utf8_width(const char **start, size_t *remainder_p) + * string, assuming that the string is utf8. Returns strlen() instead + * if the string does not look like a valid utf8 string. + */ +-int utf8_strnwidth(const char *string, int len, int skip_ansi) ++int utf8_strnwidth(const char *string, size_t len, int skip_ansi) + { + int width = 0; + const char *orig = string; + +- if (len == -1) +- len = strlen(string); + while (string && string < orig + len) { + int skip; + while (skip_ansi && +@@ -225,7 +223,7 @@ int utf8_strnwidth(const char *string, int len, int skip_ansi) + + int utf8_strwidth(const char *string) + { +- return utf8_strnwidth(string, -1, 0); ++ return utf8_strnwidth(string, strlen(string), 0); + } + + int is_utf8(const char *text) +@@ -791,7 +789,7 @@ int skip_utf8_bom(char **text, size_t len) + void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width, + const char *s) + { +- int slen = strlen(s); ++ size_t slen = strlen(s); + int display_len = utf8_strnwidth(s, slen, 0); + int utf8_compensation = slen - display_len; + +diff --git a/utf8.h b/utf8.h +index fcd5167baf..6da1b6d05e 100644 +--- a/utf8.h ++++ b/utf8.h +@@ -7,7 +7,7 @@ typedef unsigned int ucs_char_t; /* assuming 32bit int */ + + size_t display_mode_esc_sequence_len(const char *s); + int utf8_width(const char **start, size_t *remainder_p); +-int utf8_strnwidth(const char *string, int len, int skip_ansi); ++int utf8_strnwidth(const char *string, size_t len, int skip_ansi); + int utf8_strwidth(const char *string); + int is_utf8(const char *text); + int is_encoding_utf8(const char *name); +-- +2.27.0 + diff --git a/backport-CVE-2022-41903-utf8-refactor-strbuf_utf8_replace-to-not-rely-on-pre.patch b/backport-CVE-2022-41903-utf8-refactor-strbuf_utf8_replace-to-not-rely-on-pre.patch new file mode 100644 index 0000000000000000000000000000000000000000..1b1639a662fe966e5fce911d4632289da7ed981b --- /dev/null +++ b/backport-CVE-2022-41903-utf8-refactor-strbuf_utf8_replace-to-not-rely-on-pre.patch @@ -0,0 +1,87 @@ +From f930a2394303b902e2973f4308f96529f736b8bc Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Thu, 1 Dec 2022 15:47:15 +0100 +Subject: [PATCH] utf8: refactor `strbuf_utf8_replace` to not rely on + preallocated buffer + +In `strbuf_utf8_replace`, we preallocate the destination buffer and then +use `memcpy` to copy bytes into it at computed offsets. This feels +rather fragile and is hard to understand at times. Refactor the code to +instead use `strbuf_add` and `strbuf_addstr` so that we can be sure that +there is no possibility to perform an out-of-bounds write. + +Signed-off-by: Patrick Steinhardt +Signed-off-by: Junio C Hamano +--- + utf8.c | 34 +++++++++++++--------------------- + 1 file changed, 13 insertions(+), 21 deletions(-) + +diff --git a/utf8.c b/utf8.c +index 077daf4b20..d8a16af87c 100644 +--- a/utf8.c ++++ b/utf8.c +@@ -365,26 +365,20 @@ void strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len, + void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width, + const char *subst) + { +- struct strbuf sb_dst = STRBUF_INIT; +- char *src = sb_src->buf; +- char *end = src + sb_src->len; +- char *dst; +- int w = 0, subst_len = 0; ++ const char *src = sb_src->buf, *end = sb_src->buf + sb_src->len; ++ struct strbuf dst; ++ int w = 0; + +- if (subst) +- subst_len = strlen(subst); +- strbuf_grow(&sb_dst, sb_src->len + subst_len); +- dst = sb_dst.buf; ++ strbuf_init(&dst, sb_src->len); + + while (src < end) { ++ const char *old; + int glyph_width; +- char *old; + size_t n; + + while ((n = display_mode_esc_sequence_len(src))) { +- memcpy(dst, src, n); ++ strbuf_add(&dst, src, n); + src += n; +- dst += n; + } + + if (src >= end) +@@ -404,21 +398,19 @@ void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width, + + if (glyph_width && w >= pos && w < pos + width) { + if (subst) { +- memcpy(dst, subst, subst_len); +- dst += subst_len; ++ strbuf_addstr(&dst, subst); + subst = NULL; + } +- w += glyph_width; +- continue; ++ } else { ++ strbuf_add(&dst, old, src - old); + } +- memcpy(dst, old, src - old); +- dst += src - old; ++ + w += glyph_width; + } +- strbuf_setlen(&sb_dst, dst - sb_dst.buf); +- strbuf_swap(sb_src, &sb_dst); ++ ++ strbuf_swap(sb_src, &dst); + out: +- strbuf_release(&sb_dst); ++ strbuf_release(&dst); + } + + /* +-- +2.27.0 + diff --git a/git.spec b/git.spec index 5b0d69913aca3ca35e5e44241454d6d451771534..afaa25124d032f0d66369ab20e56f661bd87cc56 100644 --- a/git.spec +++ b/git.spec @@ -1,7 +1,7 @@ %global gitexecdir %{_libexecdir}/git-core Name: git Version: 2.27.0 -Release: 9 +Release: 10 Summary: A popular and widely used Version Control System License: GPLv2+ or LGPLv2.1 URL: https://git-scm.com/ @@ -28,6 +28,28 @@ Patch13: backport-CVE-2022-39253-builtin-clone.c-disallow-local-clones-with-s Patch14: backport-CVE-2022-39260-shell-add-basic-tests.patch Patch15: backport-CVE-2022-39260-shell-limit-size-of-interactive-commands.patch Patch16: backport-CVE-2022-39260-alias.c-reject-too-long-cmdline-strings-in-split_cmd.patch +Patch17: backport-CVE-2022-23521-attr-fix-overflow-when-upserting-attribute-with-over.patch +Patch18: backport-CVE-2022-23521-attr-fix-out-of-bounds-read-with-huge-attribute-name.patch +Patch19: backport-CVE-2022-23521-attr-fix-integer-overflow-when-parsing-huge-attribut.patch +Patch20: backport-CVE-2022-23521-attr-fix-out-of-bounds-write-when-parsing-huge-numbe.patch +Patch21: backport-CVE-2022-23521-attr-fix-out-of-bounds-read-with-unreasonable-amount.patch +Patch22: backport-CVE-2022-23521-attr-fix-integer-overflow-with-more-than-INT_MAX-mac.patch +Patch23: backport-CVE-2022-23521-attr-harden-allocation-against-integer-overflows.patch +Patch24: backport-CVE-2022-23521-attr-fix-silently-splitting-up-lines-longer-than-204.patch +Patch25: backport-CVE-2022-23521-attr-ignore-attribute-lines-exceeding-2048-bytes.patch +Patch26: backport-CVE-2022-23521-attr-ignore-overly-large-gitattributes-files.patch +Patch27: backport-CVE-2022-41903-test-lib-add-prerequisite-for-64-bit-platforms.patch +Patch28: backport-CVE-2022-41903-pretty-fix-out-of-bounds-write-caused-by-integer-ove.patch +Patch29: backport-CVE-2022-41903-pretty-fix-out-of-bounds-read-when-left-flushing-wit.patch +Patch30: backport-CVE-2022-41903-pretty-fix-out-of-bounds-read-when-parsing-invalid-p.patch +Patch31: backport-CVE-2022-41903-pretty-fix-adding-linefeed-when-placeholder-is-not-e.patch +Patch32: backport-CVE-2022-41903-pretty-fix-integer-overflow-in-wrapping-format.patch +Patch33: backport-CVE-2022-41903-utf8-fix-truncated-string-lengths-in-utf8_strnwidth.patch +Patch34: backport-CVE-2022-41903-utf8-fix-returning-negative-string-width.patch +Patch35: backport-CVE-2022-41903-utf8-fix-overflow-when-returning-string-width.patch +Patch36: backport-CVE-2022-41903-utf8-fix-checking-for-glyph-width-in-strbuf_utf8_rep.patch +Patch37: backport-CVE-2022-41903-utf8-refactor-strbuf_utf8_replace-to-not-rely-on-pre.patch +Patch38: backport-CVE-2022-41903-pretty-restrict-input-lengths-for-padding-and-wrappi.patch BuildRequires: gcc gettext BuildRequires: openssl-devel libcurl-devel expat-devel systemd asciidoc xmlto glib2-devel libsecret-devel pcre-devel desktop-file-utils @@ -277,6 +299,12 @@ make %{?_smp_mflags} test %{_mandir}/man7/git*.7.* %changelog +* Thu Jan 19 2023 fuanan - 2.27.0-10 +- Type:CVE +- ID:CVE-2022-23521 CVE-2022-41903 +- SUG:NA +- DESC:Fix CVE-2022-23521 CVE-2022-41903 + * Fri Oct 21 2022 fuanan - 2.27.0-9 - Type:CVE - ID:CVE-2022-39253 CVE-2022-39260