* [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 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
* 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; } ### "<" => "<" > + 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
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).