From 1de69c0cdd388b0a5b7bdde0bfa0bda514a354b0 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 1 Dec 2022 15:46:39 +0100 Subject: [PATCH 05/12] 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 Upstream-Status: Backport [https://github.com/git/git/commit/1de69c0cdd388b0a5b7bdde0bfa0bda514a354b0] CVE: CVE-2022-41903 Signed-off-by: Vijay Anusuri --- 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 c49e818..195d005 100644 --- a/pretty.c +++ b/pretty.c @@ -1551,9 +1551,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 8a349df..fa1bc2b 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -800,6 +800,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.25.1