From 95c5c804ce2474462fb487697631966d8e45e263 Mon Sep 17 00:00:00 2001 From: "Brian C. Lane" Date: Wed, 7 Feb 2024 17:03:47 -0800 Subject: [PATCH] - revparse: Remove error-prone, redundant test Related: RHEL-9503 - commit_list: fix possible buffer overflow in `commit_quick_parse` Resolves: RHEL-9503 --- ...possible-buffer-overflow-in-commit_q.patch | 74 +++++++++++++++++++ ...se-Remove-error-prone-redundant-test.patch | 34 +++++++++ libgit2.spec | 10 ++- 3 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 0006-commit_list-fix-possible-buffer-overflow-in-commit_q.patch create mode 100644 0007-revparse-Remove-error-prone-redundant-test.patch diff --git a/0006-commit_list-fix-possible-buffer-overflow-in-commit_q.patch b/0006-commit_list-fix-possible-buffer-overflow-in-commit_q.patch new file mode 100644 index 0000000..c3bd70c --- /dev/null +++ b/0006-commit_list-fix-possible-buffer-overflow-in-commit_q.patch @@ -0,0 +1,74 @@ +From c31dcbfd93d85a008e95b23c129c7e8887f1316e Mon Sep 17 00:00:00 2001 +From: Patrick Steinhardt +Date: Fri, 21 Jun 2019 15:53:54 +0200 +Subject: [PATCH 6/9] commit_list: fix possible buffer overflow in + `commit_quick_parse` + +The function `commit_quick_parse` provides a way to quickly parse +parts of a commit without storing or verifying most of its +metadata. The first thing it does is calculating the number of +parents by skipping "parent " lines until it finds the first +non-parent line. Afterwards, this parent count is passed to +`alloc_parents`, which will allocate an array to store all the +parent. + +To calculate the amount of storage required for the parents +array, `alloc_parents` simply multiplicates the number of parents +with the respective elements's size. This already screams "buffer +overflow", and in fact this problem is getting worse by the +result being cast to an `uint32_t`. + +In fact, triggering this is possible: git-hash-object(1) will +happily write a commit with multiple millions of parents for you. +I've stopped at 67,108,864 parents as git-hash-object(1) +unfortunately soaks up the complete object without streaming +anything to disk and thus will cause an OOM situation at a later +point. The point here is: this commit was about 4.1GB of size but +compressed down to 24MB and thus easy to distribute. + +The above doesn't yet trigger the buffer overflow, thus. As the +array's elements are all pointers which are 8 bytes on 64 bit, we +need a total of 536,870,912 parents to trigger the overflow to +`0`. The effect is that we're now underallocating the array +and do an out-of-bound writes. As the buffer is kindly provided +by the adversary, this may easily result in code execution. + +Extrapolating from the test file with 67m commits to the one with +536m commits results in a factor of 8. Thus the uncompressed +contents would be about 32GB in size and the compressed ones +192MB. While still easily distributable via the network, only +servers will have that amount of RAM and not cause an +out-of-memory condition previous to triggering the overflow. This +at least makes this attack not an easy vector for client-side use +of libgit2. + +(cherry picked from commit 3316f666566f768eb8aa8de521a5262524dc3424) +--- + src/commit_list.c | 8 ++++++-- + 1 file changed, 6 insertions(+), 2 deletions(-) + +diff --git a/src/commit_list.c b/src/commit_list.c +index 7df79bfd6..14d1c9813 100644 +--- a/src/commit_list.c ++++ b/src/commit_list.c +@@ -69,11 +69,15 @@ static int commit_error(git_commit_list_node *commit, const char *msg) + static git_commit_list_node **alloc_parents( + git_revwalk *walk, git_commit_list_node *commit, size_t n_parents) + { ++ size_t bytes; ++ + if (n_parents <= PARENTS_PER_COMMIT) + return (git_commit_list_node **)((char *)commit + sizeof(git_commit_list_node)); + +- return (git_commit_list_node **)git_pool_malloc( +- &walk->commit_pool, (uint32_t)(n_parents * sizeof(git_commit_list_node *))); ++ if (git__multiply_sizet_overflow(&bytes, n_parents, sizeof(git_commit_list_node *))) ++ return NULL; ++ ++ return (git_commit_list_node **)git_pool_malloc(&walk->commit_pool, bytes); + } + + +-- +2.43.0 + diff --git a/0007-revparse-Remove-error-prone-redundant-test.patch b/0007-revparse-Remove-error-prone-redundant-test.patch new file mode 100644 index 0000000..8104e78 --- /dev/null +++ b/0007-revparse-Remove-error-prone-redundant-test.patch @@ -0,0 +1,34 @@ +From 9a3568814bd1d7a3e6522e0f61168d63c29efe97 Mon Sep 17 00:00:00 2001 +From: Carl Dong +Date: Mon, 9 May 2022 12:09:08 -0400 +Subject: [PATCH] revparse: Remove error-prone, redundant test + +Originally introduced in: 776a6a8e5f8e258d31aded73c0ce38df6ac7bdc4 + +This test case has recently been fixed in bdab22384cc61d315005a65456a9f9563bb27c8f, but that fix will only last for a year. Next year the same problem will crop up and the test will need to be re-edited. + +This is not ideal as +- This test case becomes an unnecessary burden for developers +- Downstream distros or even just users who want to build older versions of libgit2 are guaranteed to have this test fail + +Furthermore, this test case is entirely unnecessary, as the functionality that was originally (see 776a6a8e5f8e258d31aded73c0ce38df6ac7bdc4) intended to be tested is well-covered by subsequent tests which specify a date instead of a "x ago" specification. +--- + tests/refs/revparse.c | 2 -- + 1 file changed, 2 deletions(-) + +diff --git a/tests/refs/revparse.c b/tests/refs/revparse.c +index 459188cf7..2bb19ff69 100644 +--- a/tests/refs/revparse.c ++++ b/tests/refs/revparse.c +@@ -400,8 +400,6 @@ void test_refs_revparse__date(void) + * a65fedf HEAD@{1335806603 -0900}: commit: + * be3563a HEAD@{1335806563 -0700}: clone: from /Users/ben/src/libgit2/tests/resour + */ +- test_object("HEAD@{10 years ago}", NULL); +- + test_object("HEAD@{1 second}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750"); + test_object("HEAD@{1 second ago}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750"); + test_object("HEAD@{2 days ago}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750"); +-- +2.43.0 + diff --git a/libgit2.spec b/libgit2.spec index c839d76..8bd9f2c 100644 --- a/libgit2.spec +++ b/libgit2.spec @@ -1,6 +1,6 @@ Name: libgit2 Version: 0.26.8 -Release: 2%{?dist} +Release: 3%{?dist} Summary: C implementation of the Git core methods as a library with a solid API License: GPLv2 with exceptions URL: http://libgit2.github.com/ @@ -12,6 +12,8 @@ Patch0002: 0002-CMakeLists-increase-strict-aliasing-level-to-3.patch Patch0003: 0003-Disable-online-tests.patch Patch0004: 0004-tests-Increase-TOOBIG-value-used-on-i686.patch Patch0005: 0005-openssl-Use-the-system-profile-ciphers.patch +Patch0006: 0006-commit_list-fix-possible-buffer-overflow-in-commit_q.patch +Patch0007: 0007-revparse-Remove-error-prone-redundant-test.patch BuildRequires: gcc BuildRequires: cmake @@ -80,6 +82,12 @@ popd %{_includedir}/git2/ %changelog +* Wed Feb 07 2024 Brian C. Lane - 0.26.8-3 +- revparse: Remove error-prone, redundant test + Related: RHEL-9503 +- commit_list: fix possible buffer overflow in `commit_quick_parse` + Resolves: RHEL-9503 + * Tue Jun 02 2020 Brian C. Lane - 0.26.8-2 - openssl: Use the system profile ciphers Resolves: rhbz#1842814