gitweb: Fix usability of $prevent_xss
PATCH 1/1 gitweb: Fix usability of $prevent_xss
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
---
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(-)
gitweb/gitweb.perl +1 -1
@@ -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,
Signature · 1 line
1.7.5
Prevalence of $prevent_xss
Quoted 4 lines · Jakub Narebski
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.
Signature · 1 line
Matt
Re: Prevalence of $prevent_xss
Quoted 6 lines · Matt McCutchen
Quoted 4 lines · Jakub Narebski
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).
Quoted 5 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.
Quoted 2 lines
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.
Signature · 2 lines
Jakub Narebski Poland
Implementing CSP (Content Security Policy) for gitweb in the future
In reply to Matt McCutchenOn Sat, 4 Jul 2011, Matt McCutchen qrote:
Quoted 7 lines · Jakub Narebski
Quoted 5 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...
Quoted 8 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/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.
Signature · 2 lines
Jakub Narebski Poland
Re: Implementing CSP (Content Security Policy) for gitweb in the future
Quoted 3 lines · Jakub Narebski
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.
Signature · 1 line
Matt
Re: Implementing CSP (Content Security Policy) for gitweb in the future
Quoted 6 lines · Matt McCutchen
Quoted 3 lines · Jakub Narebski
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.
Quoted 6 lines
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.
Signature · 2 lines
Jakub Narebski Poland
Re: Implementing CSP (Content Security Policy) for gitweb in the future
Quoted 17 lines · Jakub Narebski
Quoted 6 lines · Matt McCutchen
Quoted 3 lines · Jakub Narebski
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.
Quoted 6 lines
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
_inadditionto_and not replacing $prevent_xss (which would be nice to have more fine-grained control over).
Quoted 8 lines
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 withpath_infolinks is quite useful feature; this way one can use gitweb as a cheap and easy way to deploy web pages
Yes.
Quoted 1 line
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.
Quoted 4 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.
Signature · 1 line
Matt
Re: Implementing CSP (Content Security Policy) for gitweb in the future
Quoted 27 lines · On Sun 5 June 2011 Matt McCutchen wrote:
Quoted 17 lines · On Sun 2011-06-05 at 15:33 +0200 Jakub Narebski wrote:
Quoted 6 lines · On Sun 5 July 2011 Matt McCutchen wrote:
Quoted 3 lines · 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.
Quoted 6 lines
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
_inadditionto_and not replacing $prevent_xss (which would be nice to have more fine-grained control over).Quoted 8 lines
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 withpath_infolinks is quite useful feature; this way one can use gitweb as a cheap and easy way to deploy web pages
Quoted 6 lines
Yes.
Quoted 1 line
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?
Quoted 10 lines
Quoted 4 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...
Signature · 2 lines
Jakub Narebski Poland
Re: Implementing CSP (Content Security Policy) for gitweb in the future
Quoted 57 lines · Jakub Narebski
Quoted 27 lines · On Sun 5 June 2011 Matt McCutchen wrote:
Quoted 17 lines · On Sun 2011-06-05 at 15:33 +0200 Jakub Narebski wrote:
Quoted 6 lines · On Sun 5 July 2011 Matt McCutchen wrote:
Quoted 3 lines · 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.
Quoted 6 lines
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
_inadditionto_and not replacing $prevent_xss (which would be nice to have more fine-grained control over).Quoted 8 lines
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 withpath_infolinks is quite useful feature; this way one can use gitweb as a cheap and easy way to deploy web pagesQuoted 6 lines
Yes.
Quoted 1 line
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?
Quoted 10 lines
Quoted 4 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.
Quoted 9 lines
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 1/1 gitweb: Make $prevent_xss protection for 'blob_plain' more usable
In reply to Jakub NarebskiOne 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.
---
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(-)
gitweb/gitweb.perl +9 -1
@@ -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,
Signature · 1 line
1.7.5
Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable
gitweb/gitweb.perl sub git_blob_plain { · line 6149
Quoted 6 lines · Jakub Narebski
gitweb/gitweb.perl +6
-0+ # 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
Quoted 12 lines · Junio C Hamano
gitweb/gitweb.perl sub git_blob_plain { · line 6149
Quoted 6 lines · Jakub Narebski
gitweb/gitweb.perl +6
-0+ # 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.
Quoted 3 lines
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?
Quoted 1 line
Other than that, looks good to me.
Thanks.
Signature · 2 lines
Jakub Narebski Poland
Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable
Quoted 11 lines · Jakub Narebski
Quoted 6 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...
Re: [PATCH] gitweb: Make $prevent_xss protection for 'blob_plain' more usable
Quoted 25 lines · Junio C Hamano
Quoted 11 lines · Jakub Narebski
Quoted 6 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(.*)$|) {(quoted diff) +0
-0my ($subtype, $rest) = ($1, $2); $rest = '' unless defined $rest; $type = "text/plain$rest"; print "Type is now <$type>\n"; }
Quoted 2 lines
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.
Signature · 2 lines
Jakub Narebski Poland