XSS request parameter filtering magles parameters by inserting HTML character entities.

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Check in a file name "à.txt" in a git repository.
2. View that commit in GitBlit.
3. Try to view the file, or a diff with the file.

What is the expected output? What do you see instead?

Expected: The file is shown (view, raw links), or a diff is shown (commitdiff, 
for instance).

Actual: GitBlit complains that it cannot find the file à.txt.

What version of the product are you using? On what operating system?

GitBlit 1.6.2 as part of a Gerrit plugin.

Please provide any additional information below.

The problem is in JSoupXssFilter.clean(): the final call to html() introduces 
these entities. That's correct when you want to insert the result into HTML, 
but if that happens on request parameters, subsequent processing is broken 
because file paths have been mangled.

Original issue reported on code.google.com by tw201...@gmail.com on 4 Nov 2014 at 11:09

[GoogleCodeExporter]

I see that this bug tracker does not replace entities. So the above 
"à.txt" should read "à.txt".

Original comment by tw201...@gmail.com on 4 Nov 2014 at 11:10

[GoogleCodeExporter]

Actually, I don't quite see why GitBlit runs request parameters through JSoup. 
What if I have a file named "<script>.txt" in my git repository?

As I understand it, XSS can occur if you send back unescaped input from a 
non-trusted source (request parameters, external files, form input, and so on) 
verbatim to the client, typically by including it directly in some page HTML. 
So that's the point where you have to prevent XSS by properly escaping. Doing 
it upon reception of the data is too early.

Original comment by tw201...@gmail.com on 5 Nov 2014 at 11:13

[GoogleCodeExporter]

I approached this a little like handling SQL injection - take care of the 
inputs.  Sanitizing the input was a single-point change for handling reported 
XSS vulnerabilities.  Sanitizing the output is another option but required 
identifying all vulnerable locations.  Perhaps it would be better to just 
sanitize the outputs.

Original comment by James.Mo...@gmail.com on 5 Nov 2014 at 1:36

[GoogleCodeExporter]

I think so. On SQL injections: you'd take care of the inputs _to the SQL 
engine_. (Or use PreparedStatements with bind variables, or some ORM framework 
or other library that hides all that from your app.) For XSS, you'd have to 
take care of inputs to the "HTML engine" or use an HTML engine that 
automatically encodes correctly.

Request parameters are not just input to the HTML, they're also input to JGit. 
Passing JGit HTML-sanitized input doesn't work well.

Original comment by tw201...@gmail.com on 5 Nov 2014 at 3:01

[GoogleCodeExporter]

Clearly.  :)

Original comment by James.Mo...@gmail.com on 5 Nov 2014 at 3:06

[GoogleCodeExporter]

Here's a suggestion for a quick (temporary?) fix for paths only which would 
solve the issue creator's problem and ours.

Change the implementation of WicketUtils.getPath(...) method to use the apache 
commons-lang method StringEscapeUtils.unescapeHtml(...), something like this
--------
String path = params.getString("f", null);
return path == null ? null : StringEscapeUtils.unescapeHtml(path);
--------

I've played around a bit, running the gitserver with that implementation, and 
it seems to be working fine. It doesn't seem to break any tests either. All 
this with the disclaimer that Gitblit application and code is a new (and very 
nice) experience to me.

Original comment by dan.ande...@gmail.com on 6 Apr 2015 at 12:49