{"thread":{"id":"54952","subject":"[PATCH 0/2] disallow newlines in git:// URLs","startedAt":"2021-01-07T09:42:50Z","lastAt":"2021-01-07T09:45:00Z","messageCount":3,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"413664","messageId":"X/bXV9dGFK2bxBV0@coredump.intra.peff.net","threadId":"54952","inReplyTo":null,"subject":"[PATCH 0/2] disallow newlines in git:// URLs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-07T09:41:43Z","receivedAt":"2021-01-07T09:42:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This addresses an issue brought up by Harold Kim on the security list.\nIn general, Git handles repo paths with newlines just fine, and this\neven works over the git:// protocol. However, because of the sparseness\nof that protocol, it's easy to craft a malicious URL that makes a valid\nrequest for other protocols, like http (and submodules make it easy-ish\nto convince somebody to clone your crafted URL).\n\nSince it's unlikely that anybody is relying on having a newline in their\ngit:// repo in the first place, it's worth outlawing them to make it\nless likely for a Git client to be used as a protocol redirect.\n\n [1/2]: git_connect_git(): forbid newlines in host and path\n [2/2]: fsck: reject .gitmodules git:// urls with newlines\n\n connect.c | 2 ++\n fsck.c | 2 +-\n t/t5570-git-daemon.sh | 5 +++++\n t/t7416-submodule-dash-url.sh | 15 +++++++++++++++\n 4 files changed, 23 insertions(+), 1 deletion(-)\n\n-Peff\n"},{"id":"413665","messageId":"X/bX3jUMapnw2hYI@coredump.intra.peff.net","threadId":"54952","inReplyTo":"X/bXV9dGFK2bxBV0@coredump.intra.peff.net","subject":"[PATCH 1/2] git_connect_git(): forbid newlines in host and path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-07T09:43:58Z","receivedAt":"2021-01-07T09:44:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we connect to a git:// server, we send an initial request that\nlooks something like:\n\n 002dgit-upload-pack repo.git\\0host=example.com\n\nIf the repo path contains a newline, then it's included literally, and\nwe get:\n\n 002egit-upload-pack repo\n .git\\0host=example.com\n\nThis works fine if you really do have a newline in your repository name;\nthe server side uses the pktline framing to parse the string, not\nnewlines. However, there are many _other_ protocols in the wild that do\nparse on newlines, such as HTTP. So a carefully constructed git:// URL\ncan actually turn into a valid HTTP request. For example:\n\n git://localhost:1234/%0d%0a%0d%0aGET%20/%20HTTP/1.1 %0d%0aHost:localhost%0d%0a%0d%0a\n\nbecomes:\n\n 0050git-upload-pack /\n GET / HTTP/1.1\n Host:localhost\n\n host=localhost:1234\n\non the wire. Again, this isn't a problem for a real Git server, but it\ndoes mean that feeding a malicious URL to Git (e.g., through a\nsubmodule) can cause it to make unexpected cross-protocol requests.\nSince repository names with newlines are presumably quite rare (and\nindeed, we already disallow them in git-over-http), let's just disallow\nthem over this protocol.\n\nHostnames could likewise inject a newline, but this is unlikely a\nproblem in practice; we'd try resolving the hostname with a newline in\nit, which wouldn't work. Still, it doesn't hurt to err on the side of\ncaution there, since we would not expect them to work in the first\nplace.\n\nThe ssh and local code paths are unaffected by this patch. In both cases\nwe're trying to run upload-pack via a shell, and will quote the newline\nso that it makes it intact. An attacker can point an ssh url at an\narbitrary port, of course, but unless there's an actual ssh server\nthere, we'd never get as far as sending our shell command anyway. We\n_could_ similarly restrict newlines in those protocols out of caution,\nbut there seems little benefit to doing so.\n\nThe new test here is run alongside the git-daemon tests, which cover the\nsame protocol, but it shouldn't actually contact the daemon at all. In\ntheory we could make the test more robust by setting up an actual\nrepository with a newline in it (so that our clone would succeed if our\nnew check didn't kick in). But a repo directory with newline in it is\nlikely not portable across all filesystems. Likewise, we could check\ngit-daemon's log that it was not contacted at all, but we do not\ncurrently record the log (and anyway, it would make the test racy with\nthe daemon's log write). We'll just check the client-side stderr to make\nsure we hit the expected code path.\n\nReported-by: Harold Kim \nSigned-off-by: Jeff King \n---\n connect.c | 2 ++\n t/t5570-git-daemon.sh | 5 +++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 8b8f56cf6d..9c97fee430 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -1160,6 +1160,8 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,\n \t\ttarget_host = xstrdup(hostandport);\n \n \ttransport_check_allowed(\"git\");\n+\tif (strchr(target_host, '\\n') || strchr(path, '\\n'))\n+\t\tdie(_(\"newline is forbidden in git:// hosts and repo paths\"));\n \n \t/*\n \t * These underlying connection commands die() if they\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 8f69a7854f..0fbb194810 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -103,6 +103,11 @@ test_expect_success 'fetch notices corrupt idx' '\n \t)\n '\n \n+test_expect_success 'client refuses to ask for repo with newline' '\n+\ttest_must_fail git clone \"$GIT_DAEMON_URL/repo$LF.git\" dst 2>stderr &&\n+\ttest_i18ngrep newline.is.forbidden stderr\n+'\n+\n test_remote_error()\n {\n \tdo_export=YesPlease\n-- \n2.30.0.572.g094076f6a2\n\n"},{"id":"413666","messageId":"X/bX8XDYH2rni9uV@coredump.intra.peff.net","threadId":"54952","inReplyTo":"X/bXV9dGFK2bxBV0@coredump.intra.peff.net","subject":"[PATCH 2/2] fsck: reject .gitmodules git:// urls with newlines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-07T09:44:17Z","receivedAt":"2021-01-07T09:45:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit taught the clone/fetch client side to reject a\ngit:// URL with a newline in it. Let's also catch these when fscking a\n.gitmodules file, which will give an earlier warning.\n\nNote that it would be simpler to just complain about newline in _any_\nURL, but an earlier tightening for http/ftp made sure we kept allowing\nnewlines for unknown protocols (and this is covered in the tests). So\nwe'll stick to that precedent.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 2 +-\n t/t7416-submodule-dash-url.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex f82e2fe9e3..5e282b3b6b 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1082,7 +1082,7 @@ static int check_submodule_url(const char *url)\n \tif (looks_like_command_line_option(url))\n \t\treturn -1;\n \n-\tif (submodule_url_is_relative(url)) {\n+\tif (submodule_url_is_relative(url) || starts_with(url, \"git://\")) {\n \t\tchar *decoded;\n \t\tconst char *next;\n \t\tint has_nl;\ndiff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh\nindex eec96e0ba9..d21dc8b009 100755\n--- a/t/t7416-submodule-dash-url.sh\n+++ b/t/t7416-submodule-dash-url.sh\n@@ -201,4 +201,19 @@ test_expect_success 'fsck rejects embedded newline in relative url' '\n \tgrep gitmodulesUrl err\n '\n \n+test_expect_success 'fsck rejects embedded newline in git url' '\n+\tgit checkout --orphan git-newline &&\n+\tcat >.gitmodules <<-\\EOF &&\n+\t[submodule \"foo\"]\n+\turl = \"git://example.com:1234/repo%0a.git\"\n+\tEOF\n+\tgit add .gitmodules &&\n+\tgit commit -m \"git url with newline\" &&\n+\ttest_when_finished \"rm -rf dst\" &&\n+\tgit init --bare dst &&\n+\tgit -C dst config transfer.fsckObjects true &&\n+\ttest_must_fail git push dst HEAD 2>err &&\n+\tgrep gitmodulesUrl err\n+'\n+\n test_done\n-- \n2.30.0.572.g094076f6a2\n"}]}