[PATCH] gitweb: Fix usability of $prevent_xss

[PATCH] gitweb: Fix usability of $prevent_xss

[Jakub Narebski]

With XSS prevention on (enabled using $prevent_xss), blobs
('blob_plain') of all types except a few known safe ones are served
with "Content-Disposition: attachment".  However the check was too
strict; it didn't take into account optional parameter attributes,

  media-type     = type "/" subtype *( ";" parameter )

as described in RFC 2616

  http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.17
  http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7

This fixes that, and it for example treats following as safe MIME
media type:

  text/plain; charset=utf-8

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The fact that it this buglet was present for so long, since its
introduction by Matt McCutchen in 7e1100e (gitweb: add $prevent_xss
option to prevent XSS by repository content, 2009-02-07) without
complaint shows that not many people are using this feature...

That, and that we don't have automated tests for that.

 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dc3f37d..85acbed 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6139,7 +6139,7 @@ sub git_blob_plain {
 	# want to be sure not to break that by serving the image as an
 	# attachment (though Firefox 3 doesn't seem to care).
 	my $sandbox = $prevent_xss &&
-		$type !~ m!^(?:text/plain|image/(?:gif|png|jpeg))$!;
+		$type !~ m!^(?:text/plain|image/(?:gif|png|jpeg))(?:[ ;]|$)!;
 
 	print $cgi->header(
 		-type => $type,
-- 
1.7.5

Prevalence of $prevent_xss

[Matt McCutchen]

On Sat, 2011-06-04 at 10:43 +0200, Jakub Narebski wrote:
> The fact that it this buglet was present for so long, since its
> introduction by Matt McCutchen in 7e1100e (gitweb: add $prevent_xss
> option to prevent XSS by repository content, 2009-02-07) without
> complaint shows that not many people are using this feature...

Yes.  Well, I'm still using it, and I found a few mentions on the web:

https://android.git.kernel.org/?p=tools/gerrit.git;a=blob;f=gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java;h=947fbb423f1f8cf46db9876f4b80c600cdf9ee41;hb=HEAD#l193
http://ao2.it/wiki/How_to_setup_a_GIT_server_with_gitosis_and_gitweb
http://www.digitalfoo.net/posts/2009/11/git,_gitosis,_gitweb_on_FreeBSD/

And there are probably others who did their own custom things (GitHub?)
before the feature was added upstream.

-- 
Matt

Re: Prevalence of $prevent_xss

[Jakub Narebski]

On Sat, 4 Jul 2011, Matt McCutchen wrote:
> On Sat, 2011-06-04 at 10:43 +0200, Jakub Narebski wrote:

> > The fact that it this buglet was present for so long, since its
> > introduction by Matt McCutchen in 7e1100e (gitweb: add $prevent_xss
> > option to prevent XSS by repository content, 2009-02-07) without
> > complaint shows that not many people are using this feature...

Well, and the fact that it is just minor issue (and might not be visible
at all if there is mime.types file, and text files do use mime.types
extensions).

> Yes.  Well, I'm still using it, and I found a few mentions on the web:
> 
> https://android.git.kernel.org/?p=tools/gerrit.git;a=blob;f=gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java;h=947fbb423f1f8cf46db9876f4b80c600cdf9ee41;hb=HEAD#l193
> http://ao2.it/wiki/How_to_setup_a_GIT_server_with_gitosis_and_gitweb
> http://www.digitalfoo.net/posts/2009/11/git,_gitosis,_gitweb_on_FreeBSD/

Thanks for research.  Nice to know.
 
> And there are probably others who did their own custom things (GitHub?)
> before the feature was added upstream.

GitHub does not use gitweb, but its own [integrated] custom web interface.

-- 
Jakub Narebski
Poland

Implementing CSP (Content Security Policy) for gitweb in the future

[Jakub Narebski]

On Sat, 4 Jul 2011, Matt McCutchen qrote:
> On Sat, 2011-06-04 at 10:43 +0200, Jakub Narebski wrote:

> > The fact that it this buglet was present for so long, since its
> > introduction by Matt McCutchen in 7e1100e (gitweb: add $prevent_xss
> > option to prevent XSS by repository content, 2009-02-07) without
> > complaint shows that not many people are using this feature...
> 
> Yes.  Well, I'm still using it, and I found a few mentions on the web:
> 
> https://android.git.kernel.org/?p=tools/gerrit.git;a=blob;f=gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java;h=947fbb423f1f8cf46db9876f4b80c600cdf9ee41;hb=HEAD#l193
> http://ao2.it/wiki/How_to_setup_a_GIT_server_with_gitosis_and_gitweb
> http://www.digitalfoo.net/posts/2009/11/git,_gitosis,_gitweb_on_FreeBSD/
> 
> And there are probably others who did their own custom things (GitHub?)
> before the feature was added upstream.

In the future however it might be better solution for gitweb to implement
(as an option) support for CSP (Content Security Policy), which IIRC did
not exists in 2009, in addition to current $prevent_xss.

-- 
Jakub Narebski
Poland

Re: Implementing CSP (Content Security Policy) for gitweb in the future

[Matt McCutchen]

On Sun, 2011-06-05 at 11:03 +0200, Jakub Narebski wrote:
> In the future however it might be better solution for gitweb to implement
> (as an option) support for CSP (Content Security Policy), which IIRC did
> not exists in 2009, in addition to current $prevent_xss.

Sure.  CSP is not a substitute for designing to prevent harmful HTML
injection, but a mitigation for some of its worst effects in case some
injection points are overlooked.  There's no reason not to enable it by
default with $prevent_xss, though third parties adding functionality to
gitweb would need to know to disable it or modify the policy
accordingly.

-- 
Matt

Re: Implementing CSP (Content Security Policy) for gitweb in the future

[Jakub Narebski]

On Sun, 5 July 2011, Matt McCutchen wrote:
> On Sun, 2011-06-05 at 11:03 +0200, Jakub Narebski wrote:

> > In the future however it might be better solution for gitweb to implement
> > (as an option) support for CSP (Content Security Policy), which IIRC did
> > not exists in 2009, in addition to current $prevent_xss.
> 
> Sure.  CSP is not a substitute for designing to prevent harmful HTML
> injection, but a mitigation for some of its worst effects in case some
> injection points are overlooked.  There's no reason not to enable it by
> default with $prevent_xss, though third parties adding functionality to
> gitweb would need to know to disable it or modify the policy
> accordingly.

I propose CSP support _in addition to_ and not replacing $prevent_xss
(which would be nice to have more fine-grained control over).

Well, while we can whitelist HTML fragment from README.html, or render
README.md / README.rs / README.pod etc. instead of blocking it like gitweb
currently does if $prevent_xss is enabled, I don't think it would be
feasible to do the same for text/html 'blob_plain' pages. 

Serving HTML pages etc. from 'blob_plain' view with path_info links
is quite useful feature; this way one can use gitweb as a cheap and easy
way to deploy web pages and web apps; or just test results of development.
CSP would serve this purpose well; current $prevent_xss behavior of
serving as attachment (forcing download), or serving them as text/plain
as e.g. GitHub does simply remove this feature.

-- 
Jakub Narebski
Poland

Re: Implementing CSP (Content Security Policy) for gitweb in the future

[Matt McCutchen]

On Sun, 2011-06-05 at 15:33 +0200, Jakub Narebski wrote:
> On Sun, 5 July 2011, Matt McCutchen wrote:
> > On Sun, 2011-06-05 at 11:03 +0200, Jakub Narebski wrote:
> 
> > > In the future however it might be better solution for gitweb to implement
> > > (as an option) support for CSP (Content Security Policy), which IIRC did
> > > not exists in 2009, in addition to current $prevent_xss.
> > 
> > Sure.  CSP is not a substitute for designing to prevent harmful HTML
> > injection, but a mitigation for some of its worst effects in case some
> > injection points are overlooked.  There's no reason not to enable it by
> > default with $prevent_xss, though third parties adding functionality to
> > gitweb would need to know to disable it or modify the policy
> > accordingly.
> 
> I propose CSP support _in addition to_ and not replacing $prevent_xss
> (which would be nice to have more fine-grained control over).
> 
> Well, while we can whitelist HTML fragment from README.html, or render
> README.md / README.rs / README.pod etc. instead of blocking it like gitweb
> currently does if $prevent_xss is enabled, I don't think it would be
> feasible to do the same for text/html 'blob_plain' pages. 
> 
> Serving HTML pages etc. from 'blob_plain' view with path_info links
> is quite useful feature; this way one can use gitweb as a cheap and easy
> way to deploy web pages

Yes.

> and web apps;

Probably not: the browser features needed to make a nontrivial web app
are probably the same ones that are dangerous to other web apps.

> or just test results of development.
> CSP would serve this purpose well; current $prevent_xss behavior of
> serving as attachment (forcing download), or serving them as text/plain
> as e.g. GitHub does simply remove this feature.

CSP is not intended to be used by itself as a sandbox, although it might
almost work for the purpose.  It would be more appropriate to set up a
wildcard virtual host and appropriate rewrite rules to expose each
repository at a different DNS name and take advantage of the usual
same-origin policy.

-- 
Matt

Re: Implementing CSP (Content Security Policy) for gitweb in the future

[Jakub Narebski]

On Sun 5 June 2011 Matt McCutchen wrote:
> On Sun 2011-06-05 at 15:33 +0200 Jakub Narebski wrote:
>> On Sun 5 July 2011 Matt McCutchen wrote:
>>> On Sun 2011-06-05 at 11:03 +0200 Jakub Narebski wrote:
>> 
>>>> In the future however it might be better solution for gitweb to implement
>>>> (as an option) support for CSP (Content Security Policy) which IIRC did
>>>> not exists in 2009 in addition to current $prevent_xss.
>>> 
>>> Sure. CSP is not a substitute for designing to prevent harmful HTML
>>> injection but a mitigation for some of its worst effects in case some
>>> injection points are overlooked. There's no reason not to enable it by
>>> default with $prevent_xss though third parties adding functionality to
>>> gitweb would need to know to disable it or modify the policy
>>> accordingly.
>> 
>> I propose CSP support _in addition to_ and not replacing $prevent_xss
>> (which would be nice to have more fine-grained control over).
>> 
>> Well while we can whitelist HTML fragment from README.html or render
>> README.md / README.rs / README.pod etc. instead of blocking it like gitweb
>> currently does if $prevent_xss is enabled I don't think it would be
>> feasible to do the same for text/html 'blob_plain' pages. 
>> 
>> Serving HTML pages etc. from 'blob_plain' view with path_info links
>> is quite useful feature; this way one can use gitweb as a cheap and easy
>> way to deploy web pages
> 
> Yes.
> 
>> and web apps;
> 
> Probably not: the browser features needed to make a nontrivial web app
> are probably the same ones that are dangerous to other web apps.

"Deploying" with gitweb doesn't allow for server-side scripting, so it
is "web apps" only as far as there can be web application done entirely
on client-side: HTML or HTML5 + JavaScript.  Well, there is demo of a
game played in HTML5+JavaScript played entirely in URL bar ;-)

With CSP you would be restricted to prerequisites (web page itself,
scripts, stylesheets, images) to be also hosted/deployed via gitweb.

What features would non server-side nontrivial web app need that would
be dangerous to other web apps?

>> or just test results of development.
>> CSP would serve this purpose well; current $prevent_xss behavior of
>> serving as attachment (forcing download) or serving them as text/plain
>> as e.g. GitHub does simply remove this feature.
> 
> CSP is not intended to be used by itself as a sandbox although it might
> almost work for the purpose. It would be more appropriate to set up a
> wildcard virtual host and appropriate rewrite rules to expose each
> repository at a different DNS name and take advantage of the usual
> same-origin policy.

Could this virtualhost + DNS + same-origin sandboxing be used for gitweb?
If not, then perhaps it is better solution in other cases, but not for
gitweb.


P.S. I don't know how difficult implementing CSP support for gitweb would
be, given that gitweb is quite configurable wrt. external resources it
uses: $javascript, @stylesheets, various *logo variables...

-- 
Jakub Narebski
Poland

Re: Implementing CSP (Content Security Policy) for gitweb in the future

[J.H.]

On 06/08/2011 03:27 AM, Jakub Narebski wrote:
> On Sun 5 June 2011 Matt McCutchen wrote:
>> On Sun 2011-06-05 at 15:33 +0200 Jakub Narebski wrote:
>>> On Sun 5 July 2011 Matt McCutchen wrote:
>>>> On Sun 2011-06-05 at 11:03 +0200 Jakub Narebski wrote:
>>>
>>>>> In the future however it might be better solution for gitweb to implement
>>>>> (as an option) support for CSP (Content Security Policy) which IIRC did
>>>>> not exists in 2009 in addition to current $prevent_xss.
>>>>
>>>> Sure. CSP is not a substitute for designing to prevent harmful HTML
>>>> injection but a mitigation for some of its worst effects in case some
>>>> injection points are overlooked. There's no reason not to enable it by
>>>> default with $prevent_xss though third parties adding functionality to
>>>> gitweb would need to know to disable it or modify the policy
>>>> accordingly.
>>>
>>> I propose CSP support _in addition to_ and not replacing $prevent_xss
>>> (which would be nice to have more fine-grained control over).
>>>
>>> Well while we can whitelist HTML fragment from README.html or render
>>> README.md / README.rs / README.pod etc. instead of blocking it like gitweb
>>> currently does if $prevent_xss is enabled I don't think it would be
>>> feasible to do the same for text/html 'blob_plain' pages. 
>>>
>>> Serving HTML pages etc. from 'blob_plain' view with path_info links
>>> is quite useful feature; this way one can use gitweb as a cheap and easy
>>> way to deploy web pages
>>
>> Yes.
>>
>>> and web apps;
>>
>> Probably not: the browser features needed to make a nontrivial web app
>> are probably the same ones that are dangerous to other web apps.
> 
> "Deploying" with gitweb doesn't allow for server-side scripting, so it
> is "web apps" only as far as there can be web application done entirely
> on client-side: HTML or HTML5 + JavaScript.  Well, there is demo of a
> game played in HTML5+JavaScript played entirely in URL bar ;-)
> 
> With CSP you would be restricted to prerequisites (web page itself,
> scripts, stylesheets, images) to be also hosted/deployed via gitweb.
> 
> What features would non server-side nontrivial web app need that would
> be dangerous to other web apps?
> 
>>> or just test results of development.
>>> CSP would serve this purpose well; current $prevent_xss behavior of
>>> serving as attachment (forcing download) or serving them as text/plain
>>> as e.g. GitHub does simply remove this feature.
>>
>> CSP is not intended to be used by itself as a sandbox although it might
>> almost work for the purpose. It would be more appropriate to set up a
>> wildcard virtual host and appropriate rewrite rules to expose each
>> repository at a different DNS name and take advantage of the usual
>> same-origin policy.
> 
> Could this virtualhost + DNS + same-origin sandboxing be used for gitweb?
> If not, then perhaps it is better solution in other cases, but not for
> gitweb.
> 
> 
> P.S. I don't know how difficult implementing CSP support for gitweb would
> be, given that gitweb is quite configurable wrt. external resources it
> uses: $javascript, @stylesheets, various *logo variables...
> 

