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

From: Junio C Hamano <[email-masked]>
Date: Mon, 22 Apr 2024 09:15:41 -0700

Patrick Steinhardt <[email-masked]> writes:

> 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.

> Signed-off-by: Patrick Steinhardt <[email-masked]>
> ---
>  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;