Re: [PATCH] object-name: reject too-deep recursive ancestor queries

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2023, 17:14 UTC
Message-ID: <xmqqjzqcl53p.fsf@gitster.g>
In-Reply-To: <57c0b30ddfe7c0ae78069682ff8454791e54469f.1700496801.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 7 quoted lines

> Since this is a local-only exploit, a user would have to be tricked into
> running such a query by an adversary. Even if they were successfully
> tricked into running the malicious query, the blast radius is limited to
> a local stack overflow, which does not have meaningful paths to remote
> code execution, arbitrary memory reads, or any more grave security
> concerns.
> ...

So the difference in practice is if we make a controlled call to die() or just let it crash? It still does sound worthwhile thing to do to make sure we make a controlled death. But ...

> +static int get_nth_ancestor_max_depth = 4096;
> +static int get_nth_ancestor_curr_depth;

... do we have a lock at a much higher level that prevents multiple name-to-oid look-ups from running simultaneously, or something similar, to make use of this static counter safe? I am not offhand sure how safe it is to assume that we'd always be single-threaded. This variable leaves a bad taste in my mouth.

I am not offhand sure how hard it is to count the depth per callpath; get_oid_1() is the sole caller of get_nth_ancestor(), so if you rename the former into a separate helper with a new "recursion_depth" parameter, create a thin wrapper around it that starts the recursion at depth 0 and have everybody else (i.e., peel_onion() and get_oid_with_context_1()) call it, and have get_nth_ancestor increment (and die as needed) the counter, would that be sufficient to ensure that we count the depth per call invocation?

Thanks.