Re: [PATCH] osxkeychain: restrict queries to requests with a valid host

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 21, 2020, 06:15 UTC
Message-ID: <20200421061556.GD96152@google.com>
In-Reply-To: <20200420224310.9989-1-carenas@gmail.com>

Hi!

Carlo Marcelo Arenas Belón wrote:

Show 7 quoted lines

> make sure that requests to this helper to get credentials return early if
> there is no host ord the host is empty.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  contrib/credential/osxkeychain/git-credential-osxkeychain.c | 6 ++++++
>  1 file changed, 6 insertions(+)

We had mentioned while preparing v2.26.2 that after that release hardening the git side of the credential helper protocol, we should harden the helper side. Thanks for getting it started.

[...]

Show 13 quoted lines

> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index bcd3f575a3..2264a88c41 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -69,6 +69,12 @@ static void find_internet_password(void)
>  	UInt32 len;
>  	SecKeychainItemRef item;
>  
> +	/*
> +	 * Require at valid host to fix CVE-2020-11008
> +	 */
> +	if (!host || !*host)
> +		return;

While we're here, is there any validation we should do for any of the other parameters to SecKeychainFindInternetPassword (username, path, port, protocol)?

Also, should we check for duplicate fields as in CVE-2020-5260?

> +
>  	if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))
>  		return;

Thanks, Jonathan