git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: Derrick Stolee <derrickstolee@github.com>,
	Jeff King <peff@peff.net>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>,
	Victoria Dye <vdye@github.com>
Subject: [PATCH 01/20] packfile.c: prevent overflow in `nth_packed_object_id()`
Date: Wed, 12 Jul 2023 19:37:27 -0400	[thread overview]
Message-ID: <5e92582e2912806e0068af97c265fb50e8bbe54f.1689205042.git.me@ttaylorr.com> (raw)
In-Reply-To: <cover.1689205042.git.me@ttaylorr.com>

In 37fec86a83 (packfile: abstract away hash constant values,
2018-05-02), `nth_packed_object_id()` started using the variable
`the_hash_algo->rawsz` instead of a fixed constant when trying to
compute an offset into the ".idx" file for some object position.

This can lead to surprising truncation when looking for an object
towards the end of a large enough pack, like the following:

    (gdb) p hashsz
    $1 = 20
    (gdb) p n
    $2 = 215043814
    (gdb) p hashsz * n
    $3 = 5908984

, which is a debugger session broken on a known-bad call to the
`nth_packed_object_id()` function.

This behavior predates 37fec86a83, and is original to the v2 index
format, via: 74e34e1fca (sha1_file.c: learn about index version 2,
2007-04-09).

This is due to §6.4.4.1 of the C99 standard, which states that an
untyped integer constant will take the first type in which the value can
be accurately represented, among `int`, `long int`, and `long long int`.

Since 20 can be represented as an `int`, and `n` is a 32-bit unsigned
integer, the resulting computation is defined by §6.3.1.8, and the
(signed) integer value representing `n` is converted to an unsigned
type, meaning that `20 * n` (for `n` having type `uint32_t`) is
equivalent to a multiplication between two unsigned 32-bit integers.

When multiplying a sufficiently large `n`, the resulting value can
exceed 2^32-1, wrapping around and producing an invalid result. Let's
follow the example in f86f769550e (compute pack .idx byte offsets using
size_t, 2020-11-13) and replace this computation with `st_mult()`, which
will ensure that the computation is done using 64-bits.

While here, guard the corresponding computation for packs with v1
indexes, too. Though the likelihood of seeing a bug there is much
smaller, since (a) v1 indexes are generated far less frequently than v2
indexes, and (b) they all correspond to packs no larger than 2 GiB, so
having enough objects to trigger this overflow is unlikely if not
impossible.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 packfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/packfile.c b/packfile.c
index c2e753ef8f..89220f0e03 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1920,10 +1920,10 @@ int nth_packed_object_id(struct object_id *oid,
 		return -1;
 	index += 4 * 256;
 	if (p->index_version == 1) {
-		oidread(oid, index + (hashsz + 4) * n + 4);
+		oidread(oid, index + st_add(st_mult(hashsz + 4, n), 4));
 	} else {
 		index += 8;
-		oidread(oid, index + hashsz * n);
+		oidread(oid, index + st_mult(hashsz, n));
 	}
 	return 0;
 }
-- 
2.41.0.347.g7b976b8871f


  reply	other threads:[~2023-07-12 23:37 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-12 23:37 [PATCH 00/20] guard object lookups against 32-bit overflow Taylor Blau
2023-07-12 23:37 ` Taylor Blau [this message]
2023-07-12 23:37 ` [PATCH 02/20] packfile.c: prevent overflow in `load_idx()` Taylor Blau
2023-07-13  8:21   ` Phillip Wood
2023-07-13 14:24     ` Taylor Blau
2023-07-14  0:54       ` Taylor Blau
2023-07-14  9:56         ` Phillip Wood
2023-07-14 16:29         ` Junio C Hamano
2023-07-14  9:55       ` Phillip Wood
2023-07-13 16:14     ` Junio C Hamano
2023-07-12 23:37 ` [PATCH 03/20] packfile.c: use checked arithmetic in `nth_packed_object_offset()` Taylor Blau
2023-07-12 23:37 ` [PATCH 04/20] midx.c: use `size_t`'s for fanout nr and alloc Taylor Blau
2023-07-12 23:37 ` [PATCH 05/20] midx.c: prevent overflow in `nth_midxed_object_oid()` Taylor Blau
2023-07-12 23:37 ` [PATCH 06/20] midx.c: prevent overflow in `nth_midxed_offset()` Taylor Blau
2023-07-12 23:37 ` [PATCH 07/20] midx.c: store `nr`, `alloc` variables as `size_t`'s Taylor Blau
2023-07-12 23:37 ` [PATCH 08/20] midx.c: prevent overflow in `write_midx_internal()` Taylor Blau
2023-07-12 23:37 ` [PATCH 09/20] midx.c: prevent overflow in `fill_included_packs_batch()` Taylor Blau
2023-07-12 23:37 ` [PATCH 10/20] pack-bitmap.c: ensure that eindex lookups don't overflow Taylor Blau
2023-07-12 23:37 ` [PATCH 11/20] commit-graph.c: prevent overflow in `write_commit_graph_file()` Taylor Blau
2023-07-12 23:37 ` [PATCH 12/20] commit-graph.c: prevent overflow in add_graph_to_chain() Taylor Blau
2023-07-12 23:38 ` [PATCH 13/20] commit-graph.c: prevent overflow in `load_oid_from_graph()` Taylor Blau
2023-07-12 23:38 ` [PATCH 14/20] commit-graph.c: prevent overflow in `fill_commit_graph_info()` Taylor Blau
2023-07-12 23:38 ` [PATCH 15/20] commit-graph.c: prevent overflow in `fill_commit_in_graph()` Taylor Blau
2023-07-12 23:38 ` [PATCH 16/20] commit-graph.c: prevent overflow in `load_tree_for_commit()` Taylor Blau
2023-07-12 23:38 ` [PATCH 17/20] commit-graph.c: prevent overflow in `split_graph_merge_strategy()` Taylor Blau
2023-07-12 23:38 ` [PATCH 18/20] commit-graph.c: prevent overflow in `merge_commit_graph()` Taylor Blau
2023-07-12 23:38 ` [PATCH 19/20] commit-graph.c: prevent overflow in `write_commit_graph()` Taylor Blau
2023-07-12 23:38 ` [PATCH 20/20] commit-graph.c: prevent overflow in `verify_commit_graph()` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5e92582e2912806e0068af97c265fb50e8bbe54f.1689205042.git.me@ttaylorr.com \
    --to=me@ttaylorr.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).