Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 22, 2024, 16:15 UTC
Message-ID: <xmqqjzkpxtjm.fsf@gitster.g>
In-Reply-To: <aa4d6f508b4af3923813e19ff82a4e8484d5ff11.1713519789.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 19 quoted lines

> The `validate_headref()` function takes a path to a supposed "HEAD" file
> and checks whether its format is something that we understand. It is
> used as part of our repository discovery to check whether a specific
> directory is a Git directory or not.
>
> Part of the validation is a check for a detached HEAD that contains a
> plain object ID. To do this validation we use `get_oid_hex()`, which
> relies on `the_hash_algo`. At this point in time the hash algo cannot
> yet be initialized though because we didn't yet read the Git config.
> Consequently, it will always be the SHA1 hash algorithm.
>
> In practice this works alright because `get_oid_hex()` only ends up
> checking whether the prefix of the buffer is a valid object ID. And
> because SHA1 is shorter than SHA256, the function will successfully
> parse SHA256 object IDs, as well.
>
> It is somewhat fragile though and not really the intent to only check
> for SHA1. With this in mind, harden the code to use `get_oid_hex_any()`
> to check whether the "HEAD" file parses as any known hash.

All makes sense, and given the above, I strongly suspect that we would want to make the validate_headref() function a file-scope static in setup.c to make sure it is only called/callable from the repository discovery codepath. Especially that if somebody calls this function again after we find out that the repository uses SHA-256, we would want to let the caller know that the detached HEAD records SHA-1 and we are in an inconsistent state.

> It follows that we could just leave the current code intact, as in
> practice the code change doesn't have any user visible impact. But it
> also prepares us for `the_hash_algo` being unset when there is no
> repositroy.

Or perhaps we use get_oid_hex_any() != GIT_HASH_UNKNOWN when the_hash_algo has not been determined, and use !get_oid_hex() after we have determined which algorithm we are using? It may be what you did in a later step in the series, so let me read on.

Thanks.

Show 18 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  path.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/path.c b/path.c
> index 67229edb9c..cc02165530 100644
> --- a/path.c
> +++ b/path.c
> @@ -693,7 +693,7 @@ int validate_headref(const char *path)
>  	/*
>  	 * Is this a detached HEAD?
>  	 */
> -	if (!get_oid_hex(buffer, &oid))
> +	if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)
>  		return 0;
>  
>  	return -1;