The long and the short of it is, until browsers other than
Mozilla/Firefox support CSP, it's a dead standard and we shouldn't spend
any time on it.  There are few enough resources on gitweb as it is to be
tilting at windmills.

- John 'Warthog9' Hawley

[PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable

[Jakub Narebski]

One of mechanism enabled by setting $prevent_xss to true is 'blob_plain'
view protection.  With XSS prevention on, blobs of all types except a
few known safe ones are served with "Content-Disposition: attachment" to
make sure they don't run in our security domain.

Instead of serving text/* type files, except text/plain (and including
text/html), as attachements, downgrade it to text/plain.  This way HTML
pages in 'blob_plain' (raw) wiew would be displayed in browser, but
safely as a source, and not asked to be saved.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is what GitHub interface does for text/html pages (*.html), if
I remember it correctly...

 gitweb/gitweb.perl |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 85acbed..470793a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6139,7 +6139,15 @@ sub git_blob_plain {
 	# want to be sure not to break that by serving the image as an
 	# attachment (though Firefox 3 doesn't seem to care).
 	my $sandbox = $prevent_xss &&
-		$type !~ m!^(?:text/plain|image/(?:gif|png|jpeg))(?:[ ;]|$)!;
+		$type !~ m!^(?:text/[a-z]+|image/(?:gif|png|jpeg))(?:[ ;]|$)!;
+
+	# serve text/* as text/plain
+	if ($prevent_xss &&
+	    $type =~ m!^text/([a-z]+)\b(.*)$!) {
+		my ($subtype, $rest) = ($1, $2);
+		$rest = defined $rest ? $rest : '';
+		$type = "text/plain$rest" if ($subtype ne 'plain');
+	}
 
 	print $cgi->header(
 		-type => $type,
-- 
1.7.5

Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> +	# serve text/* as text/plain
> +	if ($prevent_xss &&
> +	    $type =~ m!^text/([a-z]+)\b(.*)$!) {
> +		my ($subtype, $rest) = ($1, $2);
> +		$rest = defined $rest ? $rest : '';
> +		$type = "text/plain$rest" if ($subtype ne 'plain');

Hmph, wouldn't it be more straightforward if you dropped the statement
modifier?  I.e.

	my ($subtype, $rest) = ($1, $2);
        $rest = '' unless defined $rest;
        $type = "text/plain$rest";

Other than that, looks good to me.

Thanks.

Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > +	# serve text/* as text/plain
> > +	if ($prevent_xss &&
> > +	    $type =~ m!^text/([a-z]+)\b(.*)$!) {
> > +		my ($subtype, $rest) = ($1, $2);
> > +		$rest = defined $rest ? $rest : '';
> > +		$type = "text/plain$rest" if ($subtype ne 'plain');
> 
> Hmph, wouldn't it be more straightforward if you dropped the statement
> modifier?  I.e.
> 
> 	my ($subtype, $rest) = ($1, $2);
> 	$rest = '' unless defined $rest;
> 	$type = "text/plain$rest";

Yes, of course.

I don't know why I decided that avoiding rewriting 'text/plain; 
charset=utf-8' case was important.  It cretainly is not worth making 
code harder to follow.
 
Can you fix it during applying, or should I resend it?

> Other than that, looks good to me.

Thanks.
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> Hmph, wouldn't it be more straightforward if you dropped the statement
>> modifier?  I.e.
>> 
>> 	my ($subtype, $rest) = ($1, $2);
>> 	$rest = '' unless defined $rest;
>> 	$type = "text/plain$rest";
>
> Yes, of course.
>
> I don't know why I decided that avoiding rewriting 'text/plain; 
> charset=utf-8' case was important.

Just to make sure I understand what you are saying...

    my $type = 'text/plain; charset=utf-8';
    if ($type =~ m|^text/([a-z]+)\b(.*)$|) {
 	my ($subtype, $rest) = ($1, $2);
 	$rest = '' unless defined $rest;
 	$type = "text/plain$rest";
        print "Type is now <$type>\n";
    }


does yield "text/plain; charset=utf-8". It does rewrite but rewrite to
exactly the same thing, so...

Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable

[Jakub Narebski]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>>> Hmph, wouldn't it be more straightforward if you dropped the statement
>>> modifier?  I.e.
>>> 
>>> 	my ($subtype, $rest) = ($1, $2);
>>> 	$rest = '' unless defined $rest;
>>> 	$type = "text/plain$rest";
>>
>> Yes, of course.
>>
>> I don't know why I decided that avoiding rewriting 'text/plain; 
>> charset=utf-8' case was important.
> 
> Just to make sure I understand what you are saying...
> 
>     my $type = 'text/plain; charset=utf-8';
>     if ($type =~ m|^text/([a-z]+)\b(.*)$|) {
>  	my ($subtype, $rest) = ($1, $2);
>  	$rest = '' unless defined $rest;
>  	$type = "text/plain$rest";
>         print "Type is now <$type>\n";
>     }
> 
> 
> does yield "text/plain; charset=utf-8". It does rewrite but rewrite to
> exactly the same thing, so...

Yes, it does rewrite to the same thing.  And the code is simpler,
therefore better. 

-- 
Jakub Narebski
Poland