Forum OpenACS Development: XSS vulnerability in XoWiki and a lot of other OpenACS pages

I just got an alert from a Brazilian government institution responsible for security saying that my website has a XSS vulnerability. I had already received other alerts concerning the same matter, but this time I have to do something or they will shut my website down. This institution is reponsible to check all government sites security, and if they are not happy, they can shut you down.

The problem is very simple: we basically don't check HTML code supplied as URL vars. An example is below:

http://www.softwarepublico.gov.br/xowiki/%3C/style%3E%3Cscript%3Ea=eval;b=alert;a(b(/XSS/.source));%3C/script%3E

If you follow this link, you are going to execute a JS code in a real URL domain. There are a lot of other places where the same behaviour can be seen, basically every single place where we give the possibility to receive HTML as URL vars. Another example is account-closed:

http://www.softwarepublico.gov.br/register/account-closed?message=%3C/style%3E%3Cscript%3Ea=eval;b=alert;a(b(/XSS/.source));%3C/script%3E

Of course, in order for this vulnerability do something harmfull, an user have to click on a link he/she receives in an e-mail, for instance, and we don't supply this kind of link. The problem is that user is clicking on a valid domain, and most users (almos every single user) will consider this as a valid URL.

I'm looking for a more general solution, like an RP filter, because I guess it would be insane to check every single page that receives HTML vars for this vulnerability. So, any suggestions and comments will be appreciated.

Thank you for your comment. In fact the problem only happens where you have the option to supply the HTML code as query vars in ad_page_contract, and that's exaclty what I'm talking about. These pages show this behaviour in general, and as they are a lot of pages, fix it for every single page would be insane.

However, you gave me a good hint: maybe we should change ad_page_contract to verify the HTML code? I did the test you said: changed the allowedtag parameter, but it seems like this check is only valid to form submition, not to URL vars. A possible fix would be to add this tag check to HTML URL vars?

I don't see any reason not to security check HTML. In generaal especiall for the account closed message, there is most likely a much better technique.

For account closed I suggest modifying it to use the util_user_message feature instead of passing the message in the URL.

Not sure about xowiki but I am not aware of a good reason to pass HTML in the way you have shown.

The only time you really need to pass HTML is during a form submission, where you are soliciting content from the user that may have HTML tags and it needs to be security checked.

That said, if you allow HTML to be passed on a form submit or just url variable it must be checked and we should do that by default for html input in ad_page_contract to reduce mistakes. Since normally form input is not processed by ad_page_contract this would cover both cases as long as you are using the form builder to process your forms.

Thanks for letting us know about this issue and your suggestions.

No reason to pass the message around in that case anyway.

Dave, your patch doesn't include the util_user_message call. I've just included it, but it's not working somehow. Take a look at how my patch is right now: http://pastebin.info/902

Torben, I'm also working on ad-page-contract. I'll probably insert the same check as we have on the forms. As soon as it's working I'll post a patch.

Even my previous example in this post is not adequate. The check for the form variables should actually be against the form submitted data before actual_name is assigned, to prevent some other nasty stuff that I can think about but don't want to post here.

ps. The check should bypass the rest of the loop if it fails.

I am currently on vacation high up in the Austrian alps. If there is more to do than upgrading and/or applying the patch i can still try to help.

http://fisheye.openacs.org/changelog/OpenACS/openacs-4/packages/xowiki/tcl/package-procs.tcl?cs=MAIN%3Agustafn%3A20090915082107

ad_proc encode_html {string} {
Escape HTML code
} {
return [string map [list & "&amp;" < "&lt;" > "&gt;" \" "&quot;" ' "&#39;"] $string]
}

Then modify the patch to call this encode_html procedure instead of the xowiki includelet one and see if that helps.

guess the error message comes from the prototype page "news". Actually, the upgrade scripts are supposed to add new revision to the instantiated prototype pages. In case, something went wrong, you can reload the prototype page via ds/shell (assuming the instance is called /xowiki, and the problem comes from the "news" prototype page

::xowiki::Package initialize -url /xowiki
$package_id import-prototype-page news

Hope, this helps

I've sent other possible cases needing review to Dave. Dave, let me know if you didn't get my replies to your email..

cheers,

Torben

Thank you very much for all your help on this matter. I'm first trying to apply the patch you've provided to see if it solves the problem. My feeling is that it solves the security issue. I had to destroy the test server database because XoWiki upgrade failed, and it's going to take sometime to test it again.

If it works, I'm not sure if the upgrade is worthy, because it's always very difficult to work with HEAD version. Do you have any kind of CVS tag for a stable (or at least close to stable) version for XoWiki?