git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [BUG] gitweb: XSS vulnerability of RSS feed
@ 2012-11-11 23:28 glpk xypron
  2012-11-12 18:55 ` Drew Northup
  0 siblings, 1 reply; 15+ messages in thread
From: glpk xypron @ 2012-11-11 23:28 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-11 23:28 [BUG] gitweb: XSS vulnerability of RSS feed glpk xypron
@ 2012-11-12 18:55 ` Drew Northup
  2012-11-12 20:24   ` Jeff King
                     ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Drew Northup @ 2012-11-12 18:55 UTC (permalink / raw)
  To: glpk xypron; +Cc: git, jnareb, Junio C Hamano

On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de> 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
>> 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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-12 18:55 ` Drew Northup
@ 2012-11-12 20:24   ` Jeff King
  2012-11-12 20:27     ` Jeff King
  2012-11-13 14:44     ` Drew Northup
  2012-11-12 23:09   ` Andreas Schwab
  2012-11-13  8:31   ` Pyeron, Jason J CTR (US)
  2 siblings, 2 replies; 15+ messages in thread
From: Jeff King @ 2012-11-12 20:24 UTC (permalink / raw)
  To: Drew Northup; +Cc: glpk xypron, git, jnareb, Junio C Hamano

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

> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de> 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.
> [...]
> 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.

  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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-12 20:24   ` Jeff King
@ 2012-11-12 20:27     ` Jeff King
  2012-11-12 20:36       ` Junio C Hamano
  2012-11-13 14:44     ` Drew Northup
  1 sibling, 1 reply; 15+ messages in thread
From: Jeff King @ 2012-11-12 20:27 UTC (permalink / raw)
  To: Drew Northup; +Cc: glpk xypron, git, jnareb, Junio C Hamano

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-12 20:27     ` Jeff King
@ 2012-11-12 20:36       ` Junio C Hamano
  2012-11-12 21:13         ` Jakub Narębski
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2012-11-12 20:36 UTC (permalink / raw)
  To: Jeff King; +Cc: Drew Northup, glpk xypron, git, jnareb

Jeff King <peff@peff.net> writes:

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

Yeah, that looks correct, given the way how the other variables
emitted with the same "print" like $descr and $owner are formed.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-12 20:36       ` Junio C Hamano
@ 2012-11-12 21:13         ` Jakub Narębski
  2012-11-12 21:34           ` Jeff King
  0 siblings, 1 reply; 15+ messages in thread
From: Jakub Narębski @ 2012-11-12 21:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Drew Northup, glpk xypron, git

On Mon, Nov 12, 2012 at 9:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> 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).
>
> 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).

-- 
Jakub Narębski

-- 
Jakub Narebski

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-12 21:13         ` Jakub Narębski
@ 2012-11-12 21:34           ` Jeff King
  0 siblings, 0 replies; 15+ messages in thread
From: Jeff King @ 2012-11-12 21:34 UTC (permalink / raw)
  To: Jakub Narębski; +Cc: Junio C Hamano, Drew Northup, glpk xypron, git

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-12 18:55 ` Drew Northup
  2012-11-12 20:24   ` Jeff King
@ 2012-11-12 23:09   ` Andreas Schwab
  2012-11-13  8:31   ` Pyeron, Jason J CTR (US)
  2 siblings, 0 replies; 15+ messages in thread
From: Andreas Schwab @ 2012-11-12 23:09 UTC (permalink / raw)
  To: Drew Northup; +Cc: glpk xypron, git, jnareb, Junio C Hamano

Drew Northup <n1xim.email@gmail.com> writes:

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

A proper fix will have to add esc_html to the feed generation, something
like this (untested):

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


Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* RE: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-12 18:55 ` Drew Northup
  2012-11-12 20:24   ` Jeff King
  2012-11-12 23:09   ` Andreas Schwab
@ 2012-11-13  8:31   ` Pyeron, Jason J CTR (US)
  2 siblings, 0 replies; 15+ messages in thread
From: Pyeron, Jason J CTR (US) @ 2012-11-13  8:31 UTC (permalink / raw)
  To: git@vger.kernel.org

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

> -----Original Message-----
> From: Drew Northup
> Sent: Monday, November 12, 2012 1:56 PM
> 
> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de>
> 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.coo
> kie)</script><title>
> >
> > The generated XML contains
> > <script>alert(document.cookie)</script>

This is just an example.


> >
> > 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.


> >
> > Version tested:
> > gitweb v.1.8.0.dirty with git 1.7.2.5
> >
> > Best regards
> >> 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

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

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

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


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5615 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-12 20:24   ` Jeff King
  2012-11-12 20:27     ` Jeff King
@ 2012-11-13 14:44     ` Drew Northup
  2012-11-13 15:19       ` Jakub Narębski
                         ` (2 more replies)
  1 sibling, 3 replies; 15+ messages in thread
