[BUG] gitweb: XSS vulnerability of RSS feed

Gitweb can be used to generate an RSS feed.

Arbitrary tags can be inserted into the XML document describing
the RSS feed by careful construction of the URL.

Example
http://server/?p=project.git&a=rss&f=</title><script>alert(document.cookie)</script><title>

The generated XML contains
<script>alert(document.cookie)</script>

Depending on the system used to render the XML this might lead
to the execution of javascript in the security context of the
gitweb server pages.

Please, escape all URL parameters.

Version tested:
gitweb v.1.8.0.dirty with git 1.7.2.5

Best regards

Heinrich Schuchardt

On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron [off-list ref] wrote:

Gitweb can be used to generate an RSS feed.

Arbitrary tags can be inserted into the XML document describing
the RSS feed by careful construction of the URL.

Example
http://server/?p=project.git&a=rss&f=</title><script>alert(document.cookie)</script><title>

The generated XML contains
<script>alert(document.cookie)</script>

Depending on the system used to render the XML this might lead
to the execution of javascript in the security context of the
gitweb server pages.

Please, escape all URL parameters.

Version tested:
gitweb v.1.8.0.dirty with git 1.7.2.5

Best regards

quoted

Heinrich Schuchardt

Something like this may be useful to defuse the "file" parameter, but
I presume a more definitive fix is in order...

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 10ed9e5..af93e65 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -1447,6 +1447,10 @@ sub validate_pathname {
        if ($input =~ m!\0!) {
                return undef;
        }
+       # No XSS <script></script> inclusions
+       if ($input =~ m!(<script>)(.*)(</script>)!){
+               return undef;
+       }
        return $input;
 }

(I am not a perl god, so this was the lowest hanging fruit.)

If desired I'll fashion this up into a proper patch.

-- 
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:

This is the wrong fix for a few reasons:

  1. It is on the input-validation side, whereas the real problem is on
     the output-quoting side. Your patch means I could not access a file
     called "<script>foo</script>". What we really want is to have the
     unquoted name internally, but then make sure we quote it when
     outputting as part of an HTML (or XML) file.

  2. Script tags are only part of the problem. They are what make it
     obviously a security vulnerability, but it is equally incorrect for
     us to show the filename "<b>foo</b>" as bold. I would also not be
     surprised if there are other cross-site attacks one can do without
     using <script>.

  3. Your filter is too simplistic. At the very least, it would not
     filter out "<SCRIPT>". I am not up to date on all of the
     sneaking-around-HTML-filters attacks that are available these days,
     but I wonder if one could also get around it using XML entities or
     similar.

I think the right answer is going to be a well-placed call to esc_html.
This already happens automatically when we go through the CGI
element-building functions, but obviously we failed to make the call
when building the output manually.  This is a great reason why template
languages which default to safe expansion should always be used.
Unfortunately, gitweb is living in 1995 in terms of web frameworks.

-Peff

On Mon, Nov 12, 2012 at 03:24:13PM -0500, Jeff King wrote:

I think the right answer is going to be a well-placed call to esc_html.

I'm guessing the right answer is this:

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);

but I did not test it (and I am not that familiar with gitweb, so it is
a slight guess from spending 5 minutes grepping and reading).

-Peff

-----Original Message-----
From: Drew Northup
Sent: Monday, November 12, 2012 1:56 PM

On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron [off-list ref]
wrote:

quoted

Gitweb can be used to generate an RSS feed.

Arbitrary tags can be inserted into the XML document describing
the RSS feed by careful construction of the URL.

Example

http://server/?p=project.git&a=rss&f=</title><script>alert(document.coo
kie)</script><title>

quoted

The generated XML contains
<script>alert(document.cookie)</script>

This is just an example.

quoted

Depending on the system used to render the XML this might lead
to the execution of javascript in the security context of the
gitweb server pages.

Please, escape all URL parameters.

We should look for the general entry points, not the script tag.

### not real perl...
foreach $xml in ( <, >, &, ...) 
{
  $input=~s/$xml/xmlescape{$xml}/g;
}

### "<" => "&lt;"

+       if ($input =~ m!(<script>)(.*)(</script>)!){
+               return undef;
+       }
        return $input;
 }

On Mon, Nov 12, 2012 at 3:24 PM, Jeff King [off-list ref] wrote:

On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:

quoted

On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron [off-list ref] wrote:

quoted

Gitweb can be used to generate an RSS feed.

Arbitrary tags can be inserted into the XML document describing
the RSS feed by careful construction of the URL.

