{"thread":{"id":"53266","subject":"[PATCH] osxkeychain: restrict queries to requests with a valid host","startedAt":"2020-04-20T22:43:22Z","lastAt":"2024-04-23T21:39:24Z","messageCount":7,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Carlo Arenas","Jonathan Nieder","Calvin Wan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"395775","messageId":"20200420224310.9989-1-carenas@gmail.com","threadId":"53266","inReplyTo":null,"subject":"[PATCH] osxkeychain: restrict queries to requests with a valid host","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-20T22:43:10Z","receivedAt":"2020-04-20T22:43:22Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"make sure that requests to this helper to get credentials return early if\nthere is no host ord the host is empty.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex bcd3f575a3..2264a88c41 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -69,6 +69,12 @@ static void find_internet_password(void)\n \tUInt32 len;\n \tSecKeychainItemRef item;\n \n+\t/*\n+\t * Require at valid host to fix CVE-2020-11008\n+\t */\n+\tif (!host || !*host)\n+\t\treturn;\n+\n \tif (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))\n \t\treturn;\n \n-- \n2.26.2.111.gbff22aa583\n\n"},{"id":"395782","messageId":"xmqqa735rbyh.fsf@gitster.c.googlers.com","threadId":"53266","inReplyTo":"20200420224310.9989-1-carenas@gmail.com","subject":"Re: [PATCH] osxkeychain: restrict queries to requests with a valid host","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-20T23:09:42Z","receivedAt":"2020-04-20T23:09:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> make sure that requests to this helper to get credentials return early if\n> there is no host ord the host is empty.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index bcd3f575a3..2264a88c41 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -69,6 +69,12 @@ static void find_internet_password(void)\n> \tUInt32 len;\n> \tSecKeychainItemRef item;\n> \n> +\t/*\n> +\t * Require at valid host to fix CVE-2020-11008\n> +\t */\n\nJust to clarify, you do not need this patch to \"fix\" it, as long as\nyou are running up-to-date Git, right? In other words, this is more\nlike a belt-and-suspender protection, isn't it?\n\n> +\tif (!host || !*host)\n> +\t\treturn;\n> +\n> \tif (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))\n> \t\treturn;\n"},{"id":"395783","messageId":"CAPUEspiBqCGTOfEESAqY=JR5jHhARSAODjsg5rGLxWEqzFH0zw@mail.gmail.com","threadId":"53266","inReplyTo":"xmqqa735rbyh.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] osxkeychain: restrict queries to requests with a valid host","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2020-04-20T23:20:55Z","receivedAt":"2020-04-20T23:21:09Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Apr 20, 2020 at 4:09 PM Junio C Hamano wrote:\n>\n> Just to clarify, you do not need this patch to \"fix\" it, as long as\n> you are running up-to-date Git, right? In other words, this is more\n> like a belt-and-suspender protection, isn't it?\n\nthe fixes in master do most of the work, but the way the underlying\nmacOS function\nused works, will still randomly select a credential for cases where host=\"\"\n\nCarlo\n"},{"id":"395803","messageId":"xmqqd081pq79.fsf@gitster.c.googlers.com","threadId":"53266","inReplyTo":"CAPUEspiBqCGTOfEESAqY=JR5jHhARSAODjsg5rGLxWEqzFH0zw@mail.gmail.com","subject":"Re: [PATCH] osxkeychain: restrict queries to requests with a valid host","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-21T01:44:58Z","receivedAt":"2020-04-21T01:45:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> On Mon, Apr 20, 2020 at 4:09 PM Junio C Hamano wrote:\n>>\n>> Just to clarify, you do not need this patch to \"fix\" it, as long as\n>> you are running up-to-date Git, right? In other words, this is more\n>> like a belt-and-suspender protection, isn't it?\n>\n> the fixes in master do most of the work, but the way the underlying\n> macOS function\n> used works, will still randomly select a credential for cases where host=\"\"\n\nThat is like saying \"most of the work but as a protection it does\nnot work at all and still allows a random stuff to be chosen\", no?\n"},{"id":"395810","messageId":"20200421061556.GD96152@google.com","threadId":"53266","inReplyTo":"20200420224310.9989-1-carenas@gmail.com","subject":"Re: [PATCH] osxkeychain: restrict queries to requests with a valid host","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-21T06:15:56Z","receivedAt":"2020-04-21T06:16:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi!\n\nCarlo Marcelo Arenas Belón wrote:\n\n> make sure that requests to this helper to get credentials return early if\n> there is no host ord the host is empty.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 6 ++++++\n> 1 file changed, 6 insertions(+)\n\nWe had mentioned while preparing v2.26.2 that after that release\nhardening the git side of the credential helper protocol, we should\nharden the helper side. Thanks for getting it started.\n\n[...]\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index bcd3f575a3..2264a88c41 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -69,6 +69,12 @@ static void find_internet_password(void)\n> \tUInt32 len;\n> \tSecKeychainItemRef item;\n> \n> +\t/*\n> +\t * Require at valid host to fix CVE-2020-11008\n> +\t */\n> +\tif (!host || !*host)\n> +\t\treturn;\n\nWhile we're here, is there any validation we should do for any of the\nother parameters to SecKeychainFindInternetPassword (username, path,\nport, protocol)?\n\nAlso, should we check for duplicate fields as in CVE-2020-5260?\n\n> +\n> \tif (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))\n> \t\treturn;\n\nThanks,\nJonathan\n"},{"id":"493317","messageId":"20240422194824.340464-1-calvinwan@google.com","threadId":"53266","inReplyTo":"20200420224310.9989-1-carenas@gmail.com","subject":"Re: [PATCH] osxkeychain: restrict queries to requests with a valid host","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-04-22T19:48:12Z","receivedAt":"2024-04-22T19:48:30Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n> make sure that requests to this helper to get credentials return early if\n> there is no host ord the host is empty.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> contrib/credential/osxkeychain/git-credential-osxkeychain.c | 6 ++++++\n> 1 file changed, 6 insertions(+)\n> \n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> index bcd3f575a3..2264a88c41 100644\n> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -69,6 +69,12 @@ static void find_internet_password(void)\n> \tUInt32 len;\n> \tSecKeychainItemRef item;\n> \n> +\t/*\n> +\t * Require at valid host to fix CVE-2020-11008\n> +\t */\n> +\tif (!host || !*host)\n> +\t\treturn;\n> +\n> \tif (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))\n> \t\treturn;\n> \n> -- \n> 2.26.2.111.gbff22aa583\n> \n\nWe're currently using this patch downstream (removed the check for\n!*host after updates to this file), but I was wondering whether this\nchange should also be in main. It seems like the discussion around this\nstalled and there was no definitive conclusion, but the change also at\nworst does nothing and could possibly be useful -- I see other functions\nwhere we're checking for the existence of \"host\". I wasn't around when\nall the changes around this CVE were happening so I'm not exactly sure\nhow useful this patch this and whether we can get rid of it or not.\n"},{"id":"493374","messageId":"20240423213915.GA1172807@coredump.intra.peff.net","threadId":"53266","inReplyTo":"20240422194824.340464-1-calvinwan@google.com","subject":"Re: [PATCH] osxkeychain: restrict queries to requests with a valid host","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-23T21:39:15Z","receivedAt":"2024-04-23T21:39:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 22, 2024 at 07:48:12PM +0000, Calvin Wan wrote:\n\n> > diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> > index bcd3f575a3..2264a88c41 100644\n> > --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> > +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> > @@ -69,6 +69,12 @@ static void find_internet_password(void)\n> > \tUInt32 len;\n> > \tSecKeychainItemRef item;\n> > \n> > +\t/*\n> > +\t * Require at valid host to fix CVE-2020-11008\n> > +\t */\n> > +\tif (!host || !*host)\n> > +\t\treturn;\n> > +\n> > \tif (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))\n> > \t\treturn;\n> > \n> > -- \n> > 2.26.2.111.gbff22aa583\n> > \n> \n> We're currently using this patch downstream (removed the check for\n> !*host after updates to this file), but I was wondering whether this\n> change should also be in main. It seems like the discussion around this\n> stalled and there was no definitive conclusion, but the change also at\n> worst does nothing and could possibly be useful -- I see other functions\n> where we're checking for the existence of \"host\". I wasn't around when\n> all the changes around this CVE were happening so I'm not exactly sure\n> how useful this patch this and whether we can get rid of it or not.\n\nI think this is mostly redundant with the protection on the sending side\nfrom 8ba8ed568e (credential: refuse to operate when missing host or\nprotocol, 2020-04-18).\n\nLocking down the individual helpers more might be useful if they are run\nas independent programs (e.g., you could run \"osxkeychain\" manually and\nfeed it input with a host, which would make its matching quite liberal).\nBut since there isn't a way to trigger it in a normal workflow with\nuntrusted input, I don't think there's much of a security implication.\n\nLooking at the patch above, I think it may cause issues with protocols\nthat do not have a host (like \"cert\" ones we use for storing client-side\ncertificate passphrases). But I'm not sure that osxkeychain can handle\nthose anyway. I'm also not sure we didn't break that in 8ba8ed568e. That\npatch allows an empty string for the host, and the code in http.c to set\nup the credential struct for the cert uses an empty string. But doing a\ntrivial test seems broken:\n\n $ touch foo.cert\n $ git -c http.sslcert=foo.cert ls-remote ...\n fatal: refusing to work with credential missing host field\n\nCuriously if you feed it a real cert and matching key, then curl itself\nprompts for the passphrase! And if you provide the correct one, then it\ndoes not need to check credential config/helpers, and it works. If you\nprovide a bad one, we hit that same message trying to \"reject\" it (and I\nthink that is what happens with the fake cert; curl likewise complains\nand we try to erase the passphrase before trying again).\n\nBut that also means we have no opportunity to prompt/store ourselves, if\ncurl is handling it. If you set GIT_SSL_CERT_PASSWORD_PROTECTED, then we\ndo our own prompt ahead of time (and this correctly sets the hostname to\nthe empty string).\n\nSo the whole cert auth handling is kind of wonky, and it's not clear to\nme that anybody actually uses it, or what's supposed to work and what\nisn't.\n\nThat's all sort of a tangent to your question, of course. ;)\n\nIt is interesting that the fix we did in 8ba8ed568e still allows empty\nstring hostnames, as I think such a hostname would cause osxkeychain to\nstill liberally match its contents. So in that case maybe the patch\nabove is doing something? I dunno. I think the real protection is\navoiding either a NULL or empty-string hostname based on untrusted input\nin the first place.\n\n-Peff\n"}]}