Roundup Tracker - Issues

Issue 2550724

Title: XSS vulnerability in ok_message handling
Type: security Severity: critical
Components: Web interface Versions: 1.4
Status: closed Resolution:
Dependencies: Superseder:
Assigned To: schlatterbeck Nosy List: ber, davidben, ezio.melotti, schlatterbeck
Priority: Keywords: patch

Created on 2011-09-04 15:02 by davidben, last changed 2012-07-13 23:13 by ezio.melotti.

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

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

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="">' gets parsed as a tag named 'a 
href=""' 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
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
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?

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