[...]
Something like this may be useful to defuse the "file" parameter, but
I presume a more definitive fix is in order...

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 10ed9e5..af93e65 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -1447,6 +1447,10 @@ sub validate_pathname {
        if ($input =~ m!\0!) {
                return undef;
        }
+       # No XSS <script></script> inclusions
+       if ($input =~ m!(<script>)(.*)(</script>)!){
+               return undef;
+       }
        return $input;
 }

This is the wrong fix for a few reasons:

  1. It is on the input-validation side, whereas the real problem is on
     the output-quoting side. Your patch means I could not access a file
     called "<script>foo</script>". What we really want is to have the
     unquoted name internally, but then make sure we quote it when
     outputting as part of an HTML (or XML) file.

I don't buy the argument that we don't need to clean up the input as
well. There are scant few of us that are going to name a file
"<script>alert("Something Awful")</script>" in this world (I am
probably one of them). Input validation is key to keeping problems
like this from coming up repeatedly as those writing the guts of
programs are typically more interested in getting the "assigned task"
done and reporting the output to the user in a safe manner.

  2. Script tags are only part of the problem. They are what make it
     obviously a security vulnerability, but it is equally incorrect for
     us to show the filename "<b>foo</b>" as bold. I would also not be
     surprised if there are other cross-site attacks one can do without
     using <script>.

Yes, there are. You are typically concerned with anything including
the following:
(1) Executable stuff;
(2) Out of nowhere resources that can reference executable stuff
(style / CSS, iframe, script includes);
(3) Media and other things that activate browser plugins directly.

  3. Your filter is too simplistic. At the very least, it would not
     filter out "<SCRIPT>". I am not up to date on all of the
     sneaking-around-HTML-filters attacks that are available these days,
     but I wonder if one could also get around it using XML entities or
     similar.

You will note that I said "a more definitive fix is in order" in my
original. In other words, I claimed it to be utterly incomplete to
start with. I wanted to get some thought going about input validation
(in particular since I am not a perl guru of any sort whatsoever--the
fair number of things I've written from scratch or mangled into shape
notwithstanding).

I think the right answer is going to be a well-placed call to esc_html.
This already happens automatically when we go through the CGI
element-building functions, but obviously we failed to make the call
when building the output manually.  This is a great reason why template
languages which default to safe expansion should always be used.
Unfortunately, gitweb is living in 1995 in terms of web frameworks.

Escaping the output protects the user, but it DOES NOT protect the
server. We MUST handle both possibilities.
Besides, inserting one call to esc_html only fixes one attack path. I
didn't look to see if all others were already covered.

-- 
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

On Tue, Nov 13, 2012 at 3:44 PM, Drew Northup [off-list ref] wrote:

On Mon, Nov 12, 2012 at 3:24 PM, Jeff King [off-list ref] wrote:

quoted

On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:

quoted

quoted

+       # No XSS <script></script> inclusions
+       if ($input =~ m!(<script>)(.*)(</script>)!){
+               return undef;
+       }

quoted

This is the wrong fix for a few reasons:

  1. It is on the input-validation side, whereas the real problem is on
     the output-quoting side. Your patch means I could not access a file
     called "<script>foo</script>". What we really want is to have the
     unquoted name internally, but then make sure we quote it when
     outputting as part of an HTML (or XML) file.

I don't buy the argument that we don't need to clean up the input as
well. There are scant few of us that are going to name a file
"<script>alert("Something Awful")</script>" in this world (I am
probably one of them). Input validation is key to keeping problems
like this from coming up repeatedly as those writing the guts of
programs are typically more interested in getting the "assigned task"
done and reporting the output to the user in a safe manner.

Input cleanup or blacklisting *does not* prevent code injection (XSS
in this case). This is a myth.

Input validation has its place, and is done by gitweb when possible
(see e.g. evaluate_and_validate_params, validate_project, etc.).

But the proposed solution is not input validation.
'<script>alert("Something Awful")</script>' is a perfectly valid filename.
As is more realistic "<<create>>.uml" or "File > Open screenshot.png".

And last and most important you have to escape output anyway;
filename is not HTML. Without escaping it would be rendered incorrectly.
And HTML escaping prevents XSS.

quoted

I think the right answer is going to be a well-placed call to esc_html.
This already happens automatically when we go through the CGI
element-building functions, but obviously we failed to make the call
when building the output manually.  This is a great reason why template
languages which default to safe expansion should always be used.
Unfortunately, gitweb is living in 1995 in terms of web frameworks.

Escaping the output protects the user, but it DOES NOT protect the
server. We MUST handle both possibilities.

Errr, what?

If you are thinking about shell injection, we are covered.
Gitweb uses list form of open which is for shell what prepared
statements are for SQL. In one or two cases where we need to
use pipe we do shell escaping.

