osxkeychain: restrict queries to requests with a valid host
diff mbox series

Message ID 20200420224310.9989-1-carenas@gmail.com
State New
Headers show
Series
  • osxkeychain: restrict queries to requests with a valid host
Related show

Commit Message

Carlo Marcelo Arenas Belón April 20, 2020, 10:43 p.m. UTC
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(+)

Comments

Junio C Hamano April 20, 2020, 11:09 p.m. UTC | #1
Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> 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(+)
>
> 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
> +	 */

Just to clarify, you do not need this patch to "fix" it, as long as
you are running up-to-date Git, right?  In other words, this is more
like a belt-and-suspender protection, isn't it?

> +	if (!host || !*host)
> +		return;
> +
>  	if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))
>  		return;
Carlo Marcelo Arenas Belón April 20, 2020, 11:20 p.m. UTC | #2
On Mon, Apr 20, 2020 at 4:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Just to clarify, you do not need this patch to "fix" it, as long as
> you are running up-to-date Git, right?  In other words, this is more
> like a belt-and-suspender protection, isn't it?

the fixes in master do most of the work, but the way the underlying
macOS function
used works, will still randomly select a credential for cases where host=""

Carlo
Junio C Hamano April 21, 2020, 1:44 a.m. UTC | #3
Carlo Arenas <carenas@gmail.com> writes:

> On Mon, Apr 20, 2020 at 4:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Just to clarify, you do not need this patch to "fix" it, as long as
>> you are running up-to-date Git, right?  In other words, this is more
>> like a belt-and-suspender protection, isn't it?
>
> the fixes in master do most of the work, but the way the underlying
> macOS function
> used works, will still randomly select a credential for cases where host=""

That is like saying "most of the work but as a protection it does
not work at all and still allows a random stuff to be chosen", no?
Jonathan Nieder April 21, 2020, 6:15 a.m. UTC | #4
Hi!

Carlo Marcelo Arenas Belón wrote:

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

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

Patch
diff mbox series

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;
+
 	if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))
 		return;