Roundup Tracker - Issues

Issue 2550724

classification
XSS vulnerability in ok_message handling
Type: security Severity: critical
Components: Web interface Versions: 1.4
process
Status: closed
:
: schlatterbeck : ber, davidben, ezio.melotti, schlatterbeck
Priority: : patch

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:35ezio.melottisetfiles: - issue2550724.diff
2012年05月22日 06:56:42schlatterbecksetstatus: new -> closed
messages: + msg4567
2012年05月16日 10:30:11bersetassignee: schlatterbeck
messages: + msg4563
2012年05月15日 06:46:57ezio.melottisetmessages: + msg4555
2012年01月08日 07:33:16ezio.melottisetnosy: + schlatterbeck
2011年11月29日 04:01:36ezio.melottisetmessages: + msg4461
2011年09月19日 11:05:29ezio.melottisetfiles: + issue2550724-3.diff
messages: + msg4431
2011年09月19日 07:19:17bersetmessages: + msg4430
2011年09月18日 17:58:50ezio.melottisetfiles: + issue2550724.diff
messages: + msg4428
2011年09月18日 17:36:14davidbensetmessages: + msg4427
2011年09月18日 13:03:55ezio.melottisetfiles: + issue2550724-2.diff
2011年09月18日 12:58:29ezio.melottisetfiles: + issue2550724.diff
keywords: + patch
messages: + msg4426
2011年09月05日 09:40:57bersetnosy: + ber
messages: + msg4415
2011年09月05日 01:18:17davidbensetmessages: + msg4412
2011年09月04日 16:19:24ezio.melottisetnosy: + ezio.melotti
messages: + msg4411
2011年09月04日 15:02:20davidbencreate

Supported by The Python Software Foundation,
Powered by Roundup

Last modified: Fri Feb 28 22:02:04 EST 2020

AltStyle によって変換されたページ (->オリジナル) /