Re: [BUG] gitweb: XSS vulnerability of RSS feed

From: Jakub Narębski <jnareb@gmail.com>
Date: Nov 13, 2012, 15:19 UTC
Message-ID: <CANQwDwd0UfGshpwj-R6iatE+93PRD5p9giC_9dJ09YwXQmL6Wg@mail.gmail.com>
In-Reply-To: <CAM9Z-nkuHj8MWLfWsvY=EqHXCUS+Pk5Ezv6m5J+cnh7cQHNc_g@mail.gmail.com>

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;
>>> +       }

Show 15 quoted lines

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

Show 9 quoted lines

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