From: Drew Northup @ 2012-11-13 14:44 UTC (permalink / raw)
  To: Jeff King
  Cc: glpk xypron, git, jnareb, Junio C Hamano, Jason J Pyeron CTR (US),
	Andreas Schwab

On Mon, Nov 12, 2012 at 3:24 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:
>
>> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de> 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.
>> [...]
>> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-13 14:44     ` Drew Northup
@ 2012-11-13 15:19       ` Jakub Narębski
  2012-11-13 15:45       ` Kevin
  2012-11-13 17:04       ` Jeff King
  2 siblings, 0 replies; 15+ messages in thread
From: Jakub Narębski @ 2012-11-13 15:19 UTC (permalink / raw)
  To: Drew Northup
  Cc: Jeff King, glpk xypron, git, Junio C Hamano,
	Jason J Pyeron CTR (US), Andreas Schwab

On Tue, Nov 13, 2012 at 3:44 PM, Drew Northup <n1xim.email@gmail.com> wrote:
> On Mon, Nov 12, 2012 at 3:24 PM, Jeff King <peff@peff.net> wrote:
>> On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:

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

>> 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.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-13 14:44     ` Drew Northup
  2012-11-13 15:19       ` Jakub Narębski
@ 2012-11-13 15:45       ` Kevin
  2012-11-13 15:57         ` Jakub Narębski
  2012-11-13 17:04       ` Jeff King
  2 siblings, 1 reply; 15+ messages in thread
From: Kevin @ 2012-11-13 15:45 UTC (permalink / raw)
  To: Drew Northup
  Cc: Jeff King, glpk xypron, git, Jakub Narębski, Junio C Hamano,
	Jason J Pyeron CTR (US), Andreas Schwab

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-13 15:45       ` Kevin
@ 2012-11-13 15:57         ` Jakub Narębski
  0 siblings, 0 replies; 15+ messages in thread
From: Jakub Narębski @ 2012-11-13 15:57 UTC (permalink / raw)
  To: Kevin
  Cc: Drew Northup, Jeff King, glpk xypron, git, Junio C Hamano,
	Jason J Pyeron CTR (US), Andreas Schwab

On Tue, Nov 13, 2012 at 4:45 PM, Kevin <ikke@ikke.info> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-13 14:44     ` Drew Northup
  2012-11-13 15:19       ` Jakub Narębski
  2012-11-13 15:45       ` Kevin
@ 2012-11-13 17:04       ` Jeff King
  2012-11-13 17:22         ` Jakub Narębski
  2 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2012-11-13 17:04 UTC (permalink / raw)
  To: Drew Northup
  Cc: glpk xypron, git, jnareb, Junio C Hamano, Jason J Pyeron CTR (US),
	Andreas Schwab

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?

> >   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.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
  2012-11-13 17:04       ` Jeff King
@ 2012-11-13 17:22         ` Jakub Narębski
  0 siblings, 0 replies; 15+ messages in thread
From: Jakub Narębski @ 2012-11-13 17:22 UTC (permalink / raw)
  To: Jeff King
  Cc: Drew Northup, glpk xypron, git, Junio C Hamano,
	Jason J Pyeron CTR (US), Andreas Schwab

On Tue, Nov 13, 2012 at 6:04 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 13, 2012 at 09:44:06AM -0500, Drew Northup wrote:

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2012-11-13 17:23 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-11 23:28 [BUG] gitweb: XSS vulnerability of RSS feed glpk xypron
2012-11-12 18:55 ` Drew Northup
2012-11-12 20:24   ` Jeff King
2012-11-12 20:27     ` Jeff King
2012-11-12 20:36       ` Junio C Hamano
2012-11-12 21:13         ` Jakub Narębski
2012-11-12 21:34           ` Jeff King
2012-11-13 14:44     ` Drew Northup
2012-11-13 15:19       ` Jakub Narębski
2012-11-13 15:45       ` Kevin
2012-11-13 15:57         ` Jakub Narębski
2012-11-13 17:04       ` Jeff King
2012-11-13 17:22         ` Jakub Narębski
2012-11-12 23:09   ` Andreas Schwab
2012-11-13  8:31   ` Pyeron, Jason J CTR (US)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).