| Message ID | 20200420224310.9989-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | osxkeychain: restrict queries to requests with a valid host | expand |
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;
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
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?
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
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;
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(+)