Skip to content

Commit 33e6c40

Browse files
committed
patch_parse: fix out-of-bounds reads caused by integer underflow
The patch format for binary files is a simple Base85 encoding with a length byte as prefix that encodes the current line's length. For each line, we thus check whether the line's actual length matches its expected length in order to not faultily apply a truncated patch. This also acts as a check to verify that we're not reading outside of the line's string: if (encoded_len > ctx->parse_ctx.line_len - 1) { error = git_parse_err(...); goto done; } There is the possibility for an integer underflow, though. Given a line with a single prefix byte, only, `line_len` will be zero when reaching this check. As a result, subtracting one from that will result in an integer underflow, causing us to assume that there's a wealth of bytes available later on. Naturally, this may result in an out-of-bounds read. Fix the issue by checking both `encoded_len` and `line_len` for a non-zero value. The binary format doesn't make use of zero-length lines anyway, so we need to know that there are both encoded bytes and remaining characters available at all. This patch also adds a test that works based on the last error message. Checking error messages is usually too tightly coupled, but in fact parsing the patch failed even before the change. Thus the only possibility is to use e.g. Valgrind, but that'd result in us not catching issues when run without Valgrind. As a result, using the error message is considered a viable tradeoff as we know that we didn't start decoding Base85 in the first place.
1 parent fb439c9 commit 33e6c40

File tree

3 files changed

+16
-1
lines changed

3 files changed

+16
-1
lines changed

src/patch_parse.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -808,7 +808,7 @@ static int parse_patch_binary_side(
808808

809809
encoded_len = ((decoded_len / 4) + !!(decoded_len % 4)) * 5;
810810

811-
if (encoded_len > ctx->parse_ctx.line_len - 1) {
811+
if (!encoded_len || !ctx->parse_ctx.line_len || encoded_len > ctx->parse_ctx.line_len - 1) {
812812
error = git_parse_err("truncated binary data at line %"PRIuZ, ctx->parse_ctx.line_num);
813813
goto done;
814814
}

tests/patch/parse.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,14 @@ void test_patch_parse__binary_file_path_without_body_paths(void)
184184
strlen(PATCH_BINARY_FILE_PATH_WITHOUT_BODY_PATHS), NULL));
185185
}
186186

187+
void test_patch_parse__binary_file_with_truncated_delta(void)
188+
{
189+
git_patch *patch;
190+
cl_git_fail(git_patch_from_buffer(&patch, PATCH_BINARY_FILE_WITH_TRUNCATED_DELTA,
191+
strlen(PATCH_BINARY_FILE_WITH_TRUNCATED_DELTA), NULL));
192+
cl_assert_equal_s(git_error_last()->message, "truncated binary data at line 5");
193+
}
194+
187195
void test_patch_parse__memory_leak_on_multiple_paths(void)
188196
{
189197
git_patch *patch;

tests/patch/patch_common.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -974,6 +974,13 @@
974974
"+++ \n" \
975975
"Binary files a b c and d e f differ"
976976

977+
#define PATCH_BINARY_FILE_WITH_TRUNCATED_DELTA \
978+
"diff --git a/file b/file\n" \
979+
"index 1420..b71f\n" \
980+
"GIT binary patch\n" \
981+
"delta 7\n" \
982+
"d"
983+
977984
#define PATCH_MULTIPLE_OLD_PATHS \
978985
"diff --git \n" \
979986
"--- \n" \

0 commit comments

Comments
 (0)