Re: [BUG] gitweb: XSS vulnerability of RSS feed

[Jeff King <peff@peff.net>]

On Mon, Nov 12, 2012 at 10:13:27PM +0100, Jakub Narębski wrote:

> > Yeah, that looks correct, given the way how the other variables
> > emitted with the same "print" like $descr and $owner are formed.
> 
> It looks like good solution to me too.
> 
> Nb. the problems with feed are mainly because it is generated
> by hand even more than HTML (which uses CGI.pm).

Yeah, I noticed that. Here it is in patch form with a test. It would be
nice if people interested in gitweb would add more entries to the XSS
test below (I put in the one that fails, along with an obvious variation
that is actually OK).

I didn't look carefully through the rest of gitweb for more XSS
instances. From a glance, it looks like we mostly use the safe CGI
methods, but probably it could use a full audit (which again, I would be
happy if people who care more about gitweb would do).

-- >8 --
Subject: [PATCH] gitweb: escape html in rss title

The title of an RSS feed is generated from many components,
including the filename provided as a query parameter, but we
failed to quote it.  Besides showing the wrong output, this
is a vector for XSS attacks.

Signed-off-by: Jeff King <peff@peff.net>
---
 gitweb/gitweb.perl                        |  1 +
 t/t9502-gitweb-standalone-parse-output.sh | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 10ed9e5..a51a8ba 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -8055,6 +8055,7 @@ sub git_feed {
 		$feed_type = 'history';
 	}
 	$title .= " $feed_type";
+	$title = esc_html($title);
 	my $descr = git_get_project_description($project);
 	if (defined $descr) {
 		$descr = esc_html($descr);
diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
index 731e64c..3a8e7d3 100755
--- a/t/t9502-gitweb-standalone-parse-output.sh
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -185,5 +185,20 @@ test_expect_success 'forks: project_index lists all projects (incl. forks)' '
 	test_cmp expected actual
 '
 
+xss() {
+	echo >&2 "Checking $1..." &&
+	gitweb_run "$1" &&
+	if grep "$TAG" gitweb.body; then
+		echo >&2 "xss: $TAG should have been quoted in output"
+		return 1
+	fi
+	return 0
+}
+
+test_expect_success 'xss checks' '
+	TAG="<magic-xss-tag>" &&
+	xss "a=rss&p=$TAG" &&
+	xss "a=rss&p=foo.git&f=$TAG"
+'
 
 test_done
-- 
1.8.0.207.gdf2154c