Created on 2011年09月04日 15:02 by davidben, last changed 2012年07月13日 23:13 by ezio.melotti.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | Remove |
| issue2550724.diff | ezio.melotti, 2011年09月18日 12:58 | |||
| issue2550724-2.diff | ezio.melotti, 2011年09月18日 13:03 | |||
| issue2550724-3.diff | ezio.melotti, 2011年09月19日 11:05 | |||
| Messages | |||
|---|---|---|---|
| msg4410 | Author: [hidden] (davidben) | Date: 2011年09月04日 15:02 | |
The ok_message parameter is filtered by a regular expression to attempt to restrict the tags. The check isn't strict enough and can be bybassed as follows. This leaves the site vulnerable to a cross-site scripting attack and allows an attacker to run arbitrary javascript within Roundup's origin. http://issues.roundup-tracker.org/?@ok_message=%3C%3Cscript%20%3E%3Ealert(42)%3B5%3C%3C%2Fscript%20%3E%3E (The string is "<<script >>alert(42);5<</script >>") The regular expression also does not escape an unclosed tag (which gets closed later by stray >s in the page), although this is less obviously exploitable. http://issues.roundup-tracker.org/?@ok_message=%3Cscript%20 It could also be possible to create a link to a javascript: URL and trick the user into clicking it, although the check for links in the regular expression doesn't work. '<a href="http://example.com">' gets parsed as a tag named 'a href="http://example.com"' because * is greedy. Given that the check evidently doesn't work anyway, it's probably better to disallow it altogether and avoid the need for a more complex and error-prone filter. |
|||
| msg4411 | Author: [hidden] (ezio.melotti) | Date: 2011年09月04日 16:19 | |
In my opinion the "ok_message" doesn't even belong to the URL, and should be generated server side. See also http://psf.upfronthosting.co.za/roundup/meta/issue296 |
|||
| msg4412 | Author: [hidden] (davidben) | Date: 2011年09月05日 01:18 | |
Agreed. Allowing arbitrary HTML specified in the URL like this is sloppy and extremely prone to XSS attacks. But in absence of redesigning the messages altogether, the filter should be as absolutely strict as possible. |
|||
| msg4415 | Author: [hidden] (ber) | Date: 2011年09月05日 09:40 | |
David, thanks for reporting! Ezio, thanks for commenting! Next we would need a patch to improve the situation, I'd say. Maybe we can make the filter better while also splitting out another issue for the architectural issues, if there are any. I am willing to review a patch, if there is one. |
|||
| msg4426 | Author: [hidden] (ezio.melotti) | Date: 2011年09月18日 12:58 | |
Attached a patch to fix the issue. I changed the clean_message function to escape everything first and then "restore" the whitelisted elements (i.e. strong, em, b, i, a, br). This might not be super-efficient, but it's safer and this is not a performance-critical part anyway. |
|||
| msg4427 | Author: [hidden] (davidben) | Date: 2011年09月18日 17:36 | |
May as well drop support for <a> tags, since it doesn't work anyway and makes things more complex. Also, maybe you could do something poor with javascript: URLs. (I was wrong about the reason. It only matches one- character URLs, so it happens to match the unit test, but nothing else.) |
|||
| msg4428 | Author: [hidden] (ezio.melotti) | Date: 2011年09月18日 17:58 | |
Indeed, I didn't notice that the regex for <a href="..."> was broken. The fix is trivial, but since no one seem to have noticed the brokenness, I think it's safe to remove it and avoid other possible vulnerabilities. Attached new patch. |
|||
| msg4430 | Author: [hidden] (ber) | Date: 2011年09月19日 07:19 | |
David, Ezio, thanks for work on the patch. How much testing do we need, what do you think? Bernhard |
|||
| msg4431 | Author: [hidden] (ezio.melotti) | Date: 2011年09月19日 11:05 | |
I managed to upload the wrong patch, the correct one is the -3 one *. My patch includes some tests, if you can think about other ways to break the function I can add more tests (and possibly make the function more robust if they fail). FWIW the "Clear this message" link still works, probably because it's added after the escaping. * If I try to remove the old patches with the "Remove" button I get an "Invalid request" error. I think that's because method="post" is missing from the form, and it should be already fixed in recent versions of Roundup (is this instance updated?) |
|||
| msg4461 | Author: [hidden] (ezio.melotti) | Date: 2011年11月29日 04:01 | |
Any news on this? The last patch I uploaded should be ready to go in. |
|||
| msg4555 | Author: [hidden] (ezio.melotti) | Date: 2012年05月15日 06:46 | |
This has been fixed by Ralf by disallowing all the tags (except <br>), so the issue can be closed. Note however that my patch escapes all the tags first, and then restores only the allowed ones, so it should be as safe as the committed fix. (I also noticed a minor mistake in the comment of the patch -- <a> is not allowed anymore.) |
|||
| msg4563 | Author: [hidden] (ber) | Date: 2012年05月16日 10:30 | |
Ralf, I think you should comment why you didn't end up using Ezio's solution, before resolving this one. :) |
|||
| msg4567 | Author: [hidden] (schlatterbeck) | Date: 2012年05月22日 06:56 | |
I've looked through the code and found no message that used html markup (e.g. bold or italics) in the message. For this reason I chose to go the more secure route and completely escape the message string. Ezio: I hadn't looked at your patch before but I think, the approach of first escaping everything and *then* re-enable allowed tags should also be secure enough. So if anybody really needs the feature of highlighting parts of a message I'm open for including this again, feel free to open a feature request. But given that the feature seems to be unused I guess we can live without highlighting in messages. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2012年07月13日 23:13:35 | ezio.melotti | set | files: - issue2550724.diff |
| 2012年05月22日 06:56:42 | schlatterbeck | set | status: new -> closed messages: + msg4567 |
| 2012年05月16日 10:30:11 | ber | set | assignee: schlatterbeck messages: + msg4563 |
| 2012年05月15日 06:46:57 | ezio.melotti | set | messages: + msg4555 |
| 2012年01月08日 07:33:16 | ezio.melotti | set | nosy: + schlatterbeck |
| 2011年11月29日 04:01:36 | ezio.melotti | set | messages: + msg4461 |
| 2011年09月19日 11:05:29 | ezio.melotti | set | files:
+ issue2550724-3.diff messages: + msg4431 |
| 2011年09月19日 07:19:17 | ber | set | messages: + msg4430 |
| 2011年09月18日 17:58:50 | ezio.melotti | set | files:
+ issue2550724.diff messages: + msg4428 |
| 2011年09月18日 17:36:14 | davidben | set | messages: + msg4427 |
| 2011年09月18日 13:03:55 | ezio.melotti | set | files: + issue2550724-2.diff |
| 2011年09月18日 12:58:29 | ezio.melotti | set | files:
+ issue2550724.diff keywords: + patch messages: + msg4426 |
| 2011年09月05日 09:40:57 | ber | set | nosy:
+ ber messages: + msg4415 |
| 2011年09月05日 01:18:17 | davidben | set | messages: + msg4412 |
| 2011年09月04日 16:19:24 | ezio.melotti | set | nosy:
+ ezio.melotti messages: + msg4411 |
| 2011年09月04日 15:02:20 | davidben | create | |
Supported by The Python Software Foundation,
Powered by Roundup