95c5c804ce
Related: RHEL-9503 - commit_list: fix possible buffer overflow in `commit_quick_parse` Resolves: RHEL-9503
75 lines
3.1 KiB
Diff
75 lines
3.1 KiB
Diff
From c31dcbfd93d85a008e95b23c129c7e8887f1316e Mon Sep 17 00:00:00 2001
|
|
From: Patrick Steinhardt <ps@pks.im>
|
|
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
|
|
|