From 0eba60c9b781ce8169f1288a73b538f1ecf3b615 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 15 Nov 2019 04:05:56 -0500 Subject: [PATCH 1/4] t9502: pass along all arguments in xss helper This function is just a thin wrapper around gitweb_run(), which takes multiple arguments. But we only pass along "$1". Let's pass everything we get, which will let a future patch add an XSS test that affects PATH_INFO (which gitweb_run() takes as $2). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t9502-gitweb-standalone-parse-output.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh index 0796a438bc..1b04c29037 100755 --- a/t/t9502-gitweb-standalone-parse-output.sh +++ b/t/t9502-gitweb-standalone-parse-output.sh @@ -188,8 +188,8 @@ test_expect_success 'forks: project_index lists all projects (incl. forks)' ' ' xss() { - echo >&2 "Checking $1..." && - gitweb_run "$1" && + echo >&2 "Checking $*..." && + gitweb_run "$@" && if grep "$TAG" gitweb.body; then echo >&2 "xss: $TAG should have been quoted in output" return 1 From f28bceca7545727064b4e9a73ac7e9acf08b54b4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 15 Nov 2019 04:06:01 -0500 Subject: [PATCH 2/4] t/gitweb-lib.sh: drop confusing quotes Some variables assignments in gitweb_run() look like this: FOO=""$1"" The extra quotes aren't doing anything. Each set opens and closes an empty string, and $1 is actually outside of any double-quotes (which is OK, because variable assignment does not do whitespace splitting on the expanded value). Let's drop them, as they're simply confusing. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/gitweb-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh index 006d2a8152..130c7ed64f 100644 --- a/t/gitweb-lib.sh +++ b/t/gitweb-lib.sh @@ -58,8 +58,8 @@ gitweb_run () { GATEWAY_INTERFACE='CGI/1.1' HTTP_ACCEPT='*/*' REQUEST_METHOD='GET' - QUERY_STRING=""$1"" - PATH_INFO=""$2"" + QUERY_STRING=$1 + PATH_INFO=$2 export GATEWAY_INTERFACE HTTP_ACCEPT REQUEST_METHOD \ QUERY_STRING PATH_INFO From b178c207d72bd814a8004cefc477a47b6602f0be Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 15 Nov 2019 04:06:04 -0500 Subject: [PATCH 3/4] t/gitweb-lib.sh: set $REQUEST_URI In a real webserver's CGI call, gitweb.cgi would typically see $REQUEST_URI set. This variable does impact how we display our URL in the resulting page, so let's try to make our test as realistic as possible (we can just use the $PATH_INFO our caller passed in, if any). This doesn't change the outcome of any tests, but it will help us add some new tests in a future patch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/gitweb-lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh index 130c7ed64f..1f32ca66ea 100644 --- a/t/gitweb-lib.sh +++ b/t/gitweb-lib.sh @@ -60,8 +60,9 @@ gitweb_run () { REQUEST_METHOD='GET' QUERY_STRING=$1 PATH_INFO=$2 + REQUEST_URI=/gitweb.cgi$PATH_INFO export GATEWAY_INTERFACE HTTP_ACCEPT REQUEST_METHOD \ - QUERY_STRING PATH_INFO + QUERY_STRING PATH_INFO REQUEST_URI GITWEB_CONFIG=$(pwd)/gitweb_config.perl export GITWEB_CONFIG From a376e37b2c418b8f630741552d76a5b18c87c133 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 15 Nov 2019 04:06:07 -0500 Subject: [PATCH 4/4] gitweb: escape URLs generated by href() There's a cross-site scripting problem in gitweb, where it will print URLs generated by its href() helper without further quoting. This allows an attacker to point a victim to a specially crafted gitweb URL and inject arbitrary HTML into the resulting page (which the victim sees as coming from gitweb). The base of the URL comes from evaluate_uri(), which pulls the value of $REQUEST_URI via the CGI module. It tries to strip off $PATH_INFO, but fails to do so in some cases (including ones that contain special characters, like "+"). Most of the uses of the URL end up being passed to "$cgi->a(-href = href())", which will get quoted properly by the CGI module. But in a few places, we output them ourselves as part of manually-generated HTML, and whatever was in the original URL will appear unquoted in the output. Given that all of the nearby variables placed into this manual HTML _are_ quoted, it seems like the authors assumed that these URLs would not need quoting. So it's possible that the bug is actually in evaluate_uri(), which should be doing a more careful job of stripping $PATH_INFO. There's some discussion in a comment in that function, as well as the commit message in 81d3fe9f48 (gitweb: fix wrong base URL when non-root DirectoryIndex, 2009-02-15). But I'm not sure I understand it. Regardless, it's a good idea to quote these values at the point of insertion into the HTML output: 1. Even if there is a bug in evaluate_uri(), this would give us belt-and-suspenders protection. 2. evaluate_uri() is only handling the base. Some generated URLs will also mention arbitrary refs or filenames in the repositories, and these should be quoted anyway. 3. It should never _hurt_ to quote (and that's what all of the $cgi->a() calls are doing already). So there may be further work here, but this patch at least prevents the XSS vulnerability, and shouldn't make anything worse. The test here covers the calls in print_feed_meta(), but I manually audited every call to href() to see how its output was used, and quoted appropriately. Most of them are esc_attr(), as they're used in tag attributes, but I used esc_html() when the URLs were printed bare. The distinction is largely academic, as one is implemented as a wrapper for the other. Reported-by: NAKAYAMA DAISUKE Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- gitweb/gitweb.perl | 31 +++++++++++++---------- t/t9502-gitweb-standalone-parse-output.sh | 3 ++- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 7fef19fe59..a2cc4d9fb0 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -4048,7 +4048,7 @@ sub print_feed_meta { $href_params{'extra_options'} = undef; $href_params{'action'} = $type; - $link_attr{'-href'} = href(%href_params); + $link_attr{'-href'} = esc_attr(href(%href_params)); print "\n"; $href_params{'extra_options'} = '--no-merges'; - $link_attr{'-href'} = href(%href_params); + $link_attr{'-href'} = esc_attr(href(%href_params)); $link_attr{'-title'} .= ' (no merges)'; print "'."\n", - esc_attr($site_name), href(project=>undef, action=>"project_index")); + esc_attr($site_name), + esc_attr(href(project=>undef, action=>"project_index"))); printf(''."\n", - esc_attr($site_name), href(project=>undef, action=>"opml")); + esc_attr($site_name), + esc_attr(href(project=>undef, action=>"opml"))); } } @@ -4287,8 +4289,8 @@ sub git_footer_html { if (defined $action && $action eq 'blame_incremental') { print qq!\n!; } else { my ($jstimezone, $tz_cookie, $datetime_class) = @@ -7155,8 +7157,8 @@ sub git_blob { print qq! alt="!.esc_attr($file_name).qq!" title="!.esc_attr($file_name).qq!"!; } print qq! src="! . - href(action=>"blob_plain", hash=>$hash, - hash_base=>$hash_base, file_name=>$file_name) . + esc_attr(href(action=>"blob_plain", hash=>$hash, + hash_base=>$hash_base, file_name=>$file_name)) . qq!" />\n!; } else { my $nr; @@ -8239,6 +8241,7 @@ sub git_feed { } else { $alt_url = href(-full=>1, action=>"summary"); } + $alt_url = esc_attr($alt_url); print qq!\n!; if ($format eq 'rss') { print <' . "\n" . '' . "\n" . - "" . href(-full=>1) . "\n" . + "" . esc_url(href(-full=>1)) . "\n" . # use project owner for feed author "$owner\n"; if (defined $favicon) { @@ -8322,7 +8325,7 @@ sub git_feed { "" . esc_html($co{'author'}) . "\n" . "$cd{'rfc2822'}\n" . "$co_url\n" . - "$co_url\n" . + "" . esc_html($co_url) . "\n" . "" . esc_html($co{'title'}) . "\n" . "" . "\n" . "$cd{'iso-8601'}\n" . - "\n" . - "$co_url\n" . + "\n" . + "" . esc_html($co_url) . "\n" . "\n" . "
\n"; } @@ -8452,8 +8455,8 @@ sub git_opml { } my $path = esc_html(chop_str($proj{'path'}, 25, 5)); - my $rss = href('project' => $proj{'path'}, 'action' => 'rss', -full => 1); - my $html = href('project' => $proj{'path'}, 'action' => 'summary', -full => 1); + my $rss = esc_attr(href('project' => $proj{'path'}, 'action' => 'rss', -full => 1)); + my $html = esc_attr(href('project' => $proj{'path'}, 'action' => 'summary', -full => 1)); print "\n"; } print <