{"thread":{"id":"60536","subject":"[PATCH] object-name: reject too-deep recursive ancestor queries","startedAt":"2023-11-20T16:13:48Z","lastAt":"2023-12-07T06:52:25Z","messageCount":7,"participants":["Taylor Blau","Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"485016","messageId":"57c0b30ddfe7c0ae78069682ff8454791e54469f.1700496801.git.me@ttaylorr.com","threadId":"60536","inReplyTo":null,"subject":"[PATCH] object-name: reject too-deep recursive ancestor queries","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-20T16:13:45Z","receivedAt":"2023-11-20T16:13:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When trying to resolve a revision query like \"HEAD~~~~~\", our call\npattern looks something like:\n\n - object-name.c::get_oid_with_context()\n - object-name.c::get_oid_1()\n - object-name.c::get_nth_ancestor()\n - object-name.c::get_oid_1()\n - ...\n\nWith `get_nth_ancestor()` and `get_oid_1()` mutually recurring, popping\none '~' off of the revision query for each round of the recursion.\n\nSince this recursive behavior is unbounded, having too many \"~\"'s\ncontained in a revision query will cause us to blow the stack.\nGenerating a message like this when compiled under SANITIZE=address:\n\n $ valgrind git rev-parse \"HEAD$(perl -e \"print \\\"~\\\" x 1000000000000\")\"\n ==597453== Memcheck, a memory error detector\n ==597453== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.\n ==597453== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info\n ==597453== Command: /home/ttaylorr/local/bin/git.compile diff HEAD~~~~~~~~~~~~[...]\n ==597453==\n AddressSanitizer:DEADLYSIGNAL\n =================================================================\n ==597453==ERROR: AddressSanitizer: stack-overflow on address 0x7fffdd838ff8 (pc 0x7f2726082748 bp 0x7fffdd839110 sp 0x7fffdd839000 T0)\n #0 0x7f2726082748 in __asan::GetTLSFakeStack() ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:176\n #1 0x7f2726082748 in GetFakeStackFast ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:193\n #2 0x7f27260833de in OnMalloc ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:207\n #3 0x7f27260833de in __asan_stack_malloc_1 ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:256\n #4 0x563f9077d9d8 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1087\n #5 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295\n #6 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092\n #7 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295\n #8 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092\n [...]\n #247 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295\n #248 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092\n\n SUMMARY: AddressSanitizer: stack-overflow ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:176 in __asan::GetTLSFakeStack()\n ==597453==ABORTING\n\n(Note that the actual stack is much deeper. GDB reports that the bottom\nof the stack looks something like the following):\n\n #54866 0x0000555555c6d3bf in get_oid_with_context_1 (repo=0x5555563849a0 , name=0x7fffffff4be5 \"HEAD\", '~' ..., flags=128, prefix=0x0, oid=0x7ffff5713d40, oc=0x7ffff5713d90) at object-name.c:1947\n #54867 0x0000555555c6e2fa in get_oid_with_context (repo=0x5555563849a0 , str=0x7fffffff4be5 \"HEAD\", '~' ..., flags=128, oid=0x7ffff5713d40, oc=0x7ffff5713d90) at object-name.c:2096\n #54868 0x0000555555d8eed8 in handle_revision_arg_1 (arg_=0x7fffffff4be5 \"HEAD\", '~' ..., revs=0x7ffff5b000d0, flags=0, revarg_opt=0) at revision.c:2174\n #54869 0x0000555555d8f1a9 in handle_revision_arg (arg=0x7fffffff4be5 \"HEAD\", '~' ..., revs=0x7ffff5b000d0, flags=0, revarg_opt=0) at revision.c:2189\n #54870 0x0000555555d97ca9 in setup_revisions (argc=2, argv=0x7fffffff4970, revs=0x7ffff5b000d0, opt=0x0) at revision.c:2932\n #54871 0x00005555557d6a63 in cmd_diff (argc=2, argv=0x7fffffff4970, prefix=0x0) at builtin/diff.c:502\n #54872 0x00005555557367bf in run_builtin (p=0x5555561c4c30 , argc=2, argv=0x7fffffff4970) at git.c:469\n #54873 0x000055555573716b in handle_builtin (argc=2, argv=0x7fffffff4970) at git.c:723\n #54874 0x000055555573785a in run_argv (argcp=0x7ffff56028b0, argv=0x7ffff56028e0) at git.c:787\n #54875 0x0000555555738626 in cmd_main (argc=2, argv=0x7fffffff4970) at git.c:922\n #54876 0x00005555559d3fdd in main (argc=3, argv=0x7fffffff4968) at common-main.c:62\n\nFortunately, we can impose a limit on the maximum recursion depth we're\nwilling to accept when resolving queries like the above without\nsignificantly impeding users. This patch sets the limit at 4096, though\nwe could probably increase that limit depending on the size of each\nframe.\n\nThe limit introduced here is large enough that any reasonable query\nshould still run to completion, but small enough that if the frame size\nwere to significantly increase, our protection would still be effective.\n\nThe change here is straightforward: each call to get_nth_ancestor()\nincreases a counter, and then decrements that counter before returning.\n\nThe diff is a little noisy since there are a handful of return paths\nfrom `get_nth_ancestor()`, all of which need to decrement the depth\nvariable.\n\nSince this is a local-only exploit, a user would have to be tricked into\nrunning such a query by an adversary. Even if they were successfully\ntricked into running the malicious query, the blast radius is limited to\na local stack overflow, which does not have meaningful paths to remote\ncode execution, arbitrary memory reads, or any more grave security\nconcerns.\n\nReported-by: Carlos Andrés Ramírez Cataño \nSigned-off-by: Taylor Blau \n---\n object-name.c | 26 ++++++++++++++++++++------\n t/t1506-rev-parse-diagnosis.sh | 5 +++++\n 2 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 0bfa29dbbf..675e0a759e 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1080,6 +1080,9 @@ static enum get_oid_result get_parent(struct repository *r,\n \treturn MISSING_OBJECT;\n }\n \n+static int get_nth_ancestor_max_depth = 4096;\n+static int get_nth_ancestor_curr_depth;\n+\n static enum get_oid_result get_nth_ancestor(struct repository *r,\n \t\t\t\t\t const char *name, int len,\n \t\t\t\t\t struct object_id *result,\n@@ -1089,20 +1092,31 @@ static enum get_oid_result get_nth_ancestor(struct repository *r,\n \tstruct commit *commit;\n \tint ret;\n \n+\tif (++get_nth_ancestor_curr_depth > get_nth_ancestor_max_depth)\n+\t\t return error(_(\"exceeded maximum ancestor depth\"));\n+\n \tret = get_oid_1(r, name, len, &oid, GET_OID_COMMITTISH);\n \tif (ret)\n-\t\treturn ret;\n+\t\tgoto done;\n \tcommit = lookup_commit_reference(r, &oid);\n-\tif (!commit)\n-\t\treturn MISSING_OBJECT;\n+\tif (!commit) {\n+\t\tret = MISSING_OBJECT;\n+\t\tgoto done;\n+\t}\n \n \twhile (generation--) {\n-\t\tif (repo_parse_commit(r, commit) || !commit->parents)\n-\t\t\treturn MISSING_OBJECT;\n+\t\tif (repo_parse_commit(r, commit) || !commit->parents) {\n+\t\t\tret = MISSING_OBJECT;\n+\t\t\tgoto done;\n+\t\t}\n \t\tcommit = commit->parents->item;\n \t}\n \toidcpy(result, &commit->object.oid);\n-\treturn FOUND;\n+\n+\tret = FOUND;\n+done:\n+\tget_nth_ancestor_curr_depth--;\n+\treturn ret;\n }\n \n struct object *repo_peel_to_type(struct repository *r, const char *name, int namelen,\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex ef40511d89..b3b9f6c8c5 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -244,6 +244,11 @@ test_expect_success 'reject Nth ancestor if N is too high' '\n \ttest_must_fail git rev-parse HEAD~100000000000000000000000000000000\n '\n \n+test_expect_success 'reject too-deep recursive ancestor queries' '\n+\ttest_must_fail git rev-parse \"HEAD$(perl -e \"print \\\"~\\\" x 4097\")\" 2>err &&\n+\tgrep \"error: exceeded maximum ancestor depth\" err\n+'\n+\n test_expect_success 'pathspecs with wildcards are not ambiguous' '\n \techo \"*.c\" >expect &&\n \tgit rev-parse \"*.c\" >actual &&\n\nbase-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169\n-- \n2.43.0.rc2.19.geadd45bf00\n"},{"id":"485019","messageId":"xmqqjzqcl53p.fsf@gitster.g","threadId":"60536","inReplyTo":"57c0b30ddfe7c0ae78069682ff8454791e54469f.1700496801.git.me@ttaylorr.com","subject":"Re: [PATCH] object-name: reject too-deep recursive ancestor queries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-20T17:14:18Z","receivedAt":"2023-11-20T17:14:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Since this is a local-only exploit, a user would have to be tricked into\n> running such a query by an adversary. Even if they were successfully\n> tricked into running the malicious query, the blast radius is limited to\n> a local stack overflow, which does not have meaningful paths to remote\n> code execution, arbitrary memory reads, or any more grave security\n> concerns.\n> ...\n\nSo the difference in practice is if we make a controlled call to\ndie() or just let it crash? It still does sound worthwhile thing to\ndo to make sure we make a controlled death. But ...\n\n> +static int get_nth_ancestor_max_depth = 4096;\n> +static int get_nth_ancestor_curr_depth;\n\n... do we have a lock at a much higher level that prevents multiple\nname-to-oid look-ups from running simultaneously, or something\nsimilar, to make use of this static counter safe? I am not offhand\nsure how safe it is to assume that we'd always be single-threaded.\nThis variable leaves a bad taste in my mouth.\n\nI am not offhand sure how hard it is to count the depth per\ncallpath; get_oid_1() is the sole caller of get_nth_ancestor(), so\nif you rename the former into a separate helper with a new\n\"recursion_depth\" parameter, create a thin wrapper around it that\nstarts the recursion at depth 0 and have everybody else (i.e.,\npeel_onion() and get_oid_with_context_1()) call it, and have\nget_nth_ancestor increment (and die as needed) the counter, would\nthat be sufficient to ensure that we count the depth per call\ninvocation?\n\nThanks.\n"},{"id":"485065","messageId":"ZV9Za7iCL6WiE-Py@tanuki","threadId":"60536","inReplyTo":"57c0b30ddfe7c0ae78069682ff8454791e54469f.1700496801.git.me@ttaylorr.com","subject":"Re: [PATCH] object-name: reject too-deep recursive ancestor queries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-23T13:53:47Z","receivedAt":"2023-11-23T13:53:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Nov 20, 2023 at 11:13:45AM -0500, Taylor Blau wrote:\n> When trying to resolve a revision query like \"HEAD~~~~~\", our call\n> pattern looks something like:\n> \n> - object-name.c::get_oid_with_context()\n> - object-name.c::get_oid_1()\n> - object-name.c::get_nth_ancestor()\n> - object-name.c::get_oid_1()\n> - ...\n> \n> With `get_nth_ancestor()` and `get_oid_1()` mutually recurring, popping\n> one '~' off of the revision query for each round of the recursion.\n\nOne thing I noticed just now is that we have exactly the same problem\nwith `^`, just with a different callstack. This problem isn't yet\naddressed by your patch.\n\nI have to wonder whether we should tighten restrictions even further:\ninstead of manually keeping track of how deep in the stack we are, we\nlimit the length of revisions to at most 1MB. I would claim that this\nlimit is sufficiently large to never be a problem in practice. Revisions\nare limited to 4kB on most platforms anyway due to the maximum path\nlength. And if somebody really wants to request the (1024 * 1024) + 1th\nparent, they shouldn't do that by appending this many \"~\" or \"^\" chars,\nbut instead they should ask for \"~1048577\" or \"^1048577\".\n\nI realize that this is much more restrictive than the current patch. But\nit would be a good defensive mechanism against all kinds of weird revs,\nand I am very certain that there are other ways to blow the stack or\ncause out-of-bounds reads or writes here.\n\nPatrick\n\n> Since this recursive behavior is unbounded, having too many \"~\"'s\n> contained in a revision query will cause us to blow the stack.\n> Generating a message like this when compiled under SANITIZE=address:\n> \n> $ valgrind git rev-parse \"HEAD$(perl -e \"print \\\"~\\\" x 1000000000000\")\"\n> ==597453== Memcheck, a memory error detector\n> ==597453== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.\n> ==597453== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info\n> ==597453== Command: /home/ttaylorr/local/bin/git.compile diff HEAD~~~~~~~~~~~~[...]\n> ==597453==\n> AddressSanitizer:DEADLYSIGNAL\n> =================================================================\n> ==597453==ERROR: AddressSanitizer: stack-overflow on address 0x7fffdd838ff8 (pc 0x7f2726082748 bp 0x7fffdd839110 sp 0x7fffdd839000 T0)\n> #0 0x7f2726082748 in __asan::GetTLSFakeStack() ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:176\n> #1 0x7f2726082748 in GetFakeStackFast ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:193\n> #2 0x7f27260833de in OnMalloc ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:207\n> #3 0x7f27260833de in __asan_stack_malloc_1 ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:256\n> #4 0x563f9077d9d8 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1087\n> #5 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295\n> #6 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092\n> #7 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295\n> #8 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092\n> [...]\n> #247 0x563f9077e957 in get_oid_1 /home/ttaylorr/src/git/object-name.c:1295\n> #248 0x563f9077da64 in get_nth_ancestor /home/ttaylorr/src/git/object-name.c:1092\n> \n> SUMMARY: AddressSanitizer: stack-overflow ../../../../src/libsanitizer/asan/asan_fake_stack.cpp:176 in __asan::GetTLSFakeStack()\n> ==597453==ABORTING\n> \n> (Note that the actual stack is much deeper. GDB reports that the bottom\n> of the stack looks something like the following):\n> \n> #54866 0x0000555555c6d3bf in get_oid_with_context_1 (repo=0x5555563849a0 , name=0x7fffffff4be5 \"HEAD\", '~' ..., flags=128, prefix=0x0, oid=0x7ffff5713d40, oc=0x7ffff5713d90) at object-name.c:1947\n> #54867 0x0000555555c6e2fa in get_oid_with_context (repo=0x5555563849a0 , str=0x7fffffff4be5 \"HEAD\", '~' ..., flags=128, oid=0x7ffff5713d40, oc=0x7ffff5713d90) at object-name.c:2096\n> #54868 0x0000555555d8eed8 in handle_revision_arg_1 (arg_=0x7fffffff4be5 \"HEAD\", '~' ..., revs=0x7ffff5b000d0, flags=0, revarg_opt=0) at revision.c:2174\n> #54869 0x0000555555d8f1a9 in handle_revision_arg (arg=0x7fffffff4be5 \"HEAD\", '~' ..., revs=0x7ffff5b000d0, flags=0, revarg_opt=0) at revision.c:2189\n> #54870 0x0000555555d97ca9 in setup_revisions (argc=2, argv=0x7fffffff4970, revs=0x7ffff5b000d0, opt=0x0) at revision.c:2932\n> #54871 0x00005555557d6a63 in cmd_diff (argc=2, argv=0x7fffffff4970, prefix=0x0) at builtin/diff.c:502\n> #54872 0x00005555557367bf in run_builtin (p=0x5555561c4c30 , argc=2, argv=0x7fffffff4970) at git.c:469\n> #54873 0x000055555573716b in handle_builtin (argc=2, argv=0x7fffffff4970) at git.c:723\n> #54874 0x000055555573785a in run_argv (argcp=0x7ffff56028b0, argv=0x7ffff56028e0) at git.c:787\n> #54875 0x0000555555738626 in cmd_main (argc=2, argv=0x7fffffff4970) at git.c:922\n> #54876 0x00005555559d3fdd in main (argc=3, argv=0x7fffffff4968) at common-main.c:62\n> \n> Fortunately, we can impose a limit on the maximum recursion depth we're\n> willing to accept when resolving queries like the above without\n> significantly impeding users. This patch sets the limit at 4096, though\n> we could probably increase that limit depending on the size of each\n> frame.\n> \n> The limit introduced here is large enough that any reasonable query\n> should still run to completion, but small enough that if the frame size\n> were to significantly increase, our protection would still be effective.\n> \n> The change here is straightforward: each call to get_nth_ancestor()\n> increases a counter, and then decrements that counter before returning.\n> \n> The diff is a little noisy since there are a handful of return paths\n> from `get_nth_ancestor()`, all of which need to decrement the depth\n> variable.\n> \n> Since this is a local-only exploit, a user would have to be tricked into\n> running such a query by an adversary. Even if they were successfully\n> tricked into running the malicious query, the blast radius is limited to\n> a local stack overflow, which does not have meaningful paths to remote\n> code execution, arbitrary memory reads, or any more grave security\n> concerns.\n> \n> Reported-by: Carlos Andrés Ramírez Cataño \n> Signed-off-by: Taylor Blau \n> ---\n> object-name.c | 26 ++++++++++++++++++++------\n> t/t1506-rev-parse-diagnosis.sh | 5 +++++\n> 2 files changed, 25 insertions(+), 6 deletions(-)\n> \n> diff --git a/object-name.c b/object-name.c\n> index 0bfa29dbbf..675e0a759e 100644\n> --- a/object-name.c\n> +++ b/object-name.c\n> @@ -1080,6 +1080,9 @@ static enum get_oid_result get_parent(struct repository *r,\n> \treturn MISSING_OBJECT;\n> }\n> \n> +static int get_nth_ancestor_max_depth = 4096;\n> +static int get_nth_ancestor_curr_depth;\n> +\n> static enum get_oid_result get_nth_ancestor(struct repository *r,\n> \t\t\t\t\t const char *name, int len,\n> \t\t\t\t\t struct object_id *result,\n> @@ -1089,20 +1092,31 @@ static enum get_oid_result get_nth_ancestor(struct repository *r,\n> \tstruct commit *commit;\n> \tint ret;\n> \n> +\tif (++get_nth_ancestor_curr_depth > get_nth_ancestor_max_depth)\n> +\t\t return error(_(\"exceeded maximum ancestor depth\"));\n> +\n> \tret = get_oid_1(r, name, len, &oid, GET_OID_COMMITTISH);\n> \tif (ret)\n> -\t\treturn ret;\n> +\t\tgoto done;\n> \tcommit = lookup_commit_reference(r, &oid);\n> -\tif (!commit)\n> -\t\treturn MISSING_OBJECT;\n> +\tif (!commit) {\n> +\t\tret = MISSING_OBJECT;\n> +\t\tgoto done;\n> +\t}\n> \n> \twhile (generation--) {\n> -\t\tif (repo_parse_commit(r, commit) || !commit->parents)\n> -\t\t\treturn MISSING_OBJECT;\n> +\t\tif (repo_parse_commit(r, commit) || !commit->parents) {\n> +\t\t\tret = MISSING_OBJECT;\n> +\t\t\tgoto done;\n> +\t\t}\n> \t\tcommit = commit->parents->item;\n> \t}\n> \toidcpy(result, &commit->object.oid);\n> -\treturn FOUND;\n> +\n> +\tret = FOUND;\n> +done:\n> +\tget_nth_ancestor_curr_depth--;\n> +\treturn ret;\n> }\n> \n> struct object *repo_peel_to_type(struct repository *r, const char *name, int namelen,\n> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\n> index ef40511d89..b3b9f6c8c5 100755\n> --- a/t/t1506-rev-parse-diagnosis.sh\n> +++ b/t/t1506-rev-parse-diagnosis.sh\n> @@ -244,6 +244,11 @@ test_expect_success 'reject Nth ancestor if N is too high' '\n> \ttest_must_fail git rev-parse HEAD~100000000000000000000000000000000\n> '\n> \n> +test_expect_success 'reject too-deep recursive ancestor queries' '\n> +\ttest_must_fail git rev-parse \"HEAD$(perl -e \"print \\\"~\\\" x 4097\")\" 2>err &&\n> +\tgrep \"error: exceeded maximum ancestor depth\" err\n> +'\n> +\n> test_expect_success 'pathspecs with wildcards are not ambiguous' '\n> \techo \"*.c\" >expect &&\n> \tgit rev-parse \"*.c\" >actual &&\n> \n> base-commit: cfb8a6e9a93adbe81efca66e6110c9b4d2e57169\n> -- \n> 2.43.0.rc2.19.geadd45bf00\n"},{"id":"485102","messageId":"xmqqy1en7af2.fsf@gitster.g","threadId":"60536","inReplyTo":"ZV9Za7iCL6WiE-Py@tanuki","subject":"Re: [PATCH] object-name: reject too-deep recursive ancestor queries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-24T09:44:33Z","receivedAt":"2023-11-24T09:44:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I have to wonder whether we should tighten restrictions even further:\n> instead of manually keeping track of how deep in the stack we are, we\n> limit the length of revisions to at most 1MB. I would claim that this\n> limit is sufficiently large to never be a problem in practice.\n\nTempting.\n\n> Revisions\n> are limited to 4kB on most platforms anyway due to the maximum path\n> length.\n\nI do not quite get this part, though.\n\nWhen we get \"HEAD~~~~~~~~~^2~~~~~~\" from the user, do we somehow try\nto create a file or a directory with that name and fail due to\nENAMETOOLONG?\n\nThere are ways like \"git rev-list --stdin\" to cause Git read input\nlines of arbitrary length, so I do not think the command line length\nlimit does not come into the picture, either.\n\nBut I do agree that the only useful use of such a revision string\nthat is longer than 1MB would be to attack.\n\n"},{"id":"485104","messageId":"ZWB26TH0CFW1KC4L@tanuki","threadId":"60536","inReplyTo":"xmqqy1en7af2.fsf@gitster.g","subject":"Re: [PATCH] object-name: reject too-deep recursive ancestor queries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-24T10:11:53Z","receivedAt":"2023-11-24T10:12:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 24, 2023 at 06:44:33PM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > I have to wonder whether we should tighten restrictions even further:\n> > instead of manually keeping track of how deep in the stack we are, we\n> > limit the length of revisions to at most 1MB. I would claim that this\n> > limit is sufficiently large to never be a problem in practice.\n> \n> Tempting.\n> \n> > Revisions\n> > are limited to 4kB on most platforms anyway due to the maximum path\n> > length.\n> \n> I do not quite get this part, though.\n> \n> When we get \"HEAD~~~~~~~~~^2~~~~~~\" from the user, do we somehow try\n> to create a file or a directory with that name and fail due to\n> ENAMETOOLONG?\n\nSorry, this was a typo on my part. I didn't mean \"revision\", I meant\n\"reference\" here. References are limited to at most 4kB on most\nplatforms due to filesystem limitations, whereas revisions currently\nhave no limits in place.\n\nPatrick\n\n> There are ways like \"git rev-list --stdin\" to cause Git read input\n> lines of arbitrary length, so I do not think the command line length\n> limit does not come into the picture, either.\n> \n> But I do agree that the only useful use of such a revision string\n> that is longer than 1MB would be to attack.\n> \n"},{"id":"485392","messageId":"20231206194035.GB103708@coredump.intra.peff.net","threadId":"60536","inReplyTo":"ZWB26TH0CFW1KC4L@tanuki","subject":"Re: [PATCH] object-name: reject too-deep recursive ancestor queries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-12-06T19:40:35Z","receivedAt":"2023-12-06T19:40:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 24, 2023 at 11:11:53AM +0100, Patrick Steinhardt wrote:\n\n> > When we get \"HEAD~~~~~~~~~^2~~~~~~\" from the user, do we somehow try\n> > to create a file or a directory with that name and fail due to\n> > ENAMETOOLONG?\n> \n> Sorry, this was a typo on my part. I didn't mean \"revision\", I meant\n> \"reference\" here. References are limited to at most 4kB on most\n> platforms due to filesystem limitations, whereas revisions currently\n> have no limits in place.\n\nEven without filesystem limitations, references are effectively limited\nto 64kb due to the pkt-line format.\n\nRevisions can be much longer than a reference, though. We accept\n\"some_ref:some/path/in/tree\", for instance[1]. I think you could argue\nthat paths are likewise limited by the filesystem, though. Even on\nsystems like Linux where paths can grow arbitrarily long (by descending\nand adding to the current directory), you're still limited in specifying\na full pathname. And Git will always use the full path from the project\nroot when creating worktree entries. Plus my recent tree-depth patches\neffectively limit us to 16MB in the default config.\n\nSo I think it might be reasonable to limit revision lengths just as a\nbelt-and-suspenders against overflow attacks, etc. But I suspect that\nthe limits we'd choose there might not match what we'd want for\nprotection against stack exhaustion via recursion. E.g., I think 8k is\nprobably the minimum I'd want for a revision (\"my/4k/ref:my/4k/path\").\nIf one \"~\" character can create an expensive recursion, that might be\ntoo much.\n\nSo we probably need something like Taylor's patch anyway (or to switch\nto an iterative algorithm, though that might be tricky because of the\nway we parse). I agree it needs to handle \"^\", though.\n\n-Peff\n\n[1] There are other more exotic revisions, too. The most arbitrary-sized\n that comes to mind is \":/some-string-to-match\". I doubt anybody\n would be too mad if that were limited to 8k or even 4k, though.\n"},{"id":"485412","messageId":"ZXFroM6x5xY8fszx@tanuki","threadId":"60536","inReplyTo":"20231206194035.GB103708@coredump.intra.peff.net","subject":"Re: [PATCH] object-name: reject too-deep recursive ancestor queries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-07T06:52:16Z","receivedAt":"2023-12-07T06:52:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 06, 2023 at 02:40:35PM -0500, Jeff King wrote:\n> On Fri, Nov 24, 2023 at 11:11:53AM +0100, Patrick Steinhardt wrote:\n> \n> > > When we get \"HEAD~~~~~~~~~^2~~~~~~\" from the user, do we somehow try\n> > > to create a file or a directory with that name and fail due to\n> > > ENAMETOOLONG?\n> > \n> > Sorry, this was a typo on my part. I didn't mean \"revision\", I meant\n> > \"reference\" here. References are limited to at most 4kB on most\n> > platforms due to filesystem limitations, whereas revisions currently\n> > have no limits in place.\n> \n> Even without filesystem limitations, references are effectively limited\n> to 64kb due to the pkt-line format.\n> \n> Revisions can be much longer than a reference, though. We accept\n> \"some_ref:some/path/in/tree\", for instance[1]. I think you could argue\n> that paths are likewise limited by the filesystem, though. Even on\n> systems like Linux where paths can grow arbitrarily long (by descending\n> and adding to the current directory), you're still limited in specifying\n> a full pathname. And Git will always use the full path from the project\n> root when creating worktree entries. Plus my recent tree-depth patches\n> effectively limit us to 16MB in the default config.\n\nI was only able to trigger these issues with _really_ long revisions,\nlike hundreds of megabytes. But that's on a glibc system, other systems\nbased on e.g. musl libc have a much smaller stack by default where these\nlimits would be hit sooner.\n\n> So I think it might be reasonable to limit revision lengths just as a\n> belt-and-suspenders against overflow attacks, etc. But I suspect that\n> the limits we'd choose there might not match what we'd want for\n> protection against stack exhaustion via recursion. E.g., I think 8k is\n> probably the minimum I'd want for a revision (\"my/4k/ref:my/4k/path\").\n> If one \"~\" character can create an expensive recursion, that might be\n> too much.\n\nFair enough. I think combining the two approaches would be sensible as a\ndefense-in-depth approach.\n\nPatrick\n\n> So we probably need something like Taylor's patch anyway (or to switch\n> to an iterative algorithm, though that might be tricky because of the\n> way we parse). I agree it needs to handle \"^\", though.\n> \n> -Peff\n> \n> [1] There are other more exotic revisions, too. The most arbitrary-sized\n> that comes to mind is \":/some-string-to-match\". I doubt anybody\n> would be too mad if that were limited to 8k or even 4k, though.\n"}]}