{"thread":{"id":"52270","subject":"[PATCH 0/4] gitweb: quote base url more consistently","startedAt":"2019-11-15T09:05:48Z","lastAt":"2019-11-18T01:45:52Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"386263","messageId":"20191115090545.GA30971@sigill.intra.peff.net","threadId":"52270","inReplyTo":null,"subject":"[PATCH 0/4] gitweb: quote base url more consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-15T09:05:45Z","receivedAt":"2019-11-15T09:05:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes an XSS issue reported to the git-security list where\ngitweb doesn't always quote its base url, meaning a specially-crafted\nURL can inject HTML into the finished page. Given the relatively low\nseverity of the problem and my lack of familiarity with gitweb, it makes\nsense to me to just discuss this one in the open.\n\nCredit for the finding the problem (and some patient explanations) goes\nto NAKAYAMA DAISUKE .\n\n [1/4]: t9502: pass along all arguments in xss helper\n [2/4]: t/gitweb-lib.sh: drop confusing quotes\n [3/4]: t/gitweb-lib.sh: set $REQUEST_URI\n [4/4]: gitweb: escape URLs generated by href()\n\n gitweb/gitweb.perl | 31 +++++++++++++----------\n t/gitweb-lib.sh | 7 ++---\n t/t9502-gitweb-standalone-parse-output.sh | 7 ++---\n 3 files changed, 25 insertions(+), 20 deletions(-)\n\n-Peff\n"},{"id":"386264","messageId":"20191115090556.GA21987@sigill.intra.peff.net","threadId":"52270","inReplyTo":"20191115090545.GA30971@sigill.intra.peff.net","subject":"[PATCH 1/4] t9502: pass along all arguments in xss helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-15T09:05:56Z","receivedAt":"2019-11-15T09:05:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function is just a thin wrapper around gitweb_run(), which takes\nmultiple arguments. But we only pass along \"$1\". Let's pass everything\nwe get, which will let a future patch add an XSS test that affects\nPATH_INFO (which gitweb_run() takes as $2).\n\nSigned-off-by: Jeff King \n---\n t/t9502-gitweb-standalone-parse-output.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh\nindex 0796a438bc..1b04c29037 100755\n--- a/t/t9502-gitweb-standalone-parse-output.sh\n+++ b/t/t9502-gitweb-standalone-parse-output.sh\n@@ -188,8 +188,8 @@ test_expect_success 'forks: project_index lists all projects (incl. forks)' '\n '\n \n xss() {\n-\techo >&2 \"Checking $1...\" &&\n-\tgitweb_run \"$1\" &&\n+\techo >&2 \"Checking $*...\" &&\n+\tgitweb_run \"$@\" &&\n \tif grep \"$TAG\" gitweb.body; then\n \t\techo >&2 \"xss: $TAG should have been quoted in output\"\n \t\treturn 1\n-- \n2.24.0.739.gb5632e4929\n\n"},{"id":"386265","messageId":"20191115090601.GB21987@sigill.intra.peff.net","threadId":"52270","inReplyTo":"20191115090545.GA30971@sigill.intra.peff.net","subject":"[PATCH 2/4] t/gitweb-lib.sh: drop confusing quotes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-15T09:06:01Z","receivedAt":"2019-11-15T09:06:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some variables assignments in gitweb_run() look like this:\n\n FOO=\"\"$1\"\"\n\nThe extra quotes aren't doing anything. Each set opens and closes an\nempty string, and $1 is actually outside of any double-quotes (which is\nOK, because variable assignment does not do whitespace splitting on the\nexpanded value).\n\nLet's drop them, as they're simply confusing.\n\nSigned-off-by: Jeff King \n---\n t/gitweb-lib.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh\nindex 006d2a8152..130c7ed64f 100644\n--- a/t/gitweb-lib.sh\n+++ b/t/gitweb-lib.sh\n@@ -58,8 +58,8 @@ gitweb_run () {\n \tGATEWAY_INTERFACE='CGI/1.1'\n \tHTTP_ACCEPT='*/*'\n \tREQUEST_METHOD='GET'\n-\tQUERY_STRING=\"\"$1\"\"\n-\tPATH_INFO=\"\"$2\"\"\n+\tQUERY_STRING=$1\n+\tPATH_INFO=$2\n \texport GATEWAY_INTERFACE HTTP_ACCEPT REQUEST_METHOD \\\n \t\tQUERY_STRING PATH_INFO\n \n-- \n2.24.0.739.gb5632e4929\n\n"},{"id":"386266","messageId":"20191115090604.GC21987@sigill.intra.peff.net","threadId":"52270","inReplyTo":"20191115090545.GA30971@sigill.intra.peff.net","subject":"[PATCH 3/4] t/gitweb-lib.sh: set $REQUEST_URI","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-15T09:06:04Z","receivedAt":"2019-11-15T09:06:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In a real webserver's CGI call, gitweb.cgi would typically see\n$REQUEST_URI set. This variable does impact how we display our URL in\nthe resulting page, so let's try to make our test as realistic as\npossible (we can just use the $PATH_INFO our caller passed in, if any).\n\nThis doesn't change the outcome of any tests, but it will help us add\nsome new tests in a future patch.\n\nSigned-off-by: Jeff King \n---\n t/gitweb-lib.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh\nindex 130c7ed64f..1f32ca66ea 100644\n--- a/t/gitweb-lib.sh\n+++ b/t/gitweb-lib.sh\n@@ -60,8 +60,9 @@ gitweb_run () {\n \tREQUEST_METHOD='GET'\n \tQUERY_STRING=$1\n \tPATH_INFO=$2\n+\tREQUEST_URI=/gitweb.cgi$PATH_INFO\n \texport GATEWAY_INTERFACE HTTP_ACCEPT REQUEST_METHOD \\\n-\t\tQUERY_STRING PATH_INFO\n+\t\tQUERY_STRING PATH_INFO REQUEST_URI\n \n \tGITWEB_CONFIG=$(pwd)/gitweb_config.perl\n \texport GITWEB_CONFIG\n-- \n2.24.0.739.gb5632e4929\n\n"},{"id":"386267","messageId":"20191115090607.GD21987@sigill.intra.peff.net","threadId":"52270","inReplyTo":"20191115090545.GA30971@sigill.intra.peff.net","subject":"[PATCH 4/4] gitweb: escape URLs generated by href()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-15T09:06:07Z","receivedAt":"2019-11-15T09:06:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a cross-site scripting problem in gitweb, where it will print\nURLs generated by its href() helper without further quoting. This allows\nan attacker to point a victim to a specially crafted gitweb URL and\ninject arbitrary HTML into the resulting page (which the victim sees as\ncoming from gitweb).\n\nThe base of the URL comes from evaluate_uri(), which pulls the value of\n$REQUEST_URI via the CGI module. It tries to strip off $PATH_INFO, but\nfails to do so in some cases (including ones that contain special\ncharacters, like \"+\"). Most of the uses of the URL end up being passed\nto \"$cgi->a(-href = href())\", which will get quoted properly by the CGI\nmodule. But in a few places, we output them ourselves as part of\nmanually-generated HTML, and whatever was in the original URL will\nappear unquoted in the output.\n\nGiven that all of the nearby variables placed into this manual HTML\n_are_ quoted, it seems like the authors assumed that these URLs would\nnot need quoting. So it's possible that the bug is actually in\nevaluate_uri(), which should be doing a more careful job of stripping\n$PATH_INFO. There's some discussion in a comment in that function, as\nwell as the commit message in 81d3fe9f48 (gitweb: fix wrong base URL\nwhen non-root DirectoryIndex, 2009-02-15). But I'm not sure I understand\nit.\n\nRegardless, it's a good idea to quote these values at the point of\ninsertion into the HTML output:\n\n 1. Even if there is a bug in evaluate_uri(), this would give us\n belt-and-suspenders protection.\n\n 2. evaluate_uri() is only handling the base. Some generated URLs will\n also mention arbitrary refs or filenames in the repositories, and\n these should be quoted anyway.\n\n 3. It should never _hurt_ to quote (and that's what all of the\n $cgi->a() calls are doing already).\n\nSo there may be further work here, but this patch at least prevents the\nXSS vulnerability, and shouldn't make anything worse.\n\nThe test here covers the calls in print_feed_meta(), but I manually\naudited every call to href() to see how its output was used, and quoted\nappropriately. Most of them are esc_attr(), as they're used in tag\nattributes, but I used esc_html() when the URLs were printed bare. The\ndistinction is largely academic, as one is implemented as a wrapper for\nthe other.\n\nReported-by: NAKAYAMA DAISUKE \nSigned-off-by: Jeff King \n---\n gitweb/gitweb.perl | 31 +++++++++++++----------\n t/t9502-gitweb-standalone-parse-output.sh | 3 ++-\n 2 files changed, 19 insertions(+), 15 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 7fef19fe59..a2cc4d9fb0 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -4048,7 +4048,7 @@ sub print_feed_meta {\n \n \t\t\t$href_params{'extra_options'} = undef;\n \t\t\t$href_params{'action'} = $type;\n-\t\t\t$link_attr{'-href'} = href(%href_params);\n+\t\t\t$link_attr{'-href'} = esc_attr(href(%href_params));\n \t\t\tprint \"\\n\";\n \n \t\t\t$href_params{'extra_options'} = '--no-merges';\n-\t\t\t$link_attr{'-href'} = href(%href_params);\n+\t\t\t$link_attr{'-href'} = esc_attr(href(%href_params));\n \t\t\t$link_attr{'-title'} .= ' (no merges)';\n \t\t\tprint \"'.\"\\n\",\n-\t\t esc_attr($site_name), href(project=>undef, action=>\"project_index\"));\n+\t\t esc_attr($site_name),\n+\t\t esc_attr(href(project=>undef, action=>\"project_index\")));\n \t\tprintf(''.\"\\n\",\n-\t\t esc_attr($site_name), href(project=>undef, action=>\"opml\"));\n+\t\t esc_attr($site_name),\n+\t\t esc_attr(href(project=>undef, action=>\"opml\")));\n \t}\n }\n \n@@ -4287,8 +4289,8 @@ sub git_footer_html {\n \tif (defined $action &&\n \t $action eq 'blame_incremental') {\n \t\tprint qq!\\n!;\n \t} else {\n \t\tmy ($jstimezone, $tz_cookie, $datetime_class) =\n@@ -7155,8 +7157,8 @@ sub git_blob {\n \t\t\tprint qq! alt=\"!.esc_attr($file_name).qq!\" title=\"!.esc_attr($file_name).qq!\"!;\n \t\t}\n \t\tprint qq! src=\"! .\n-\t\t href(action=>\"blob_plain\", hash=>$hash,\n-\t\t hash_base=>$hash_base, file_name=>$file_name) .\n+\t\t esc_attr(href(action=>\"blob_plain\", hash=>$hash,\n+\t\t hash_base=>$hash_base, file_name=>$file_name)) .\n \t\t qq!\" />\\n!;\n \t} else {\n \t\tmy $nr;\n@@ -8239,6 +8241,7 @@ sub git_feed {\n \t} else {\n \t\t$alt_url = href(-full=>1, action=>\"summary\");\n \t}\n+\t$alt_url = esc_attr($alt_url);\n \tprint qq!\\n!;\n \tif ($format eq 'rss') {\n \t\tprint <' . \"\\n\" .\n \t\t 'self_url() . '\" />' . \"\\n\" .\n-\t\t \"\" . href(-full=>1) . \"\\n\" .\n+\t\t \"\" . esc_url(href(-full=>1)) . \"\\n\" .\n \t\t # use project owner for feed author\n \t\t \"$owner\\n\";\n \t\tif (defined $favicon) {\n@@ -8322,7 +8325,7 @@ sub git_feed {\n \t\t\t \"\" . esc_html($co{'author'}) . \"\\n\" .\n \t\t\t \"$cd{'rfc2822'}\\n\" .\n \t\t\t \"$co_url\\n\" .\n-\t\t\t \"$co_url\\n\" .\n+\t\t\t \"\" . esc_html($co_url) . \"\\n\" .\n \t\t\t \"\" . esc_html($co{'title'}) . \"\\n\" .\n \t\t\t \"\" .\n \t\t\t \"\\n\" .\n \t\t\t \"$cd{'iso-8601'}\\n\" .\n-\t\t\t \"\\n\" .\n-\t\t\t \"$co_url\\n\" .\n+\t\t\t \"\\n\" .\n+\t\t\t \"\" . esc_html($co_url) . \"\\n\" .\n \t\t\t \"\\n\" .\n \t\t\t \"
\\n\";\n \t\t}\n@@ -8452,8 +8455,8 @@ sub git_opml {\n \t\t}\n \n \t\tmy $path = esc_html(chop_str($proj{'path'}, 25, 5));\n-\t\tmy $rss = href('project' => $proj{'path'}, 'action' => 'rss', -full => 1);\n-\t\tmy $html = href('project' => $proj{'path'}, 'action' => 'summary', -full => 1);\n+\t\tmy $rss = esc_attr(href('project' => $proj{'path'}, 'action' => 'rss', -full => 1));\n+\t\tmy $html = esc_attr(href('project' => $proj{'path'}, 'action' => 'summary', -full => 1));\n \t\tprint \"\\n\";\n \t}\n \tprint <\" &&\n \txss \"a=rss&p=$TAG\" &&\n-\txss \"a=rss&p=foo.git&f=$TAG\"\n+\txss \"a=rss&p=foo.git&f=$TAG\" &&\n+\txss \"\" \"$TAG+\"\n '\n \n test_done\n-- \n2.24.0.739.gb5632e4929\n"},{"id":"386429","messageId":"xmqq1ru63qk5.fsf@gitster-ct.c.googlers.com","threadId":"52270","inReplyTo":"20191115090545.GA30971@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] gitweb: quote base url more consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-18T01:45:46Z","receivedAt":"2019-11-18T01:45:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This series fixes an XSS issue reported to the git-security list where\n> gitweb doesn't always quote its base url, meaning a specially-crafted\n> URL can inject HTML into the finished page. Given the relatively low\n> severity of the problem and my lack of familiarity with gitweb, it makes\n> sense to me to just discuss this one in the open.\n>\n> Credit for the finding the problem (and some patient explanations) goes\n> to NAKAYAMA DAISUKE .\n>\n> [1/4]: t9502: pass along all arguments in xss helper\n> [2/4]: t/gitweb-lib.sh: drop confusing quotes\n> [3/4]: t/gitweb-lib.sh: set $REQUEST_URI\n> [4/4]: gitweb: escape URLs generated by href()\n>\n> gitweb/gitweb.perl | 31 +++++++++++++----------\n> t/gitweb-lib.sh | 7 ++---\n> t/t9502-gitweb-standalone-parse-output.sh | 7 ++---\n> 3 files changed, 25 insertions(+), 20 deletions(-)\n>\n> -Peff\n\n\nThanks, will queue.\n"}]}