{"thread":{"id":"32083","subject":"[BUG] gitweb: XSS vulnerability of RSS feed","startedAt":"2012-11-11T23:28:20Z","lastAt":"2012-11-13T17:22:56Z","messageCount":15,"participants":["glpk xypron","Drew Northup","Jeff King","Junio C Hamano","Jakub Narębski","Andreas Schwab","Pyeron, Jason J CTR (US)","Kevin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"202931","messageId":"20121111232820.284510@gmx.net","threadId":"32083","inReplyTo":null,"subject":"[BUG] gitweb: XSS vulnerability of RSS feed","fromName":"glpk xypron","fromEmail":"xypron.glpk@gmx.de","sentAt":"2012-11-11T23:28:20Z","receivedAt":"2012-11-11T23:28:20Z","isPatch":false,"sender":{"key":"xypron.glpk@gmx.de","avatar":"https://gravatar.com/avatar/ecfc545fcbe89519e2ffdf4eccf04756d830b2a7a26780b6f5b3d5d569d1d362?d=mp&s=160"},"body":"Gitweb can be used to generate an RSS feed.\n\nArbitrary tags can be inserted into the XML document describing\nthe RSS feed by careful construction of the URL.\n\nExample\nhttp://server/?p=project.git&a=rss&f=</title><script>alert(document.cookie)</script><title>\n\nThe generated XML contains\n<script>alert(document.cookie)</script>\n\nDepending on the system used to render the XML this might lead\nto the execution of javascript in the security context of the\ngitweb server pages.\n\nPlease, escape all URL parameters.\n\nVersion tested:\ngitweb v.1.8.0.dirty with git 1.7.2.5\n\nBest regards\n\nHeinrich Schuchardt\n"},{"id":"202983","messageId":"CAM9Z-n=6xsC7yiKJ+NU-CxNPxEXWmJzvXLUocgZgWPQnuK6G4Q@mail.gmail.com","threadId":"32083","inReplyTo":"20121111232820.284510@gmx.net","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Drew Northup","fromEmail":"n1xim.email@gmail.com","sentAt":"2012-11-12T18:55:46Z","receivedAt":"2012-11-12T18:55:46Z","isPatch":false,"sender":{"key":"n1xim.email@gmail.com","avatar":null},"body":"On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de> wrote:\n> Gitweb can be used to generate an RSS feed.\n>\n> Arbitrary tags can be inserted into the XML document describing\n> the RSS feed by careful construction of the URL.\n>\n> Example\n> http://server/?p=project.git&a=rss&f=</title><script>alert(document.cookie)</script><title>\n>\n> The generated XML contains\n> <script>alert(document.cookie)</script>\n>\n> Depending on the system used to render the XML this might lead\n> to the execution of javascript in the security context of the\n> gitweb server pages.\n>\n> Please, escape all URL parameters.\n>\n> Version tested:\n> gitweb v.1.8.0.dirty with git 1.7.2.5\n>\n> Best regards\n>> Heinrich Schuchardt\n\nSomething like this may be useful to defuse the \"file\" parameter, but\nI presume a more definitive fix is in order...\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 10ed9e5..af93e65 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1447,6 +1447,10 @@ sub validate_pathname {\n        if ($input =~ m!\\0!) {\n                return undef;\n        }\n+       # No XSS <script></script> inclusions\n+       if ($input =~ m!(<script>)(.*)(</script>)!){\n+               return undef;\n+       }\n        return $input;\n }\n\n\n(I am not a perl god, so this was the lowest hanging fruit.)\n\nIf desired I'll fashion this up into a proper patch.\n\n-- \n-Drew Northup\n--------------------------------------------------------------\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"202992","messageId":"20121112202413.GD4623@sigill.intra.peff.net","threadId":"32083","inReplyTo":"CAM9Z-n=6xsC7yiKJ+NU-CxNPxEXWmJzvXLUocgZgWPQnuK6G4Q@mail.gmail.com","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-12T20:24:13Z","receivedAt":"2012-11-12T20:24:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:\n\n> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de> wrote:\n> > Gitweb can be used to generate an RSS feed.\n> >\n> > Arbitrary tags can be inserted into the XML document describing\n> > the RSS feed by careful construction of the URL.\n> [...]\n> Something like this may be useful to defuse the \"file\" parameter, but\n> I presume a more definitive fix is in order...\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 10ed9e5..af93e65 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1447,6 +1447,10 @@ sub validate_pathname {\n>         if ($input =~ m!\\0!) {\n>                 return undef;\n>         }\n> +       # No XSS <script></script> inclusions\n> +       if ($input =~ m!(<script>)(.*)(</script>)!){\n> +               return undef;\n> +       }\n>         return $input;\n>  }\n\nThis is the wrong fix for a few reasons:\n\n  1. It is on the input-validation side, whereas the real problem is on\n     the output-quoting side. Your patch means I could not access a file\n     called \"<script>foo</script>\". What we really want is to have the\n     unquoted name internally, but then make sure we quote it when\n     outputting as part of an HTML (or XML) file.\n\n  2. Script tags are only part of the problem. They are what make it\n     obviously a security vulnerability, but it is equally incorrect for\n     us to show the filename \"<b>foo</b>\" as bold. I would also not be\n     surprised if there are other cross-site attacks one can do without\n     using <script>.\n\n  3. Your filter is too simplistic. At the very least, it would not\n     filter out \"<SCRIPT>\". I am not up to date on all of the\n     sneaking-around-HTML-filters attacks that are available these days,\n     but I wonder if one could also get around it using XML entities or\n     similar.\n\nI think the right answer is going to be a well-placed call to esc_html.\nThis already happens automatically when we go through the CGI\nelement-building functions, but obviously we failed to make the call\nwhen building the output manually.  This is a great reason why template\nlanguages which default to safe expansion should always be used.\nUnfortunately, gitweb is living in 1995 in terms of web frameworks.\n\n-Peff\n"},{"id":"202993","messageId":"20121112202701.GE4623@sigill.intra.peff.net","threadId":"32083","inReplyTo":"20121112202413.GD4623@sigill.intra.peff.net","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-12T20:27:01Z","receivedAt":"2012-11-12T20:27:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 12, 2012 at 03:24:13PM -0500, Jeff King wrote:\n\n> I think the right answer is going to be a well-placed call to esc_html.\n\nI'm guessing the right answer is this:\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 10ed9e5..a51a8ba 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -8055,6 +8055,7 @@ sub git_feed {\n \t\t$feed_type = 'history';\n \t}\n \t$title .= \" $feed_type\";\n+\t$title = esc_html($title);\n \tmy $descr = git_get_project_description($project);\n \tif (defined $descr) {\n \t\t$descr = esc_html($descr);\n\nbut I did not test it (and I am not that familiar with gitweb, so it is\na slight guess from spending 5 minutes grepping and reading).\n\n-Peff\n"},{"id":"202996","messageId":"7vmwymh83r.fsf@alter.siamese.dyndns.org","threadId":"32083","inReplyTo":"20121112202701.GE4623@sigill.intra.peff.net","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-12T20:36:08Z","receivedAt":"2012-11-12T20:36:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Nov 12, 2012 at 03:24:13PM -0500, Jeff King wrote:\n>\n>> I think the right answer is going to be a well-placed call to esc_html.\n>\n> I'm guessing the right answer is this:\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 10ed9e5..a51a8ba 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -8055,6 +8055,7 @@ sub git_feed {\n>  \t\t$feed_type = 'history';\n>  \t}\n>  \t$title .= \" $feed_type\";\n> +\t$title = esc_html($title);\n>  \tmy $descr = git_get_project_description($project);\n>  \tif (defined $descr) {\n>  \t\t$descr = esc_html($descr);\n>\n> but I did not test it (and I am not that familiar with gitweb, so it is\n> a slight guess from spending 5 minutes grepping and reading).\n\nYeah, that looks correct, given the way how the other variables\nemitted with the same \"print\" like $descr and $owner are formed.\n"},{"id":"203000","messageId":"CANQwDwdRTeaVS5cMic5gv9SP1A8Z1vruOsZBFfMDQDTZHBAtvQ@mail.gmail.com","threadId":"32083","inReplyTo":"7vmwymh83r.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2012-11-12T21:13:27Z","receivedAt":"2012-11-12T21:13:27Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, Nov 12, 2012 at 9:36 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>> On Mon, Nov 12, 2012 at 03:24:13PM -0500, Jeff King wrote:\n>>\n>>> I think the right answer is going to be a well-placed call to esc_html.\n>>\n>> I'm guessing the right answer is this:\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 10ed9e5..a51a8ba 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -8055,6 +8055,7 @@ sub git_feed {\n>>               $feed_type = 'history';\n>>       }\n>>       $title .= \" $feed_type\";\n>> +     $title = esc_html($title);\n>>       my $descr = git_get_project_description($project);\n>>       if (defined $descr) {\n>>               $descr = esc_html($descr);\n>>\n>> but I did not test it (and I am not that familiar with gitweb, so it is\n>> a slight guess from spending 5 minutes grepping and reading).\n>\n> Yeah, that looks correct, given the way how the other variables\n> emitted with the same \"print\" like $descr and $owner are formed.\n\nIt looks like good solution to me too.\n\nNb. the problems with feed are mainly because it is generated\nby hand even more than HTML (which uses CGI.pm).\n\n-- \nJakub Narębski\n\n-- \nJakub Narebski\n"},{"id":"203002","messageId":"20121112213428.GK4623@sigill.intra.peff.net","threadId":"32083","inReplyTo":"CANQwDwdRTeaVS5cMic5gv9SP1A8Z1vruOsZBFfMDQDTZHBAtvQ@mail.gmail.com","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-12T21:34:28Z","receivedAt":"2012-11-12T21:34:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 12, 2012 at 10:13:27PM +0100, Jakub Narębski wrote:\n\n> > Yeah, that looks correct, given the way how the other variables\n> > emitted with the same \"print\" like $descr and $owner are formed.\n> \n> It looks like good solution to me too.\n> \n> Nb. the problems with feed are mainly because it is generated\n> by hand even more than HTML (which uses CGI.pm).\n\nYeah, I noticed that. Here it is in patch form with a test. It would be\nnice if people interested in gitweb would add more entries to the XSS\ntest below (I put in the one that fails, along with an obvious variation\nthat is actually OK).\n\nI didn't look carefully through the rest of gitweb for more XSS\ninstances. From a glance, it looks like we mostly use the safe CGI\nmethods, but probably it could use a full audit (which again, I would be\nhappy if people who care more about gitweb would do).\n\n-- >8 --\nSubject: [PATCH] gitweb: escape html in rss title\n\nThe title of an RSS feed is generated from many components,\nincluding the filename provided as a query parameter, but we\nfailed to quote it.  Besides showing the wrong output, this\nis a vector for XSS attacks.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gitweb/gitweb.perl                        |  1 +\n t/t9502-gitweb-standalone-parse-output.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 10ed9e5..a51a8ba 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -8055,6 +8055,7 @@ sub git_feed {\n \t\t$feed_type = 'history';\n \t}\n \t$title .= \" $feed_type\";\n+\t$title = esc_html($title);\n \tmy $descr = git_get_project_description($project);\n \tif (defined $descr) {\n \t\t$descr = esc_html($descr);\ndiff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh\nindex 731e64c..3a8e7d3 100755\n--- a/t/t9502-gitweb-standalone-parse-output.sh\n+++ b/t/t9502-gitweb-standalone-parse-output.sh\n@@ -185,5 +185,20 @@ test_expect_success 'forks: project_index lists all projects (incl. forks)' '\n \ttest_cmp expected actual\n '\n \n+xss() {\n+\techo >&2 \"Checking $1...\" &&\n+\tgitweb_run \"$1\" &&\n+\tif grep \"$TAG\" gitweb.body; then\n+\t\techo >&2 \"xss: $TAG should have been quoted in output\"\n+\t\treturn 1\n+\tfi\n+\treturn 0\n+}\n+\n+test_expect_success 'xss checks' '\n+\tTAG=\"<magic-xss-tag>\" &&\n+\txss \"a=rss&p=$TAG\" &&\n+\txss \"a=rss&p=foo.git&f=$TAG\"\n+'\n \n test_done\n-- \n1.8.0.207.gdf2154c\n"},{"id":"203014","messageId":"m2ip9apgfi.fsf@igel.home","threadId":"32083","inReplyTo":"CAM9Z-n=6xsC7yiKJ+NU-CxNPxEXWmJzvXLUocgZgWPQnuK6G4Q@mail.gmail.com","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2012-11-12T23:09:05Z","receivedAt":"2012-11-12T23:09:05Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Drew Northup <n1xim.email@gmail.com> writes:\n\n> Something like this may be useful to defuse the \"file\" parameter, but\n> I presume a more definitive fix is in order...\n\nA proper fix will have to add esc_html to the feed generation, something\nlike this (untested):\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 10ed9e5..a51a8ba 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -8055,6 +8055,7 @@ sub git_feed {\n \t\t$feed_type = 'history';\n \t}\n \t$title .= \" $feed_type\";\n+\t$title = esc_html($title);\n \tmy $descr = git_get_project_description($project);\n \tif (defined $descr) {\n \t\t$descr = esc_html($descr);\n\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"203059","messageId":"871B6C10EBEFE342A772D1159D13208537AA3FB5@umechphj.easf.csd.disa.mil","threadId":"32083","inReplyTo":"CAM9Z-n=6xsC7yiKJ+NU-CxNPxEXWmJzvXLUocgZgWPQnuK6G4Q@mail.gmail.com","subject":"RE: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Pyeron, Jason J CTR (US)","fromEmail":"jason.j.pyeron.ctr@mail.mil","sentAt":"2012-11-13T08:31:58Z","receivedAt":"2012-11-13T08:31:58Z","isPatch":false,"sender":{"key":"jason.j.pyeron.ctr@mail.mil","avatar":null},"body":"> -----Original Message-----\n> From: Drew Northup\n> Sent: Monday, November 12, 2012 1:56 PM\n> \n> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de>\n> wrote:\n> > Gitweb can be used to generate an RSS feed.\n> >\n> > Arbitrary tags can be inserted into the XML document describing\n> > the RSS feed by careful construction of the URL.\n> >\n> > Example\n> >\n> http://server/?p=project.git&a=rss&f=</title><script>alert(document.coo\n> kie)</script><title>\n> >\n> > The generated XML contains\n> > <script>alert(document.cookie)</script>\n\nThis is just an example.\n\n\n> >\n> > Depending on the system used to render the XML this might lead\n> > to the execution of javascript in the security context of the\n> > gitweb server pages.\n> >\n> > Please, escape all URL parameters.\n\nWe should look for the general entry points, not the script tag.\n\n\n> >\n> > Version tested:\n> > gitweb v.1.8.0.dirty with git 1.7.2.5\n> >\n> > Best regards\n> >> Heinrich Schuchardt\n> \n> Something like this may be useful to defuse the \"file\" parameter, but\n> I presume a more definitive fix is in order...\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 10ed9e5..af93e65 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -1447,6 +1447,10 @@ sub validate_pathname {\n>         if ($input =~ m!\\0!) {\n>                 return undef;\n>         }\n> +       # No XSS <script></script> inclusions\n\n### not real perl...\nforeach $xml in ( <, >, &, ...) \n{\n  $input=~s/$xml/xmlescape{$xml}/g;\n}\n\n### \"<\" => \"&lt;\"\n\n> +       if ($input =~ m!(<script>)(.*)(</script>)!){\n> +               return undef;\n> +       }\n>         return $input;\n>  }\n> \n\n"},{"id":"203085","messageId":"CAM9Z-nkuHj8MWLfWsvY=EqHXCUS+Pk5Ezv6m5J+cnh7cQHNc_g@mail.gmail.com","threadId":"32083","inReplyTo":"20121112202413.GD4623@sigill.intra.peff.net","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Drew Northup","fromEmail":"n1xim.email@gmail.com","sentAt":"2012-11-13T14:44:06Z","receivedAt":"2012-11-13T14:44:06Z","isPatch":false,"sender":{"key":"n1xim.email@gmail.com","avatar":null},"body":"On Mon, Nov 12, 2012 at 3:24 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:\n>\n>> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de> wrote:\n>> > Gitweb can be used to generate an RSS feed.\n>> >\n>> > Arbitrary tags can be inserted into the XML document describing\n>> > the RSS feed by careful construction of the URL.\n>> [...]\n>> Something like this may be useful to defuse the \"file\" parameter, but\n>> I presume a more definitive fix is in order...\n>>\n>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>> index 10ed9e5..af93e65 100755\n>> --- a/gitweb/gitweb.perl\n>> +++ b/gitweb/gitweb.perl\n>> @@ -1447,6 +1447,10 @@ sub validate_pathname {\n>>         if ($input =~ m!\\0!) {\n>>                 return undef;\n>>         }\n>> +       # No XSS <script></script> inclusions\n>> +       if ($input =~ m!(<script>)(.*)(</script>)!){\n>> +               return undef;\n>> +       }\n>>         return $input;\n>>  }\n>\n> This is the wrong fix for a few reasons:\n>\n>   1. It is on the input-validation side, whereas the real problem is on\n>      the output-quoting side. Your patch means I could not access a file\n>      called \"<script>foo</script>\". What we really want is to have the\n>      unquoted name internally, but then make sure we quote it when\n>      outputting as part of an HTML (or XML) file.\n\nI don't buy the argument that we don't need to clean up the input as\nwell. There are scant few of us that are going to name a file\n\"<script>alert(\"Something Awful\")</script>\" in this world (I am\nprobably one of them). Input validation is key to keeping problems\nlike this from coming up repeatedly as those writing the guts of\nprograms are typically more interested in getting the \"assigned task\"\ndone and reporting the output to the user in a safe manner.\n\n>   2. Script tags are only part of the problem. They are what make it\n>      obviously a security vulnerability, but it is equally incorrect for\n>      us to show the filename \"<b>foo</b>\" as bold. I would also not be\n>      surprised if there are other cross-site attacks one can do without\n>      using <script>.\n\nYes, there are. You are typically concerned with anything including\nthe following:\n(1) Executable stuff;\n(2) Out of nowhere resources that can reference executable stuff\n(style / CSS, iframe, script includes);\n(3) Media and other things that activate browser plugins directly.\n\n>   3. Your filter is too simplistic. At the very least, it would not\n>      filter out \"<SCRIPT>\". I am not up to date on all of the\n>      sneaking-around-HTML-filters attacks that are available these days,\n>      but I wonder if one could also get around it using XML entities or\n>      similar.\n\nYou will note that I said \"a more definitive fix is in order\" in my\noriginal. In other words, I claimed it to be utterly incomplete to\nstart with. I wanted to get some thought going about input validation\n(in particular since I am not a perl guru of any sort whatsoever--the\nfair number of things I've written from scratch or mangled into shape\nnotwithstanding).\n\n> I think the right answer is going to be a well-placed call to esc_html.\n> This already happens automatically when we go through the CGI\n> element-building functions, but obviously we failed to make the call\n> when building the output manually.  This is a great reason why template\n> languages which default to safe expansion should always be used.\n> Unfortunately, gitweb is living in 1995 in terms of web frameworks.\n\nEscaping the output protects the user, but it DOES NOT protect the\nserver. We MUST handle both possibilities.\nBesides, inserting one call to esc_html only fixes one attack path. I\ndidn't look to see if all others were already covered.\n\n-- \n-Drew Northup\n--------------------------------------------------------------\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"203089","messageId":"CANQwDwd0UfGshpwj-R6iatE+93PRD5p9giC_9dJ09YwXQmL6Wg@mail.gmail.com","threadId":"32083","inReplyTo":"CAM9Z-nkuHj8MWLfWsvY=EqHXCUS+Pk5Ezv6m5J+cnh7cQHNc_g@mail.gmail.com","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2012-11-13T15:19:52Z","receivedAt":"2012-11-13T15:19:52Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, Nov 13, 2012 at 3:44 PM, Drew Northup <n1xim.email@gmail.com> wrote:\n> On Mon, Nov 12, 2012 at 3:24 PM, Jeff King <peff@peff.net> wrote:\n>> On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:\n\n>>> +       # No XSS <script></script> inclusions\n>>> +       if ($input =~ m!(<script>)(.*)(</script>)!){\n>>> +               return undef;\n>>> +       }\n\n>> This is the wrong fix for a few reasons:\n>>\n>>   1. It is on the input-validation side, whereas the real problem is on\n>>      the output-quoting side. Your patch means I could not access a file\n>>      called \"<script>foo</script>\". What we really want is to have the\n>>      unquoted name internally, but then make sure we quote it when\n>>      outputting as part of an HTML (or XML) file.\n>\n> I don't buy the argument that we don't need to clean up the input as\n> well. There are scant few of us that are going to name a file\n> \"<script>alert(\"Something Awful\")</script>\" in this world (I am\n> probably one of them). Input validation is key to keeping problems\n> like this from coming up repeatedly as those writing the guts of\n> programs are typically more interested in getting the \"assigned task\"\n> done and reporting the output to the user in a safe manner.\n\nInput cleanup or blacklisting *does not* prevent code injection (XSS\nin this case). This is a myth.\n\nInput validation has its place, and is done by gitweb when possible\n(see e.g. evaluate_and_validate_params, validate_project, etc.).\n\nBut the proposed solution is not input validation.\n'<script>alert(\"Something Awful\")</script>' is a perfectly valid filename.\nAs is more realistic \"<<create>>.uml\" or \"File > Open screenshot.png\".\n\nAnd last and most important you have to escape output anyway;\nfilename is not HTML. Without escaping it would be rendered incorrectly.\nAnd HTML escaping prevents XSS.\n\n>> I think the right answer is going to be a well-placed call to esc_html.\n>> This already happens automatically when we go through the CGI\n>> element-building functions, but obviously we failed to make the call\n>> when building the output manually.  This is a great reason why template\n>> languages which default to safe expansion should always be used.\n>> Unfortunately, gitweb is living in 1995 in terms of web frameworks.\n>\n> Escaping the output protects the user, but it DOES NOT protect the\n> server. We MUST handle both possibilities.\n\nErrr, what?\n\nIf you are thinking about shell injection, we are covered.\nGitweb uses list form of open which is for shell what prepared\nstatements are for SQL. In one or two cases where we need to\nuse pipe we do shell escaping.\n\n> Besides, inserting one call to esc_html only fixes one attack path. I\n> didn't look to see if all others were already covered.\n\nThey should be covered. This case slipped.\n\n-- \nJakub Narebski\n"},{"id":"203099","messageId":"CAO54GHCzeWv41Bu5By0JOzbBHGuzXV=krdDr0U=QsMBun7PF7A@mail.gmail.com","threadId":"32083","inReplyTo":"CAM9Z-nkuHj8MWLfWsvY=EqHXCUS+Pk5Ezv6m5J+cnh7cQHNc_g@mail.gmail.com","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Kevin","fromEmail":"ikke@ikke.info","sentAt":"2012-11-13T15:45:30Z","receivedAt":"2012-11-13T15:45:30Z","isPatch":false,"sender":{"key":"ikke@ikke.info","avatar":"https://gravatar.com/avatar/60a0d08eeccb16914fdbfc7f3441548dc827dc436d5c6900ec6600e274e53c5b?d=mp&s=160"},"body":"The problem with input filtering is that you can only filter for one\noutput scenario. What if the the input is going to be output in a wiki\nlike environment, or to pdf, or whatever? Then you have to unescape\nthe data again, and maybe apply filtering/escaping for those\nenvironments.\n\nYou only know how to escape data when you are going to output it, so\nthen is the the best moment to escape it.\n"},{"id":"203100","messageId":"CANQwDwfES_hpnPOz=KTb-g3mxa8YZzzcMaj+ePj3bjgPx4DHdg@mail.gmail.com","threadId":"32083","inReplyTo":"CAO54GHCzeWv41Bu5By0JOzbBHGuzXV=krdDr0U=QsMBun7PF7A@mail.gmail.com","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2012-11-13T15:57:23Z","receivedAt":"2012-11-13T15:57:23Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, Nov 13, 2012 at 4:45 PM, Kevin <ikke@ikke.info> wrote:\n> The problem with input filtering is that you can only filter for one\n> output scenario. What if the the input is going to be output in a wiki\n> like environment, or to pdf, or whatever? Then you have to unescape\n> the data again, and maybe apply filtering/escaping for those\n> environments.\n>\n> You only know how to escape data when you are going to output it, so\n> then is the the best moment to escape it.\n\nAlso there are so many ways to evade XSS filtering\n\n  https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet\n\nIf you can and should escape data (like in our case), it cannot not work;\nnot possible to evade it.\n-- \nJakub Narebski\n"},{"id":"203115","messageId":"20121113170452.GE20361@sigill.intra.peff.net","threadId":"32083","inReplyTo":"CAM9Z-nkuHj8MWLfWsvY=EqHXCUS+Pk5Ezv6m5J+cnh7cQHNc_g@mail.gmail.com","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T17:04:52Z","receivedAt":"2012-11-13T17:04:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 09:44:06AM -0500, Drew Northup wrote:\n\n> I don't buy the argument that we don't need to clean up the input as\n> well. There are scant few of us that are going to name a file\n> \"<script>alert(\"Something Awful\")</script>\" in this world (I am\n> probably one of them). Input validation is key to keeping problems\n> like this from coming up repeatedly as those writing the guts of\n> programs are typically more interested in getting the \"assigned task\"\n> done and reporting the output to the user in a safe manner.\n\nOh, you absolutely do need to clean up the input side. And we do. Notice\nhow validate_pathname cleans out dots that could allow an attacker to do\na \"../../etc/passwd\" attack. But the input validation is _different_\nthan the output escaping. We are turning arbitrary junk from the user\ninto something we know is safe to treat as a filename. Our goal is\nprotecting the filesystem and the server, and we do that already.\nProtecting the browser on output is a different problem, and happens\nonly when we are sending to the browser.\n\nAs far as \"people will not use <script>\" in their filenames, the\nend-game to any quoting or blacklist fix is that we need to escape or\nblack _all_ HTML.  Because whether it is \"<b>\" or \"<script>\", it is\nstill wrong. Are you as comfortable saying that nobody will ever have a\n\"<\" or \"&\" in their filename?\n\n> >   3. Your filter is too simplistic. At the very least, it would not\n> >      filter out \"<SCRIPT>\". I am not up to date on all of the\n> >      sneaking-around-HTML-filters attacks that are available these days,\n> >      but I wonder if one could also get around it using XML entities or\n> >      similar.\n> \n> You will note that I said \"a more definitive fix is in order\" in my\n> original. In other words, I claimed it to be utterly incomplete to\n> start with.\n\nSorry if I came off as too harsh. My intent was to guide you in the\nright direction for the definitive fix. The fact that I ended up rolling\nthe patch myself was just because my \"probably something like this\"\nended with everybody saying \"yeah, that\", and it seemed simpler to just\nroll a test and be done.\n\n> > I think the right answer is going to be a well-placed call to esc_html.\n> > This already happens automatically when we go through the CGI\n> > element-building functions, but obviously we failed to make the call\n> > when building the output manually.  This is a great reason why template\n> > languages which default to safe expansion should always be used.\n> > Unfortunately, gitweb is living in 1995 in terms of web frameworks.\n> \n> Escaping the output protects the user, but it DOES NOT protect the\n> server. We MUST handle both possibilities.\n\nRight. But I think we already do, via validate_pathname. If that is not\nthe case, please point it out.\n\n> Besides, inserting one call to esc_html only fixes one attack path. I\n> didn't look to see if all others were already covered.\n\nProperly quoting output is something that the web framework should do\nfor you. gitweb uses CGI.pm, which does help with that, but we do not\nuse it consistently. If there are other problematic areas, I think the\nbest path forward is to use our framework more.\n\n-Peff\n"},{"id":"203118","messageId":"CANQwDwcNXPhA3Qe2K_GLuh3F8DObDQ+Wn_PHhTkJqM+4D+SK0w@mail.gmail.com","threadId":"32083","inReplyTo":"20121113170452.GE20361@sigill.intra.peff.net","subject":"Re: [BUG] gitweb: XSS vulnerability of RSS feed","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2012-11-13T17:22:56Z","receivedAt":"2012-11-13T17:22:56Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, Nov 13, 2012 at 6:04 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Nov 13, 2012 at 09:44:06AM -0500, Drew Northup wrote:\n\n>> Besides, inserting one call to esc_html only fixes one attack path. I\n>> didn't look to see if all others were already covered.\n>\n> Properly quoting output is something that the web framework should do\n> for you. gitweb uses CGI.pm, which does help with that, but we do not\n> use it consistently. If there are other problematic areas, I think the\n> best path forward is to use our framework more.\n\nWell, calling CGI.pm a _framework_ is overly generous, but it does\ninclude some HTML generation subroutines / methods, and gitweb\nmakes use of them, especially $cgi->a() for links.\n\nBut it cannot help in this case, because here we are generating XML:\nRSS or Atom feed. There was proposal some time ago to switch\nto using XML::FeedPP or XML::Atom::Feed + XML::RSS::Feed for\nfeed generation.\n\nPerhaps it is high time to switch to some Perl web (micro)framework,\nlike Dancer, Mojolicious or Catalyst... but not requiring extra modules\nhas its advantages (and there always exist Gitalist).\n-- \nJakub Narebski\n"}]}