Besides, inserting one call to esc_html only fixes one attack path. I
didn't look to see if all others were already covered.

They should be covered. This case slipped.

-- 
Jakub Narebski

The problem with input filtering is that you can only filter for one
output scenario. What if the the input is going to be output in a wiki
like environment, or to pdf, or whatever? Then you have to unescape
the data again, and maybe apply filtering/escaping for those
environments.

You only know how to escape data when you are going to output it, so
then is the the best moment to escape it.

On Tue, Nov 13, 2012 at 4:45 PM, Kevin [off-list ref] wrote:

The problem with input filtering is that you can only filter for one
output scenario. What if the the input is going to be output in a wiki
like environment, or to pdf, or whatever? Then you have to unescape
the data again, and maybe apply filtering/escaping for those
environments.

You only know how to escape data when you are going to output it, so
then is the the best moment to escape it.

Also there are so many ways to evade XSS filtering

  https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet

If you can and should escape data (like in our case), it cannot not work;
not possible to evade it.
-- 
Jakub Narebski

On Tue, Nov 13, 2012 at 09:44:06AM -0500, Drew Northup wrote:

I don't buy the argument that we don't need to clean up the input as
well. There are scant few of us that are going to name a file
"<script>alert("Something Awful")</script>" in this world (I am
probably one of them). Input validation is key to keeping problems
like this from coming up repeatedly as those writing the guts of
programs are typically more interested in getting the "assigned task"
done and reporting the output to the user in a safe manner.

Oh, you absolutely do need to clean up the input side. And we do. Notice
how validate_pathname cleans out dots that could allow an attacker to do
a "../../etc/passwd" attack. But the input validation is _different_
than the output escaping. We are turning arbitrary junk from the user
into something we know is safe to treat as a filename. Our goal is
protecting the filesystem and the server, and we do that already.
Protecting the browser on output is a different problem, and happens
only when we are sending to the browser.

As far as "people will not use <script>" in their filenames, the
end-game to any quoting or blacklist fix is that we need to escape or
black _all_ HTML.  Because whether it is "<b>" or "<script>", it is
still wrong. Are you as comfortable saying that nobody will ever have a
"<" or "&" in their filename?

quoted

  3. Your filter is too simplistic. At the very least, it would not
     filter out "<SCRIPT>". I am not up to date on all of the
     sneaking-around-HTML-filters attacks that are available these days,
     but I wonder if one could also get around it using XML entities or
     similar.

You will note that I said "a more definitive fix is in order" in my
original. In other words, I claimed it to be utterly incomplete to
start with.

Sorry if I came off as too harsh. My intent was to guide you in the
right direction for the definitive fix. The fact that I ended up rolling
the patch myself was just because my "probably something like this"
ended with everybody saying "yeah, that", and it seemed simpler to just
roll a test and be done.

quoted

I think the right answer is going to be a well-placed call to esc_html.
This already happens automatically when we go through the CGI
element-building functions, but obviously we failed to make the call
when building the output manually.  This is a great reason why template
languages which default to safe expansion should always be used.
Unfortunately, gitweb is living in 1995 in terms of web frameworks.

Escaping the output protects the user, but it DOES NOT protect the
server. We MUST handle both possibilities.

Right. But I think we already do, via validate_pathname. If that is not
the case, please point it out.

Besides, inserting one call to esc_html only fixes one attack path. I
didn't look to see if all others were already covered.

Properly quoting output is something that the web framework should do
for you. gitweb uses CGI.pm, which does help with that, but we do not
use it consistently. If there are other problematic areas, I think the
best path forward is to use our framework more.

-Peff

On Tue, Nov 13, 2012 at 6:04 PM, Jeff King [off-list ref] wrote:

On Tue, Nov 13, 2012 at 09:44:06AM -0500, Drew Northup wrote:

quoted

Besides, inserting one call to esc_html only fixes one attack path. I
didn't look to see if all others were already covered.

Properly quoting output is something that the web framework should do
for you. gitweb uses CGI.pm, which does help with that, but we do not
use it consistently. If there are other problematic areas, I think the
best path forward is to use our framework more.

Well, calling CGI.pm a _framework_ is overly generous, but it does
include some HTML generation subroutines / methods, and gitweb
makes use of them, especially $cgi->a() for links.

But it cannot help in this case, because here we are generating XML:
RSS or Atom feed. There was proposal some time ago to switch
to using XML::FeedPP or XML::Atom::Feed + XML::RSS::Feed for
feed generation.

Perhaps it is high time to switch to some Perl web (micro)framework,
like Dancer, Mojolicious or Catalyst... but not requiring extra modules
has its advantages (and there always exist Gitalist).
-- 
Jakub Narebski