[PATCH 0/4] gitweb: quote base url more consistently

[PATCH 0/4] gitweb: quote base url more consistently

[Jeff King]

This series fixes an XSS issue reported to the git-security list where
gitweb doesn't always quote its base url, meaning a specially-crafted
URL can inject HTML into the finished page. Given the relatively low
severity of the problem and my lack of familiarity with gitweb, it makes
sense to me to just discuss this one in the open.

Credit for the finding the problem (and some patient explanations) goes
to NAKAYAMA DAISUKE <nakyamad@icloud.com>.

  [1/4]: t9502: pass along all arguments in xss helper
  [2/4]: t/gitweb-lib.sh: drop confusing quotes
  [3/4]: t/gitweb-lib.sh: set $REQUEST_URI
  [4/4]: gitweb: escape URLs generated by href()

 gitweb/gitweb.perl                        | 31 +++++++++++++----------
 t/gitweb-lib.sh                           |  7 ++---
 t/t9502-gitweb-standalone-parse-output.sh |  7 ++---
 3 files changed, 25 insertions(+), 20 deletions(-)

-Peff

[PATCH 1/4] t9502: pass along all arguments in xss helper

[Jeff King]

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 <peff@peff.net>
---
 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
-- 
2.24.0.739.gb5632e4929

[PATCH 2/4] t/gitweb-lib.sh: drop confusing quotes

[Jeff King]

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 <peff@peff.net>
---
 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
 
-- 
2.24.0.739.gb5632e4929

[PATCH 3/4] t/gitweb-lib.sh: set $REQUEST_URI

[Jeff King]

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 <peff@peff.net>
---
 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
-- 
2.24.0.739.gb5632e4929

[PATCH 4/4] gitweb: escape URLs generated by href()

[Jeff King]

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 <nakyamad@icloud.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 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 "<link ".
 			      "rel=\"$link_attr{'-rel'}\" ".
 			      "title=\"$link_attr{'-title'}\" ".
@@ -4057,7 +4057,7 @@ sub print_feed_meta {
 			      "/>\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 "<link ".
 			      "rel=\"$link_attr{'-rel'}\" ".
@@ -4070,10 +4070,12 @@ sub print_feed_meta {
 	} else {
 		printf('<link rel="alternate" title="%s projects list" '.
 		       'href="%s" type="text/plain; charset=utf-8" />'."\n",
-		       esc_attr($site_name), href(project=>undef, action=>"project_index"));
+		       esc_attr($site_name),
+		       esc_attr(href(project=>undef, action=>"project_index")));
 		printf('<link rel="alternate" title="%s projects feeds" '.
 		       'href="%s" type="text/x-opml" />'."\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!<script type="text/javascript">\n!.
-		      qq!startBlame("!. href(action=>"blame_data", -replay=>1) .qq!",\n!.
-		      qq!           "!. href() .qq!");\n!.
+		      qq!startBlame("!. esc_attr(href(action=>"blame_data", -replay=>1)) .qq!",\n!.
+		      qq!           "!. esc_attr(href()) .qq!");\n!.
 		      qq!</script>\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!<?xml version="1.0" encoding="utf-8"?>\n!;
 	if ($format eq 'rss') {
 		print <<XML;
@@ -8276,7 +8279,7 @@ sub git_feed {
 		      $alt_url . '" />' . "\n" .
 		      '<link rel="self" type="' . $content_type . '" href="' .
 		      $cgi->self_url() . '" />' . "\n" .
-		      "<id>" . href(-full=>1) . "</id>\n" .
+		      "<id>" . esc_url(href(-full=>1)) . "</id>\n" .
 		      # use project owner for feed author
 		      "<author><name>$owner</name></author>\n";
 		if (defined $favicon) {
@@ -8322,7 +8325,7 @@ sub git_feed {
 			      "<author>" . esc_html($co{'author'}) . "</author>\n" .
 			      "<pubDate>$cd{'rfc2822'}</pubDate>\n" .
 			      "<guid isPermaLink=\"true\">$co_url</guid>\n" .
-			      "<link>$co_url</link>\n" .
+			      "<link>" . esc_html($co_url) . "</link>\n" .
 			      "<description>" . esc_html($co{'title'}) . "</description>\n" .
 			      "<content:encoded>" .
 			      "<![CDATA[\n";
@@ -8344,8 +8347,8 @@ sub git_feed {
 			}
 			print "</contributor>\n" .
 			      "<published>$cd{'iso-8601'}</published>\n" .
-			      "<link rel=\"alternate\" type=\"text/html\" href=\"$co_url\" />\n" .
-			      "<id>$co_url</id>\n" .
+			      "<link rel=\"alternate\" type=\"text/html\" href=\"" . esc_attr($co_url) . "\" />\n" .
+			      "<id>" . esc_html($co_url) . "</id>\n" .
 			      "<content type=\"xhtml\" xml:base=\"" . esc_url($my_url) . "\">\n" .
 			      "<div xmlns=\"http://www.w3.org/1999/xhtml\">\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 "<outline type=\"rss\" text=\"$path\" title=\"$path\" xmlUrl=\"$rss\" htmlUrl=\"$html\"/>\n";
 	}
 	print <<XML;
diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
index 1b04c29037..e38cbc97d3 100755
--- a/t/t9502-gitweb-standalone-parse-output.sh
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -200,7 +200,8 @@ xss() {
 test_expect_success 'xss checks' '
 	TAG="<magic-xss-tag>" &&
 	xss "a=rss&p=$TAG" &&
-	xss "a=rss&p=foo.git&f=$TAG"
+	xss "a=rss&p=foo.git&f=$TAG" &&
+	xss "" "$TAG+"
 '
 
 test_done
-- 
2.24.0.739.gb5632e4929

Re: [PATCH 0/4] gitweb: quote base url more consistently

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This series fixes an XSS issue reported to the git-security list where
> gitweb doesn't always quote its base url, meaning a specially-crafted
> URL can inject HTML into the finished page. Given the relatively low
> severity of the problem and my lack of familiarity with gitweb, it makes
> sense to me to just discuss this one in the open.
>
> Credit for the finding the problem (and some patient explanations) goes
> to NAKAYAMA DAISUKE <nakyamad@icloud.com>.
>
>   [1/4]: t9502: pass along all arguments in xss helper
>   [2/4]: t/gitweb-lib.sh: drop confusing quotes
>   [3/4]: t/gitweb-lib.sh: set $REQUEST_URI
>   [4/4]: gitweb: escape URLs generated by href()
>
>  gitweb/gitweb.perl                        | 31 +++++++++++++----------
>  t/gitweb-lib.sh                           |  7 ++---
>  t/t9502-gitweb-standalone-parse-output.sh |  7 ++---
>  3 files changed, 25 insertions(+), 20 deletions(-)
>
> -Peff


Thanks, will queue.