threads / patch / 27545

patchgitweb: Fix usability of $prevent_xss

Subject: [PATCH] gitweb: Fix usability of $prevent_xss

## tl;dr

14 messages between Jun 4, 2011 and Jun 14, 2011. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Jakub Narebski· Jun 4, 2011, 08:43 UTC · lore

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(-)
Show changes to gitweb/gitweb.perl +1 −1
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
Matt McCutchen· Jun 4, 2011, 21:15 UTC · re: Jakub Narebski · lore

Prevalence of $prevent_xss

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
Jakub Narebski· Jun 4, 2011, 21:53 UTC · re: Matt McCutchen · lore

Re: Prevalence of $prevent_xss

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

Show 5 quoted lines
> 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
Jakub Narebski· Jun 5, 2011, 09:03 UTC · re: Matt McCutchen · lore

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

On Sat, 4 Jul 2011, Matt McCutchen qrote:
> On Sat, 2011-06-04 at 10:43 +0200, Jakub Narebski wrote:
Show 13 quoted lines
> > 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
Matt McCutchen· Jun 5, 2011, 12:52 UTC · re: Jakub Narebski · lore

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

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
Jakub Narebski· Jun 5, 2011, 13:33 UTC · re: Matt McCutchen · lore

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

On Sun, 5 July 2011, Matt McCutchen wrote:
> On Sun, 2011-06-05 at 11:03 +0200, Jakub Narebski wrote:
Show 10 quoted lines
> > 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
Matt McCutchen· Jun 5, 2011, 16:46 UTC · re: Jakub Narebski · lore

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

On Sun, 2011-06-05 at 15:33 +0200, Jakub Narebski wrote:
Show 25 quoted lines
> 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
Jakub Narebski· Jun 8, 2011, 10:27 UTC · re: Matt McCutchen · lore

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

On Sun 5 June 2011 Matt McCutchen wrote:
Show 33 quoted lines
> 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?

Show 10 quoted lines
>> 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
J.H.· Jun 8, 2011, 17:31 UTC · re: Jakub Narebski · lore

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

On 06/08/2011 03:27 AM, Jakub Narebski wrote:
Show 66 quoted lines
> 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
Jakub Narebski· Jun 10, 2011, 12:01 UTC · re: Jakub Narebski · lore

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

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(-)
Show changes to gitweb/gitweb.perl +9 −1
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
Junio C Hamano· Jun 13, 2011, 16:47 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 6 quoted lines
> +	# 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.
Jakub Narebski· Jun 13, 2011, 21:49 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 15 quoted lines
> 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
Junio C Hamano· Jun 13, 2011, 23:12 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 11 quoted lines
>> 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...

Jakub Narebski· Jun 14, 2011, 01:33 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 27 quoted lines
> 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

← back to recent threads