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.

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

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.

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.

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.

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

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.

David, Ezio,
thanks for work on the patch.
How much testing do we need, what do you think?

Bernhard

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?)

Any news on this?
The last patch I uploaded should be ready to go in.

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

Ralf, I think you should comment why you didn't end up using Ezio's
solution, before resolving this one. :)

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.