{"thread":{"id":"62772","subject":"[PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","startedAt":"2025-01-09T06:21:35Z","lastAt":"2025-01-10T14:21:59Z","messageCount":18,"participants":["Patrick Steinhardt","Kristoffer Haugsbakk","Johannes Schindelin","shejialuo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"510214","messageId":"20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im","threadId":"62772","inReplyTo":null,"subject":"[PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-09T06:21:30Z","receivedAt":"2025-01-09T06:21:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 6411a0a896 (builtin/blame: fix type of `length` variable when\nemitting object ID, 2024-12-06) we have fixed the type of the `length`\nvariable. In order to avoid a cast from `size_t` to `int` in the call to\nprintf(3p) with the \"%.*s\" formatter we have converted the code to\ninstead use fwrite(3p), which accepts the length as a `size_t`.\n\nIt was reported though that this makes us read over the end of the OID\narray when the provided `--abbrev=` length exceeds the length of the\nobject ID. This is because fwrite(3p) of course doesn't stop when it\nsees a NUL byte, where as printf(3p) does.\n\nFix the bug by reverting back to printf(3p) and culling the provided\nlength to `GIT_MAX_HEXSZ` to keep it from overflowing when cast to an\n`int`.\n\nReported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nThis fixes the issue reported in [1]. Thanks!\n\nPatrick\n\n[1]: <4d812802-afbc-4635-7a19-73896fcda625@gmx.de>\n---\n builtin/blame.c  | 4 +++-\n t/t8002-blame.sh | 4 ++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 867032e4c16878ffd56df8a73162b89ca4bd2694..ad91fe9e97f90625dd2708fbd44bf2dd24a337a6 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -475,6 +475,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\tchar ch;\n \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n+\t\tif (length > GIT_MAX_HEXSZ)\n+\t\t\tlength = GIT_MAX_HEXSZ;\n \n \t\tif (opt & OUTPUT_COLOR_LINE) {\n \t\t\tif (cnt > 0) {\n@@ -505,7 +507,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\t\tlength--;\n \t\t\tputchar('?');\n \t\t}\n-\t\tfwrite(hex, 1, length, stdout);\n+\t\tprintf(\"%.*s\", (int)length, hex);\n \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n \t\t\tconst char *name;\n \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..fcaba8c11f7ede084e069eefd292f337e8396cb4 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n \tcheck_abbrev $hexsz --no-abbrev\n '\n \n+test_expect_success 'blame --abbrev gets truncated' '\n+\tcheck_abbrev 9000 --abbrev=$hexsz HEAD\n+'\n+\n test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git blame --exclude-promisor-objects one\n '\n\n---\nbase-commit: 14650065b76b28d3cfa9453356ac5669b19e706e\nchange-id: 20250109-b4-pks-blame-truncate-hash-length-c875cac66d71\n\n"},{"id":"510241","messageId":"10051d40-2a32-4434-a034-87b6c678a3cc@app.fastmail.com","threadId":"62772","inReplyTo":"20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im","subject":"Re: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-01-09T10:31:44Z","receivedAt":"2025-01-09T10:32:06Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Thu, Jan 9, 2025, at 07:21, Patrick Steinhardt wrote:\n> object ID. This is because fwrite(3p) of course doesn't stop when it\n> sees a NUL byte, where as printf(3p) does.\n\ns/where as/whereas\n\n-- \nKristoffer Haugsbakk\n\n\n"},{"id":"510243","messageId":"c439fcaf-11af-7862-9c3c-18dc0842b57d@gmx.de","threadId":"62772","inReplyTo":"20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im","subject":"Re: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-01-09T10:49:43Z","receivedAt":"2025-01-09T10:49:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Thu, 9 Jan 2025, Patrick Steinhardt wrote:\n\n> In 6411a0a896 (builtin/blame: fix type of `length` variable when\n> emitting object ID, 2024-12-06) we have fixed the type of the `length`\n> variable. In order to avoid a cast from `size_t` to `int` in the call to\n> printf(3p) with the \"%.*s\" formatter we have converted the code to\n> instead use fwrite(3p), which accepts the length as a `size_t`.\n>\n> It was reported though that this makes us read over the end of the OID\n> array when the provided `--abbrev=` length exceeds the length of the\n> object ID. This is because fwrite(3p) of course doesn't stop when it\n> sees a NUL byte, where as printf(3p) does.\n>\n> Fix the bug by reverting back to printf(3p) and culling the provided\n> length to `GIT_MAX_HEXSZ` to keep it from overflowing when cast to an\n> `int`.\n>\n> Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> This fixes the issue reported in [1]. Thanks!\n\nThank you for the quick fix! We will need to adjust it a little more,\nthough:\n\n>\n> Patrick\n>\n> [1]: <4d812802-afbc-4635-7a19-73896fcda625@gmx.de>\n> ---\n>  builtin/blame.c  | 4 +++-\n>  t/t8002-blame.sh | 4 ++++\n>  2 files changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 867032e4c16878ffd56df8a73162b89ca4bd2694..ad91fe9e97f90625dd2708fbd44bf2dd24a337a6 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -475,6 +475,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>  \t\tchar ch;\n>  \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n>  \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n> +\t\tif (length > GIT_MAX_HEXSZ)\n> +\t\t\tlength = GIT_MAX_HEXSZ;\n\nThis causes a subtle change of behavior because there are a couple of\nconditional code blocks between this change and the `printf()` call\ndecrease `length`, i.e. specifying values larger than the maximal hex size\ncauses potentially-desirable, different behavior (and think about\nhttps://www.hyrumslaw.com/).\n\n>\n>  \t\tif (opt & OUTPUT_COLOR_LINE) {\n>  \t\t\tif (cnt > 0) {\n> @@ -505,7 +507,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>  \t\t\tlength--;\n>  \t\t\tputchar('?');\n>  \t\t}\n> -\t\tfwrite(hex, 1, length, stdout);\n> +\t\tprintf(\"%.*s\", (int)length, hex);\n>  \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n>  \t\t\tconst char *name;\n>  \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..fcaba8c11f7ede084e069eefd292f337e8396cb4 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n>  \tcheck_abbrev $hexsz --no-abbrev\n>  '\n>\n> +test_expect_success 'blame --abbrev gets truncated' '\n> +\tcheck_abbrev 9000 --abbrev=$hexsz HEAD\n\nThis is actually incorrect: it passes `--abbrev=$hexsz` instead of a value\nthat needs to be truncated.\n\n> +'\n> +\n>  test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n>  \ttest_must_fail git blame --exclude-promisor-objects one\n>  '\n>\n> ---\n> base-commit: 14650065b76b28d3cfa9453356ac5669b19e706e\n> change-id: 20250109-b4-pks-blame-truncate-hash-length-c875cac66d71\n\nHere is my proposed fixup:\n\n-- snipsnap --\n[PATCH] fixup! builtin/blame: fix out-of-bounds read with excessive `--abbrev`\n\nThe test case needs to actually test an excessive `--abbrev` value.\n\nAlso, when calling `git blame --abbrev=<N>` with an `N` that is larger\nthan the maximal OID hex size, there is a subtle side effect that makes\nit behave _differently_ than specifying said maximal hex size: When the\ncommand outputs boundary, unblamable or ignored commits' OIDs, those\noutputs are prefixed with characters indicating this, and the `abbrev`\nvalue is used to align the information that comes after the OID,\nclipping it as needed. Specifying a \"too large\" abbrev value here tells\nGit that yes, we want the full OIDs and don't you worry about alignment.\n\nThanks to SHA-256 being _larger_ than the default SHA-1-based OIDs, and\nthanks to clipping at `GIT_MAX_HEXSZ`, this change of behavior can only\nbe observed when running the test in SHA-256 mode.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/blame.c  | 9 +++++++--\n t/t8002-blame.sh | 2 +-\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex ad91fe9e97f9..5b4976835066 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -475,8 +475,13 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\tchar ch;\n \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n-\t\tif (length > GIT_MAX_HEXSZ)\n-\t\t\tlength = GIT_MAX_HEXSZ;\n+\n+\t\t/*\n+\t\t * Leave enough space for ^, * and ? indicators (boundary,\n+\t\t * unblamable, ignored).\n+\t\t */\n+\t\tif (length > GIT_MAX_HEXSZ + 3)\n+\t\t\tlength = GIT_MAX_HEXSZ + 3;\n\n \t\tif (opt & OUTPUT_COLOR_LINE) {\n \t\t\tif (cnt > 0) {\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex fcaba8c11f7e..71fa70a64679 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -127,7 +127,7 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n '\n\n test_expect_success 'blame --abbrev gets truncated' '\n-\tcheck_abbrev 9000 --abbrev=$hexsz HEAD\n+\tcheck_abbrev 9000 --abbrev=9000 HEAD..\n '\n\n test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n--\n2.48.0.rc0.windows.1\n\n"},{"id":"510244","messageId":"Z3-vpLHvxoQCTjY1@pks.im","threadId":"62772","inReplyTo":"c439fcaf-11af-7862-9c3c-18dc0842b57d@gmx.de","subject":"Re: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-09T11:14:44Z","receivedAt":"2025-01-09T11:14:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 09, 2025 at 11:49:43AM +0100, Johannes Schindelin wrote:\n> > diff --git a/builtin/blame.c b/builtin/blame.c\n> > index 867032e4c16878ffd56df8a73162b89ca4bd2694..ad91fe9e97f90625dd2708fbd44bf2dd24a337a6 100644\n> > --- a/builtin/blame.c\n> > +++ b/builtin/blame.c\n> > @@ -475,6 +475,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n> >  \t\tchar ch;\n> >  \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n> >  \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n> > +\t\tif (length > GIT_MAX_HEXSZ)\n> > +\t\t\tlength = GIT_MAX_HEXSZ;\n> \n> This causes a subtle change of behavior because there are a couple of\n> conditional code blocks between this change and the `printf()` call\n> decrease `length`, i.e. specifying values larger than the maximal hex size\n> causes potentially-desirable, different behavior (and think about\n> https://www.hyrumslaw.com/).\n\nAlternatively we can move this until after we have done the\nsubtractions. Then we don't have to do weird gymnastics.\n\n> >\n> >  \t\tif (opt & OUTPUT_COLOR_LINE) {\n> >  \t\t\tif (cnt > 0) {\n> > @@ -505,7 +507,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n> >  \t\t\tlength--;\n> >  \t\t\tputchar('?');\n> >  \t\t}\n> > -\t\tfwrite(hex, 1, length, stdout);\n> > +\t\tprintf(\"%.*s\", (int)length, hex);\n> >  \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n> >  \t\t\tconst char *name;\n> >  \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n> > diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> > index 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..fcaba8c11f7ede084e069eefd292f337e8396cb4 100755\n> > --- a/t/t8002-blame.sh\n> > +++ b/t/t8002-blame.sh\n> > @@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n> >  \tcheck_abbrev $hexsz --no-abbrev\n> >  '\n> >\n> > +test_expect_success 'blame --abbrev gets truncated' '\n> > +\tcheck_abbrev 9000 --abbrev=$hexsz HEAD\n> \n> This is actually incorrect: it passes `--abbrev=$hexsz` instead of a value\n> that needs to be truncated.\n\nOh dear. The test did manage to catch the bug, but thinking more about\nit that was only because my initial fix was broken.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index ad91fe9e97f9..5b4976835066 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -475,8 +475,13 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>  \t\tchar ch;\n>  \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n>  \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n> -\t\tif (length > GIT_MAX_HEXSZ)\n> -\t\t\tlength = GIT_MAX_HEXSZ;\n> +\n> +\t\t/*\n> +\t\t * Leave enough space for ^, * and ? indicators (boundary,\n> +\t\t * unblamable, ignored).\n> +\t\t */\n> +\t\tif (length > GIT_MAX_HEXSZ + 3)\n> +\t\t\tlength = GIT_MAX_HEXSZ + 3;\n> \n>  \t\tif (opt & OUTPUT_COLOR_LINE) {\n>  \t\t\tif (cnt > 0) {\n\nHow about this instead?\n\n    diff --git a/builtin/blame.c b/builtin/blame.c\n    index ad91fe9e97..f92e487bed 100644\n    --- a/builtin/blame.c\n    +++ b/builtin/blame.c\n    @@ -475,8 +475,6 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n            char ch;\n            size_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n                the_hash_algo->hexsz : (size_t) abbrev;\n    -\t\tif (length > GIT_MAX_HEXSZ)\n    -\t\t\tlength = GIT_MAX_HEXSZ;\n     \n            if (opt & OUTPUT_COLOR_LINE) {\n                if (cnt > 0) {\n    @@ -507,6 +505,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n                length--;\n                putchar('?');\n            }\n    +\n    +\t\tif (length > GIT_MAX_HEXSZ)\n    +\t\t\tlength = GIT_MAX_HEXSZ;\n            printf(\"%.*s\", (int)length, hex);\n            if (opt & OUTPUT_ANNOTATE_COMPAT) {\n                const char *name;\n\nIn that case there's no need to juggle with the magic indicators, which\nmakes it a bit easier to reason about.\n\n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index fcaba8c11f7e..71fa70a64679 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -127,7 +127,7 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n>  '\n> \n>  test_expect_success 'blame --abbrev gets truncated' '\n> -\tcheck_abbrev 9000 --abbrev=$hexsz HEAD\n> +\tcheck_abbrev 9000 --abbrev=9000 HEAD..\n>  '\n\nThis should be `check_abbrev $hexsz --abbrev=9000`, shouldn't it?\n\nPatrick\n"},{"id":"510245","messageId":"20250109-b4-pks-blame-truncate-hash-length-v2-1-589c81a6ddb0@pks.im","threadId":"62772","inReplyTo":"20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im","subject":"[PATCH v2] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-09T11:48:22Z","receivedAt":"2025-01-09T11:48:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 6411a0a896 (builtin/blame: fix type of `length` variable when\nemitting object ID, 2024-12-06) we have fixed the type of the `length`\nvariable. In order to avoid a cast from `size_t` to `int` in the call to\nprintf(3p) with the \"%.*s\" formatter we have converted the code to\ninstead use fwrite(3p), which accepts the length as a `size_t`.\n\nIt was reported though that this makes us read over the end of the OID\narray when the provided `--abbrev=` length exceeds the length of the\nobject ID. This is because fwrite(3p) of course doesn't stop when it\nsees a NUL byte, whereas printf(3p) does.\n\nFix the bug by reverting back to printf(3p) and culling the provided\nlength to `GIT_MAX_HEXSZ` to keep it from overflowing when cast to an\n`int`.\n\nReported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nThis fixes the issue reported in [1]. Thanks!\n\nChanges in v2:\n\n  - Take into account that we may strip ^, * and ? indicators by moving\n    around the check.\n  - Fix the testcase so that it actually fails without the fix.\n  - Link to v1: https://lore.kernel.org/r/20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im\n\nPatrick\n\n[1]: <4d812802-afbc-4635-7a19-73896fcda625@gmx.de>\n---\n builtin/blame.c  | 5 ++++-\n t/t8002-blame.sh | 4 ++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 867032e4c16878ffd56df8a73162b89ca4bd2694..f92e487bed22eec576a4716f2e654cb61efb9903 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -505,7 +505,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\t\tlength--;\n \t\t\tputchar('?');\n \t\t}\n-\t\tfwrite(hex, 1, length, stdout);\n+\n+\t\tif (length > GIT_MAX_HEXSZ)\n+\t\t\tlength = GIT_MAX_HEXSZ;\n+\t\tprintf(\"%.*s\", (int)length, hex);\n \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n \t\t\tconst char *name;\n \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..7cf6e0253a5bbd4d6e438e627dc18b47eac4df66 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n \tcheck_abbrev $hexsz --no-abbrev\n '\n \n+test_expect_success 'blame --abbrev gets truncated' '\n+\tcheck_abbrev $hexsz --abbrev=9000 HEAD\n+'\n+\n test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git blame --exclude-promisor-objects one\n '\n\n---\nbase-commit: 14650065b76b28d3cfa9453356ac5669b19e706e\nchange-id: 20250109-b4-pks-blame-truncate-hash-length-c875cac66d71\n\n"},{"id":"510248","messageId":"Z3_Dt5SpG6qE5_9V@ArchLinux","threadId":"62772","inReplyTo":"20250109-b4-pks-blame-truncate-hash-length-v2-1-589c81a6ddb0@pks.im","subject":"Re: [PATCH v2] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-01-09T12:40:23Z","receivedAt":"2025-01-09T12:39:19Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Jan 09, 2025 at 12:48:22PM +0100, Patrick Steinhardt wrote:\n> In 6411a0a896 (builtin/blame: fix type of `length` variable when\n> emitting object ID, 2024-12-06) we have fixed the type of the `length`\n> variable. In order to avoid a cast from `size_t` to `int` in the call to\n> printf(3p) with the \"%.*s\" formatter we have converted the code to\n> instead use fwrite(3p), which accepts the length as a `size_t`.\n> \n> It was reported though that this makes us read over the end of the OID\n> array when the provided `--abbrev=` length exceeds the length of the\n> object ID. This is because fwrite(3p) of course doesn't stop when it\n> sees a NUL byte, whereas printf(3p) does.\n> \n> Fix the bug by reverting back to printf(3p) and culling the provided\n> length to `GIT_MAX_HEXSZ` to keep it from overflowing when cast to an\n> `int`.\n> \n> Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> This fixes the issue reported in [1]. Thanks!\n> \n> Changes in v2:\n> \n>   - Take into account that we may strip ^, * and ? indicators by moving\n>     around the check.\n>   - Fix the testcase so that it actually fails without the fix.\n>   - Link to v1: https://lore.kernel.org/r/20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im\n> \n> Patrick\n> \n> [1]: <4d812802-afbc-4635-7a19-73896fcda625@gmx.de>\n> ---\n>  builtin/blame.c  | 5 ++++-\n>  t/t8002-blame.sh | 4 ++++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 867032e4c16878ffd56df8a73162b89ca4bd2694..f92e487bed22eec576a4716f2e654cb61efb9903 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -505,7 +505,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>  \t\t\tlength--;\n>  \t\t\tputchar('?');\n>  \t\t}\n> -\t\tfwrite(hex, 1, length, stdout);\n> +\n> +\t\tif (length > GIT_MAX_HEXSZ)\n> +\t\t\tlength = GIT_MAX_HEXSZ;\n\nHere, we explicitly set the length if it exceeds the max hex of the\nrepo. Although `printf` will automatically hide the length, we should\nexplicitly do this. Make sense.\n\n> +\t\tprintf(\"%.*s\", (int)length, hex);\n>  \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n>  \t\t\tconst char *name;\n>  \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..7cf6e0253a5bbd4d6e438e627dc18b47eac4df66 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n>  \tcheck_abbrev $hexsz --no-abbrev\n>  '\n>  \n> +test_expect_success 'blame --abbrev gets truncated' '\n> +\tcheck_abbrev $hexsz --abbrev=9000 HEAD\n> +'\n> +\n\nBy the way, I feel this usage is a little strange as the user side. When\nI received the report mail from Johannes today morning, I feel a little\nfunny that we allow the value of the `--abrrev` option exceeds the\n`GIT_MAX_HEXSZ` in the first place.\n\n>  test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n>  \ttest_must_fail git blame --exclude-promisor-objects one\n>  '\n> \n> ---\n> base-commit: 14650065b76b28d3cfa9453356ac5669b19e706e\n> change-id: 20250109-b4-pks-blame-truncate-hash-length-c875cac66d71\n> \n\nThanks,\nJialuo\n"},{"id":"510250","messageId":"02ab59c0-a495-9f96-6323-af73cb483f38@gmx.de","threadId":"62772","inReplyTo":"Z3-vpLHvxoQCTjY1@pks.im","subject":"Re: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-01-09T13:41:16Z","receivedAt":"2025-01-09T13:41:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Thu, 9 Jan 2025, Patrick Steinhardt wrote:\n\n> On Thu, Jan 09, 2025 at 11:49:43AM +0100, Johannes Schindelin wrote:\n> > > diff --git a/builtin/blame.c b/builtin/blame.c\n> > > index 867032e4c16878ffd56df8a73162b89ca4bd2694..ad91fe9e97f90625dd2708fbd44bf2dd24a337a6 100644\n> > > --- a/builtin/blame.c\n> > > +++ b/builtin/blame.c\n> > > @@ -475,6 +475,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n> > >  \t\tchar ch;\n> > >  \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n> > >  \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n> > > +\t\tif (length > GIT_MAX_HEXSZ)\n> > > +\t\t\tlength = GIT_MAX_HEXSZ;\n> >\n> > This causes a subtle change of behavior because there are a couple of\n> > conditional code blocks between this change and the `printf()` call\n> > decrease `length`, i.e. specifying values larger than the maximal hex size\n> > causes potentially-desirable, different behavior (and think about\n> > https://www.hyrumslaw.com/).\n>\n> Alternatively we can move this until after we have done the\n> subtractions. Then we don't have to do weird gymnastics.\n\nOr we can even avoid assiging a maximum altogether:\n\n\t\tif (length < GIT_MAX_HEXSZ)\n\t\t\tprintf(\"%.*s\", (int)length, hex);\n\t\telse\n\t\t\tprintf(\"%s\", hex);\n\nOr be more consistent with Git's source code style which often prefers\nternaries, favoring succinctness over readability:\n\n\t\tprintf(\"%.*s\", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);\n\n> > >  \t\tif (opt & OUTPUT_COLOR_LINE) {\n> > >  \t\t\tif (cnt > 0) {\n> > > @@ -505,7 +507,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n> > >  \t\t\tlength--;\n> > >  \t\t\tputchar('?');\n> > >  \t\t}\n> > > -\t\tfwrite(hex, 1, length, stdout);\n> > > +\t\tprintf(\"%.*s\", (int)length, hex);\n> > >  \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n> > >  \t\t\tconst char *name;\n> > >  \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n> > > diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> > > index 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..fcaba8c11f7ede084e069eefd292f337e8396cb4 100755\n> > > --- a/t/t8002-blame.sh\n> > > +++ b/t/t8002-blame.sh\n> > > @@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n> > >  \tcheck_abbrev $hexsz --no-abbrev\n> > >  '\n> > >\n> > > +test_expect_success 'blame --abbrev gets truncated' '\n> > > +\tcheck_abbrev 9000 --abbrev=$hexsz HEAD\n> >\n> > This is actually incorrect: it passes `--abbrev=$hexsz` instead of a value\n> > that needs to be truncated.\n>\n> Oh dear. The test did manage to catch the bug, but thinking more about\n> it that was only because my initial fix was broken.\n>\n> > diff --git a/builtin/blame.c b/builtin/blame.c\n> > index ad91fe9e97f9..5b4976835066 100644\n> > --- a/builtin/blame.c\n> > +++ b/builtin/blame.c\n> > @@ -475,8 +475,13 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n> >  \t\tchar ch;\n> >  \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n> >  \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n> > -\t\tif (length > GIT_MAX_HEXSZ)\n> > -\t\t\tlength = GIT_MAX_HEXSZ;\n> > +\n> > +\t\t/*\n> > +\t\t * Leave enough space for ^, * and ? indicators (boundary,\n> > +\t\t * unblamable, ignored).\n> > +\t\t */\n> > +\t\tif (length > GIT_MAX_HEXSZ + 3)\n> > +\t\t\tlength = GIT_MAX_HEXSZ + 3;\n> >\n> >  \t\tif (opt & OUTPUT_COLOR_LINE) {\n> >  \t\t\tif (cnt > 0) {\n>\n> How about this instead?\n>\n>     diff --git a/builtin/blame.c b/builtin/blame.c\n>     index ad91fe9e97..f92e487bed 100644\n>     --- a/builtin/blame.c\n>     +++ b/builtin/blame.c\n>     @@ -475,8 +475,6 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>             char ch;\n>             size_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n>                 the_hash_algo->hexsz : (size_t) abbrev;\n>     -\t\tif (length > GIT_MAX_HEXSZ)\n>     -\t\t\tlength = GIT_MAX_HEXSZ;\n>\n>             if (opt & OUTPUT_COLOR_LINE) {\n>                 if (cnt > 0) {\n>     @@ -507,6 +505,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>                 length--;\n>                 putchar('?');\n>             }\n>     +\n>     +\t\tif (length > GIT_MAX_HEXSZ)\n>     +\t\t\tlength = GIT_MAX_HEXSZ;\n>             printf(\"%.*s\", (int)length, hex);\n>             if (opt & OUTPUT_ANNOTATE_COMPAT) {\n>                 const char *name;\n>\n> In that case there's no need to juggle with the magic indicators, which\n> makes it a bit easier to reason about.\n>\n> > diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> > index fcaba8c11f7e..71fa70a64679 100755\n> > --- a/t/t8002-blame.sh\n> > +++ b/t/t8002-blame.sh\n> > @@ -127,7 +127,7 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n> >  '\n> >\n> >  test_expect_success 'blame --abbrev gets truncated' '\n> > -\tcheck_abbrev 9000 --abbrev=$hexsz HEAD\n> > +\tcheck_abbrev 9000 --abbrev=9000 HEAD..\n> >  '\n>\n> This should be `check_abbrev $hexsz --abbrev=9000`, shouldn't it?\n\nI kind of liked the idea to keep the same cut-off threshold for the\nvalidation. But I won't insist. The construct is obtuse in either case ;-)\n(Not your fault, of course, you merely imitated existing code, which is\nthe correct thing to do).\n\nCiao,\nJohannes\n"},{"id":"510251","messageId":"b09009f0-c822-f81d-1f79-07610ec20193@gmx.de","threadId":"62772","inReplyTo":"20250109-b4-pks-blame-truncate-hash-length-v2-1-589c81a6ddb0@pks.im","subject":"Re: [PATCH v2] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-01-09T13:43:25Z","receivedAt":"2025-01-09T13:43:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Thu, 9 Jan 2025, Patrick Steinhardt wrote:\n\n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..7cf6e0253a5bbd4d6e438e627dc18b47eac4df66 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n>  \tcheck_abbrev $hexsz --no-abbrev\n>  '\n>\n> +test_expect_success 'blame --abbrev gets truncated' '\n> +\tcheck_abbrev $hexsz --abbrev=9000 HEAD\n\nPlease note that the patch I proposed had `HEAD..` here, not `HEAD`. The\nlatter would not have surfaced the problem with boundary commits.\n\nCiao,\nJohannes\n"},{"id":"510252","messageId":"2db1688e-bf2b-8f53-82d0-11c453a64cd8@gmx.de","threadId":"62772","inReplyTo":"Z3_Dt5SpG6qE5_9V@ArchLinux","subject":"Re: [PATCH v2] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-01-09T13:49:09Z","receivedAt":"2025-01-09T13:49:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jialuo,\n\nOn Thu, 9 Jan 2025, shejialuo wrote:\n\n> On Thu, Jan 09, 2025 at 12:48:22PM +0100, Patrick Steinhardt wrote:\n>\n> > +\t\tprintf(\"%.*s\", (int)length, hex);\n> >  \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n> >  \t\t\tconst char *name;\n> >  \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n> > diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> > index 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..7cf6e0253a5bbd4d6e438e627dc18b47eac4df66 100755\n> > --- a/t/t8002-blame.sh\n> > +++ b/t/t8002-blame.sh\n> > @@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n> >  \tcheck_abbrev $hexsz --no-abbrev\n> >  '\n> >\n> > +test_expect_success 'blame --abbrev gets truncated' '\n> > +\tcheck_abbrev $hexsz --abbrev=9000 HEAD\n> > +'\n> > +\n>\n> By the way, I feel this usage is a little strange as the user side. When\n> I received the report mail from Johannes today morning, I feel a little\n> funny that we allow the value of the `--abrrev` option exceeds the\n> `GIT_MAX_HEXSZ` in the first place.\n\nSee the explanation I provided in\nhttps://lore.kernel.org/git/c439fcaf-11af-7862-9c3c-18dc0842b57d@gmx.de/:\nWhen calling `git blame --abbrev=40 HEAD.. -- <file>` (in a SHA-1-based\nrepository), the OIDs are prefixed with a `^` and then the last hex digit\nwill be cut. The reason? Git wants to align the text after the OID.\n\nWhen calling it with `--abbrev=41`, the full OID is shown.\n\nCiao,\nJohannes\n"},{"id":"510256","messageId":"xmqqldvkkp8j.fsf@gitster.g","threadId":"62772","inReplyTo":"20250109-b4-pks-blame-truncate-hash-length-v2-1-589c81a6ddb0@pks.im","subject":"Re: [PATCH v2] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-09T14:59:08Z","receivedAt":"2025-01-09T14:59:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 867032e4c16878ffd56df8a73162b89ca4bd2694..f92e487bed22eec576a4716f2e654cb61efb9903 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -505,7 +505,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n\nMental note: the \"hex\" in question that is used later is defined as\n\n\tchar hex[GIT_MAX_HEXSZ + 1];\n\nat the top of this scope.\n\n>  \t\t\tlength--;\n>  \t\t\tputchar('?');\n>  \t\t}\n> -\t\tfwrite(hex, 1, length, stdout);\n> +\n> +\t\tif (length > GIT_MAX_HEXSZ)\n> +\t\t\tlength = GIT_MAX_HEXSZ;\n> +\t\tprintf(\"%.*s\", (int)length, hex);\n\nThis is not wrong per-se, but leaves a funny aftertaste after\nreading it, especially if the reader knows the original *bug* was\nabout showing past the end of the string in \"hex\".  The question is\n\"what if the current hash function produces shorter than MAX_HEXSZ?\"\n\nThe updated code is correct only because it reinstates the use of\nprintf() so the \"length\" being longer than the string itself no\nlonger matters, and the only requirement on \"length\" is not to\nread beyond the end of hex[] array.  So feeding the length of the\narray as the limit is not wrong, even though it may be feeding a\nlimit that is larger than the string stored in the array.\n\nAn obvious alternative would have been to base the limit on the\nvalue of strlen(hex) and then we may even keep using fwrite(); then\nwe do not have worry about \"what if MAX_HEXSZ is larger than the\ncurrent hash function?\" and nonsense like that (I personally prefer\nwhat is being reviewed much better; please do not take this \"obvious\nalternative\" as a suggestion).\n\nLimiting the \"length\" here does improve the resulting code, relative\nto v1 iteration.  This printf is about showing the hexstring and we\nare making sure we do not read past the end of the array that holds\nthe string, and doing it here means we do not have to worry about\nleading prefix characters about boundary etc.  Very good\nimprovement.\n\nThanks.\n"},{"id":"510281","messageId":"48ca0114-124b-e3f5-af80-1e302bf9ce52@gmx.de","threadId":"62772","inReplyTo":"02ab59c0-a495-9f96-6323-af73cb483f38@gmx.de","subject":"Re: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-01-10T09:27:23Z","receivedAt":"2025-01-10T09:27:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Thu, 9 Jan 2025, Johannes Schindelin wrote:\n\n> On Thu, 9 Jan 2025, Patrick Steinhardt wrote:\n>\n> > On Thu, Jan 09, 2025 at 11:49:43AM +0100, Johannes Schindelin wrote:\n> > > > diff --git a/builtin/blame.c b/builtin/blame.c\n> > > > index 867032e4c16878ffd56df8a73162b89ca4bd2694..ad91fe9e97f90625dd2708fbd44bf2dd24a337a6 100644\n> > > > --- a/builtin/blame.c\n> > > > +++ b/builtin/blame.c\n> > > > @@ -475,6 +475,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n> > > >  \t\tchar ch;\n> > > >  \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n> > > >  \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n> > > > +\t\tif (length > GIT_MAX_HEXSZ)\n> > > > +\t\t\tlength = GIT_MAX_HEXSZ;\n> > >\n> > > This causes a subtle change of behavior because there are a couple of\n> > > conditional code blocks between this change and the `printf()` call\n> > > decrease `length`, i.e. specifying values larger than the maximal hex size\n> > > causes potentially-desirable, different behavior (and think about\n> > > https://www.hyrumslaw.com/).\n> >\n> > Alternatively we can move this until after we have done the\n> > subtractions. Then we don't have to do weird gymnastics.\n>\n> Or we can even avoid assiging a maximum altogether:\n>\n> \t\tif (length < GIT_MAX_HEXSZ)\n> \t\t\tprintf(\"%.*s\", (int)length, hex);\n> \t\telse\n> \t\t\tprintf(\"%s\", hex);\n>\n> Or be more consistent with Git's source code style which often prefers\n> ternaries, favoring succinctness over readability:\n>\n> \t\tprintf(\"%.*s\", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);\n\nCoverity noticed a problem with this approach, looking at\nhttps://github.com/git/git/blob/v2.48.0-rc2/builtin/blame.c#L493:\n\n\t\t\t\tmemset(hex, ' ', length);\n\nIf the `GIT_MAX_HEXSZ` guard is moved after this statement, then we can\neasily overrun the `hex` buffer.\n\nCiao,\nJohannes\n"},{"id":"510283","messageId":"Z4DtMPN_sK4Hqxs3@pks.im","threadId":"62772","inReplyTo":"48ca0114-124b-e3f5-af80-1e302bf9ce52@gmx.de","subject":"Re: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-10T09:49:42Z","receivedAt":"2025-01-10T09:49:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 10, 2025 at 10:27:23AM +0100, Johannes Schindelin wrote:\n> Hi Patrick,\n> \n> On Thu, 9 Jan 2025, Johannes Schindelin wrote:\n> \n> > On Thu, 9 Jan 2025, Patrick Steinhardt wrote:\n> >\n> > > On Thu, Jan 09, 2025 at 11:49:43AM +0100, Johannes Schindelin wrote:\n> > > > > diff --git a/builtin/blame.c b/builtin/blame.c\n> > > > > index 867032e4c16878ffd56df8a73162b89ca4bd2694..ad91fe9e97f90625dd2708fbd44bf2dd24a337a6 100644\n> > > > > --- a/builtin/blame.c\n> > > > > +++ b/builtin/blame.c\n> > > > > @@ -475,6 +475,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n> > > > >  \t\tchar ch;\n> > > > >  \t\tsize_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?\n> > > > >  \t\t\tthe_hash_algo->hexsz : (size_t) abbrev;\n> > > > > +\t\tif (length > GIT_MAX_HEXSZ)\n> > > > > +\t\t\tlength = GIT_MAX_HEXSZ;\n> > > >\n> > > > This causes a subtle change of behavior because there are a couple of\n> > > > conditional code blocks between this change and the `printf()` call\n> > > > decrease `length`, i.e. specifying values larger than the maximal hex size\n> > > > causes potentially-desirable, different behavior (and think about\n> > > > https://www.hyrumslaw.com/).\n> > >\n> > > Alternatively we can move this until after we have done the\n> > > subtractions. Then we don't have to do weird gymnastics.\n> >\n> > Or we can even avoid assiging a maximum altogether:\n> >\n> > \t\tif (length < GIT_MAX_HEXSZ)\n> > \t\t\tprintf(\"%.*s\", (int)length, hex);\n> > \t\telse\n> > \t\t\tprintf(\"%s\", hex);\n> >\n> > Or be more consistent with Git's source code style which often prefers\n> > ternaries, favoring succinctness over readability:\n> >\n> > \t\tprintf(\"%.*s\", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);\n> \n> Coverity noticed a problem with this approach, looking at\n> https://github.com/git/git/blob/v2.48.0-rc2/builtin/blame.c#L493:\n> \n> \t\t\t\tmemset(hex, ' ', length);\n> \n> If the `GIT_MAX_HEXSZ` guard is moved after this statement, then we can\n> easily overrun the `hex` buffer.\n\nOh. That's even an old-standing issue that wasn't caused by the\nrefactoring, right? Your proposed fix to set `length = GIT_MAX_HEXSZ +\n3` to account for the old behaviour wouldn't fix it either, as we could\nstill end up overwriting two bytes.\n\nI'll send a new version with another commit on top to fix this.\n\nPatrick\n"},{"id":"510291","messageId":"20250110-b4-pks-blame-truncate-hash-length-v3-0-e61f25b68f30@pks.im","threadId":"62772","inReplyTo":"20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im","subject":"[PATCH v3 0/2] builtin/blame: fix out-of-bounds reads and writes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-10T11:26:16Z","receivedAt":"2025-01-10T11:26:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nThis fixes the issues reported in [1] and [2]. Thanks!\n\nChanges in v2:\n\n  - Take into account that we may strip ^, * and ? indicators by moving\n    around the check.\n  - Fix the testcase so that it actually fails without the fix.\n  - Link to v1: https://lore.kernel.org/r/20250109-b4-pks-blame-truncate-hash-length-v1-1-9ad4bb09e059@pks.im\n\nChanges in v3:\n\n  - Add another testcase for boundary commits.\n  - Fix another out-of-bound write noticed by Coverity. This bug is not\n    a regression in v2.48.0, but is a preexisting error.\n  - Simplify the printf statement a bit by using a ternary statement.\n  - Link to v2: https://lore.kernel.org/r/20250109-b4-pks-blame-truncate-hash-length-v2-1-589c81a6ddb0@pks.im\n\nPatrick\n\n[1]: <4d812802-afbc-4635-7a19-73896fcda625@gmx.de>\n[2]: <48ca0114-124b-e3f5-af80-1e302bf9ce52@gmx.de>\n\n---\nPatrick Steinhardt (2):\n      builtin/blame: fix out-of-bounds read with excessive `--abbrev`\n      builtin/blame: fix out-of-bounds write with blank boundary commits\n\n builtin/blame.c  |  9 +++++----\n t/t8002-blame.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 4 deletions(-)\n\nRange-diff versus v2:\n\n1:  7065637d8e ! 1:  3865bfe643 builtin/blame: fix out-of-bounds read with excessive `--abbrev`\n    @@ builtin/blame.c: static void emit_other(struct blame_scoreboard *sb, struct blam\n      \t\t}\n     -\t\tfwrite(hex, 1, length, stdout);\n     +\n    -+\t\tif (length > GIT_MAX_HEXSZ)\n    -+\t\t\tlength = GIT_MAX_HEXSZ;\n    -+\t\tprintf(\"%.*s\", (int)length, hex);\n    ++\t\tprintf(\"%.*s\", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);\n      \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n      \t\t\tconst char *name;\n      \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n    @@ t/t8002-blame.sh: test_expect_success '--no-abbrev works like --abbrev with full\n     +test_expect_success 'blame --abbrev gets truncated' '\n     +\tcheck_abbrev $hexsz --abbrev=9000 HEAD\n     +'\n    ++\n    ++test_expect_success 'blame --abbrev gets truncated with boundary commit' '\n    ++\tcheck_abbrev $hexsz --abbrev=9000 ^HEAD\n    ++'\n     +\n      test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n      \ttest_must_fail git blame --exclude-promisor-objects one\n-:  ---------- > 2:  af0af67a8a builtin/blame: fix out-of-bounds write with blank boundary commits\n\n---\nbase-commit: 14650065b76b28d3cfa9453356ac5669b19e706e\nchange-id: 20250109-b4-pks-blame-truncate-hash-length-c875cac66d71\n\n"},{"id":"510293","messageId":"20250110-b4-pks-blame-truncate-hash-length-v3-1-e61f25b68f30@pks.im","threadId":"62772","inReplyTo":"20250110-b4-pks-blame-truncate-hash-length-v3-0-e61f25b68f30@pks.im","subject":"[PATCH v3 1/2] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-10T11:26:17Z","receivedAt":"2025-01-10T11:26:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 6411a0a896 (builtin/blame: fix type of `length` variable when\nemitting object ID, 2024-12-06) we have fixed the type of the `length`\nvariable. In order to avoid a cast from `size_t` to `int` in the call to\nprintf(3p) with the \"%.*s\" formatter we have converted the code to\ninstead use fwrite(3p), which accepts the length as a `size_t`.\n\nIt was reported though that this makes us read over the end of the OID\narray when the provided `--abbrev=` length exceeds the length of the\nobject ID. This is because fwrite(3p) of course doesn't stop when it\nsees a NUL byte, whereas printf(3p) does.\n\nFix the bug by reverting back to printf(3p) and culling the provided\nlength to `GIT_MAX_HEXSZ` to keep it from overflowing when cast to an\n`int`.\n\nReported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/blame.c  | 3 ++-\n t/t8002-blame.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 867032e4c16878ffd56df8a73162b89ca4bd2694..d7630ac89cb7bd6e9ce5d72c6a98aa433b3b12da 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -505,7 +505,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\t\tlength--;\n \t\t\tputchar('?');\n \t\t}\n-\t\tfwrite(hex, 1, length, stdout);\n+\n+\t\tprintf(\"%.*s\", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);\n \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n \t\t\tconst char *name;\n \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..b3f8b63d2e6744dd434f38fd9f10b56cd432141b 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -126,6 +126,14 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n \tcheck_abbrev $hexsz --no-abbrev\n '\n \n+test_expect_success 'blame --abbrev gets truncated' '\n+\tcheck_abbrev $hexsz --abbrev=9000 HEAD\n+'\n+\n+test_expect_success 'blame --abbrev gets truncated with boundary commit' '\n+\tcheck_abbrev $hexsz --abbrev=9000 ^HEAD\n+'\n+\n test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git blame --exclude-promisor-objects one\n '\n\n-- \n2.48.0.rc2.279.g1de40edade.dirty\n\n"},{"id":"510292","messageId":"20250110-b4-pks-blame-truncate-hash-length-v3-2-e61f25b68f30@pks.im","threadId":"62772","inReplyTo":"20250110-b4-pks-blame-truncate-hash-length-v3-0-e61f25b68f30@pks.im","subject":"[PATCH v3 2/2] builtin/blame: fix out-of-bounds write with blank boundary commits","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-10T11:26:18Z","receivedAt":"2025-01-10T11:26:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When passing the `-b` flag to git-blame(1), then any blamed boundary\ncommits which were marked as uninteresting will not get their actual\ncommit ID printed, but will instead be replaced by a couple of spaces.\n\nThe flag can lead to an out-of-bounds write as though when combined with\n`--abbrev=` when the abbreviation length is longer than `GIT_MAX_HEXSZ`\nas we simply use memset(3p) on that array with the user-provided length\ndirectly. The result is most likely that we segfault.\n\nAn obvious fix would be to cull `length` to `GIT_MAX_HEXSZ` many bytes.\nBut when the underlying object ID is SHA1, and if the abbreviated length\nexceeds the SHA1 length, it would cause us to print more bytes than\ndesired, and the result would be misaligned.\n\nInstead, fix the bug by computing the length via strlen(3p). This makes\nus write as many bytes as the formatted object ID requires and thus\neffectively limits the length of what we may end up printing to the\nlength of its hash. If `--abbrev=` asks us to abbreviate to something\nshorter than the full length of the underlying hash function it would be\nhandled by the call to printf(3p) correctly.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/blame.c  |  6 +++---\n t/t8002-blame.sh | 18 ++++++++++++++++++\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex d7630ac89cb7bd6e9ce5d72c6a98aa433b3b12da..7555c445abe7ca2fa54670ac8fee1d95a6dbafe3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -489,9 +489,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n \t\t\tfputs(color, stdout);\n \n \t\tif (suspect->commit->object.flags & UNINTERESTING) {\n-\t\t\tif (blank_boundary)\n-\t\t\t\tmemset(hex, ' ', length);\n-\t\t\telse if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {\n+\t\t\tif (blank_boundary) {\n+\t\t\t\tmemset(hex, ' ', strlen(hex));\n+\t\t\t} else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {\n \t\t\t\tlength--;\n \t\t\t\tputchar('^');\n \t\t\t}\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex b3f8b63d2e6744dd434f38fd9f10b56cd432141b..1ad039e1234828ca8779ad76147bfa7fe14c5a2e 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -134,6 +134,24 @@ test_expect_success 'blame --abbrev gets truncated with boundary commit' '\n \tcheck_abbrev $hexsz --abbrev=9000 ^HEAD\n '\n \n+test_expect_success 'blame --abbrev -b truncates the blank boundary' '\n+\t# Note that `--abbrev=` always gets incremented by 1, which is why we\n+\t# expect 11 leading spaces and not 10.\n+\tcat >expect <<-EOF &&\n+\t$(printf \"%0.s \" $(test_seq 11)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n+\tEOF\n+\tgit blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'blame with excessive --abbrev and -b culls to hash length' '\n+\tcat >expect <<-EOF &&\n+\t$(printf \"%0.s \" $(test_seq $hexsz)) (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev\n+\tEOF\n+\tgit blame -b --abbrev=9000 ^HEAD -- abbrev.t >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git blame --exclude-promisor-objects one\n '\n\n-- \n2.48.0.rc2.279.g1de40edade.dirty\n\n"},{"id":"510313","messageId":"Z4EPjW1jGAMFGfJ_@ArchLinux","threadId":"62772","inReplyTo":"2db1688e-bf2b-8f53-82d0-11c453a64cd8@gmx.de","subject":"Re: [PATCH v2] builtin/blame: fix out-of-bounds read with excessive `--abbrev`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-01-10T12:16:13Z","receivedAt":"2025-01-10T12:15:09Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Jan 09, 2025 at 02:49:09PM +0100, Johannes Schindelin wrote:\n> Hi Jialuo,\n> \n> On Thu, 9 Jan 2025, shejialuo wrote:\n> \n> > On Thu, Jan 09, 2025 at 12:48:22PM +0100, Patrick Steinhardt wrote:\n> >\n> > > +\t\tprintf(\"%.*s\", (int)length, hex);\n> > >  \t\tif (opt & OUTPUT_ANNOTATE_COMPAT) {\n> > >  \t\t\tconst char *name;\n> > >  \t\t\tif (opt & OUTPUT_SHOW_EMAIL)\n> > > diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> > > index 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..7cf6e0253a5bbd4d6e438e627dc18b47eac4df66 100755\n> > > --- a/t/t8002-blame.sh\n> > > +++ b/t/t8002-blame.sh\n> > > @@ -126,6 +126,10 @@ test_expect_success '--no-abbrev works like --abbrev with full length' '\n> > >  \tcheck_abbrev $hexsz --no-abbrev\n> > >  '\n> > >\n> > > +test_expect_success 'blame --abbrev gets truncated' '\n> > > +\tcheck_abbrev $hexsz --abbrev=9000 HEAD\n> > > +'\n> > > +\n> >\n> > By the way, I feel this usage is a little strange as the user side. When\n> > I received the report mail from Johannes today morning, I feel a little\n> > funny that we allow the value of the `--abrrev` option exceeds the\n> > `GIT_MAX_HEXSZ` in the first place.\n> \n> See the explanation I provided in\n> https://lore.kernel.org/git/c439fcaf-11af-7862-9c3c-18dc0842b57d@gmx.de/:\n> When calling `git blame --abbrev=40 HEAD.. -- <file>` (in a SHA-1-based\n> repository), the OIDs are prefixed with a `^` and then the last hex digit\n> will be cut. The reason? Git wants to align the text after the OID.\n> \n\nI have read through this, thanks for the detailed explanation.\n\nThanks,\nJialuo\n"},{"id":"510324","messageId":"992502eb-5131-228e-1915-a35038211f4b@gmx.de","threadId":"62772","inReplyTo":"20250110-b4-pks-blame-truncate-hash-length-v3-2-e61f25b68f30@pks.im","subject":"Re: [PATCH v3 2/2] builtin/blame: fix out-of-bounds write with blank boundary commits","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-01-10T13:00:41Z","receivedAt":"2025-01-10T13:00:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Fri, 10 Jan 2025, Patrick Steinhardt wrote:\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index d7630ac89cb7bd6e9ce5d72c6a98aa433b3b12da..7555c445abe7ca2fa54670ac8fee1d95a6dbafe3 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -489,9 +489,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>  \t\t\tfputs(color, stdout);\n>\n>  \t\tif (suspect->commit->object.flags & UNINTERESTING) {\n> -\t\t\tif (blank_boundary)\n> -\t\t\t\tmemset(hex, ' ', length);\n> -\t\t\telse if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {\n> +\t\t\tif (blank_boundary) {\n> +\t\t\t\tmemset(hex, ' ', strlen(hex));\n\nUsing `strlen()` is a neat trick.\n\nI could have done without slipping in a style change (introducing\ncurlies), but the most important thing is that it fixes the bug.\n\nThank you,\nJohannes\n\n> +\t\t\t} else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {\n>  \t\t\t\tlength--;\n>  \t\t\t\tputchar('^');\n>  \t\t\t}\n"},{"id":"510325","messageId":"xmqq5xmmagvv.fsf@gitster.g","threadId":"62772","inReplyTo":"992502eb-5131-228e-1915-a35038211f4b@gmx.de","subject":"Re: [PATCH v3 2/2] builtin/blame: fix out-of-bounds write with blank boundary commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-10T14:21:56Z","receivedAt":"2025-01-10T14:21:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Fri, 10 Jan 2025, Patrick Steinhardt wrote:\n>\n>> diff --git a/builtin/blame.c b/builtin/blame.c\n>> index d7630ac89cb7bd6e9ce5d72c6a98aa433b3b12da..7555c445abe7ca2fa54670ac8fee1d95a6dbafe3 100644\n>> --- a/builtin/blame.c\n>> +++ b/builtin/blame.c\n>> @@ -489,9 +489,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int\n>>  \t\t\tfputs(color, stdout);\n>>\n>>  \t\tif (suspect->commit->object.flags & UNINTERESTING) {\n>> -\t\t\tif (blank_boundary)\n>> -\t\t\t\tmemset(hex, ' ', length);\n>> -\t\t\telse if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {\n>> +\t\t\tif (blank_boundary) {\n>> +\t\t\t\tmemset(hex, ' ', strlen(hex));\n>\n> Using `strlen()` is a neat trick.\n>\n> I could have done without slipping in a style change (introducing\n> curlies), but the most important thing is that it fixes the bug.\n\nThank both of you for these last-minute fixes.  Hopefully we can\nhave them in today's release, and we didn't miss unexpected side\neffects in them, I hope ;-).\n\n\n\n"}]}