{"thread":{"id":"53233","subject":"[PATCH] credential: treat \"?\" and \"#\" in URLs as end of host","startedAt":"2020-04-14T21:43:28Z","lastAt":"2020-04-15T19:17:45Z","messageCount":3,"participants":["Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"395452","messageId":"20200414214304.GA1887601@coredump.intra.peff.net","threadId":"53233","inReplyTo":null,"subject":"[PATCH] credential: treat \"?\" and \"#\" in URLs as end of host","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-14T21:43:04Z","receivedAt":"2020-04-14T21:43:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's unusual to see:\n\n  https://example.com?query-parameters\n\nwithout an intervening slash, like:\n\n  https://example.com/some-path?query-parameters\n\nor even:\n\n  https://example.com/?query-parameters\n\nbut it is a valid end to the hostname (actually \"authority component\")\naccording to RFC 3986. Likewise for \"#\".\n\nAnd curl will parse the URL according to the standard, meaning it will\ncontact example.com, but our credential code would ask about a bogus\nhostname with a \"?\" in it. Let's make sure we follow the standard, and\nmore importantly ask about the same hosts that curl will be talking to.\n\nIt would be nice if we could just ask curl to parse the URL for us. But\nit didn't grow a URL-parsing API until 7.62, so we'd be stuck with\nfallback code either way. Plus we'd need this code in the main Git\nbinary, where we've tried to avoid having a link dependency on libcurl.\n\nBut let's at least fix our parser. Moving to curl's parser would prevent\nother potential discrepancies, but this gives us immediate relief for\nthe known problem, and would help our fallback code if we eventually use\ncurl.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nJust a follow-on to today's release. This isn't security critical after\nthe earlier fix, but it made some of the attack vectors much easier.\n\n credential.c           |  9 ++++++++-\n t/t0300-credentials.sh | 36 +++++++++++++++++++++++++++++++++++-\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 21b3ba152f..8aa9777548 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -388,7 +388,14 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \tcp = proto_end + 3;\n \tat = strchr(cp, '@');\n \tcolon = strchr(cp, ':');\n-\tslash = strchrnul(cp, '/');\n+\n+\t/*\n+\t * A query or fragment marker before the slash ends the host portion.\n+\t * We'll just continue to call this \"slash\" for simplicity. Notably our\n+\t * \"trim leading slashes\" part won't skip over this part of the path,\n+\t * but that's what we'd want.\n+\t */\n+\tslash = cp + strcspn(cp, \"/?#\");\n \n \tif (!at || slash <= at) {\n \t\t/* Case (1) */\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 5b78ebbc3f..b6ec676989 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -443,11 +443,45 @@ test_expect_success 'url parser ignores embedded newlines' '\n \tusername=askpass-username\n \tpassword=askpass-password\n \t--\n-\twarning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/\n+\twarning: url contains a newline in its path component: https://one.example.com?%0ahost=two.example.com/\n \twarning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/\n \taskpass: Username:\n \taskpass: Password:\n \tEOF\n '\n \n+# usage: check_host_and_path <url> <expected-host> <expected-path>\n+check_host_and_path () {\n+\t# we always parse the path component, but we need this to make sure it\n+\t# is passed to the helper\n+\ttest_config credential.useHTTPPath true &&\n+\tcheck fill \"verbatim user pass\" <<-EOF\n+\turl=$1\n+\t--\n+\tprotocol=https\n+\thost=$2\n+\tpath=$3\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tverbatim: get\n+\tverbatim: protocol=https\n+\tverbatim: host=$2\n+\tverbatim: path=$3\n+\tEOF\n+}\n+\n+test_expect_success 'url parser handles bare query marker' '\n+\tcheck_host_and_path https://example.com?foo.git example.com ?foo.git\n+'\n+\n+test_expect_success 'url parser handles bare fragment marker' '\n+\tcheck_host_and_path https://example.com#foo.git example.com \"#foo.git\"\n+'\n+\n+test_expect_success 'url parser not confused by encoded markers' '\n+\tcheck_host_and_path https://example.com%23%3f%2f/foo.git \\\n+\t\t\"example.com#?/\" foo.git\n+'\n+\n test_done\n-- \n2.26.1.429.g609150846d\n"},{"id":"395458","messageId":"20200415001337.GA7457@syl.local","threadId":"53233","inReplyTo":"20200414214304.GA1887601@coredump.intra.peff.net","subject":"Re: [PATCH] credential: treat \"?\" and \"#\" in URLs as end of host","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-15T00:13:37Z","receivedAt":"2020-04-15T00:14:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Tue, Apr 14, 2020 at 05:43:04PM -0400, Jeff King wrote:\n> It's unusual to see:\n>\n>   https://example.com?query-parameters\n>\n> without an intervening slash, like:\n>\n>   https://example.com/some-path?query-parameters\n>\n> or even:\n>\n>   https://example.com/?query-parameters\n>\n> but it is a valid end to the hostname (actually \"authority component\")\n> according to RFC 3986. Likewise for \"#\".\n>\n> And curl will parse the URL according to the standard, meaning it will\n> contact example.com, but our credential code would ask about a bogus\n> hostname with a \"?\" in it. Let's make sure we follow the standard, and\n> more importantly ask about the same hosts that curl will be talking to.\n>\n> It would be nice if we could just ask curl to parse the URL for us. But\n> it didn't grow a URL-parsing API until 7.62, so we'd be stuck with\n> fallback code either way. Plus we'd need this code in the main Git\n> binary, where we've tried to avoid having a link dependency on libcurl.\n>\n> But let's at least fix our parser. Moving to curl's parser would prevent\n> other potential discrepancies, but this gives us immediate relief for\n> the known problem, and would help our fallback code if we eventually use\n> curl.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nAll makes sense to me. I agree that it would probably be preferable if\nwe could just use cURL's own parser and forget about this code entirely.\nBut, having it still be a fallback knocks out any benefit that we'd be\ngetting by relying on their parser rather than our own.\n\n> ---\n> Just a follow-on to today's release. This isn't security critical after\n> the earlier fix, but it made some of the attack vectors much easier.\n\nYep, thanks for noting.\n\n>  credential.c           |  9 ++++++++-\n>  t/t0300-credentials.sh | 36 +++++++++++++++++++++++++++++++++++-\n>  2 files changed, 43 insertions(+), 2 deletions(-)\n>\n> diff --git a/credential.c b/credential.c\n> index 21b3ba152f..8aa9777548 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -388,7 +388,14 @@ int credential_from_url_gently(struct credential *c, const char *url,\n>  \tcp = proto_end + 3;\n>  \tat = strchr(cp, '@');\n>  \tcolon = strchr(cp, ':');\n> -\tslash = strchrnul(cp, '/');\n> +\n> +\t/*\n> +\t * A query or fragment marker before the slash ends the host portion.\n> +\t * We'll just continue to call this \"slash\" for simplicity. Notably our\n> +\t * \"trim leading slashes\" part won't skip over this part of the path,\n> +\t * but that's what we'd want.\n> +\t */\n> +\tslash = cp + strcspn(cp, \"/?#\");\n>\n>  \tif (!at || slash <= at) {\n>  \t\t/* Case (1) */\n> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> index 5b78ebbc3f..b6ec676989 100755\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -443,11 +443,45 @@ test_expect_success 'url parser ignores embedded newlines' '\n>  \tusername=askpass-username\n>  \tpassword=askpass-password\n>  \t--\n> -\twarning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/\n> +\twarning: url contains a newline in its path component: https://one.example.com?%0ahost=two.example.com/\n>  \twarning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/\n>  \taskpass: Username:\n>  \taskpass: Password:\n>  \tEOF\n>  '\n>\n> +# usage: check_host_and_path <url> <expected-host> <expected-path>\n> +check_host_and_path () {\n> +\t# we always parse the path component, but we need this to make sure it\n> +\t# is passed to the helper\n> +\ttest_config credential.useHTTPPath true &&\n> +\tcheck fill \"verbatim user pass\" <<-EOF\n> +\turl=$1\n> +\t--\n> +\tprotocol=https\n> +\thost=$2\n> +\tpath=$3\n> +\tusername=user\n> +\tpassword=pass\n> +\t--\n> +\tverbatim: get\n> +\tverbatim: protocol=https\n> +\tverbatim: host=$2\n> +\tverbatim: path=$3\n> +\tEOF\n> +}\n> +\n> +test_expect_success 'url parser handles bare query marker' '\n> +\tcheck_host_and_path https://example.com?foo.git example.com ?foo.git\n> +'\n> +\n> +test_expect_success 'url parser handles bare fragment marker' '\n> +\tcheck_host_and_path https://example.com#foo.git example.com \"#foo.git\"\n> +'\n> +\n> +test_expect_success 'url parser not confused by encoded markers' '\n> +\tcheck_host_and_path https://example.com%23%3f%2f/foo.git \\\n> +\t\t\"example.com#?/\" foo.git\n> +'\n> +\n>  test_done\n\nThese look good, too. Thanks for working on this.\n\n> --\n> 2.26.1.429.g609150846d\n\nThanks,\nTaylor\n"},{"id":"395524","messageId":"xmqqv9m061aj.fsf@gitster.c.googlers.com","threadId":"53233","inReplyTo":"20200414214304.GA1887601@coredump.intra.peff.net","subject":"Re: [PATCH] credential: treat \"?\" and \"#\" in URLs as end of host","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-15T18:39:48Z","receivedAt":"2020-04-15T19:17:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> -\tslash = strchrnul(cp, '/');\n> +\n> +\t/*\n> +\t * A query or fragment marker before the slash ends the host portion.\n> +\t * We'll just continue to call this \"slash\" for simplicity. Notably our\n> +\t * \"trim leading slashes\" part won't skip over this part of the path,\n> +\t * but that's what we'd want.\n> +\t */\n> +\tslash = cp + strcspn(cp, \"/?#\");\n\nWhatever happened when the original did not find any slash should\nhappen in the new code when none of these three terminating bytes\nis found.  The original made slash point at the end of the string,\nand the new code does the same.\n\nSo, by definition, the code is correct ;-)\n\n> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> index 5b78ebbc3f..b6ec676989 100755\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -443,11 +443,45 @@ test_expect_success 'url parser ignores embedded newlines' '\n>  \tusername=askpass-username\n>  \tpassword=askpass-password\n>  \t--\n> -\twarning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/\n> +\twarning: url contains a newline in its path component: https://one.example.com?%0ahost=two.example.com/\n\nNice demonstration ;-).\n\n"}]}