Forum OpenACS Development: Re: XSS / Reflection with return_url

Collapse
6: Re: XSS / Reflection with return_url (response to 2)
Posted by Brian Fenton on 05/01/20 11:54 AM
Hi Frank

a while back I wrote a simple ad_page_contract filter for checking URLs. You will still need to manually apply this to your ad_page_contracts e.g.

ad_page_contract {
} {
{return_url:return_url ""}
}

Here's the proc:

ad_page_contract_filter return_url { name value } {
Because return URLs can be tampered with, check that there is no Javascript embedded in the URL.
This could be used for any URL really, not just return URLs.
} {

if {[string match -nocase "*<script>*" $value] || [string match -nocase "*javascript*" $value]} {
set quoted_value [ns_quotehtml $value]
ad_complain "Invalid value $quoted_value for input $name"
return 0
}
return 1
}

Collapse
7: Re: XSS / Reflection with return_url (response to 6)
Posted by Frank Bergmann on 05/01/20 12:20 PM
Hi Brian,

Time Herres from Daimler found the following exploit:

return_url=%27onmouseover=%27alert(5)%27style=%27position:absolute;width:100%;height:100%;top:0;left:0

This does not include "script" nor "javascript", so it would slip through your net, wouldn't it?

Did you try with this url?
http://server.com/acs-admin/apm/version-reload?version_id=31184&return_url=...

It's a good question: What characters are allowed for a return_url, and which ones are not... Can we exclude quote, double quote and angle brackets in general? What about character quoting?

Cheers
Frank

Collapse
8: Re: XSS / Reflection with return_url (response to 7)
Posted by Brian Fenton on 05/01/20 12:38 PM
Hi Frank

yes, you're correct. My filter wouldn't catch that example, it's just a starting point for a general solution.

The more correct solution in the version-reload case is of course to use the OpenACS quoting feature before reflecting the inputted value e.g. [ns_quotehtml $return_url]

Brian

Collapse
9: Re: XSS / Reflection with return_url (response to 8)
Posted by Frank Bergmann on 05/01/20 12:55 PM
I've just seen this "localurl" filter from Gustaf apparently:

https://openacs.org/api-doc/content-page-view?source_p=1&path=/packages/acs-admin/www/apm/version-reload.tcl

Cheers
Frank

Collapse
10: Re: XSS / Reflection with return_url (response to 9)
Posted by Brian Fenton on 05/01/20 01:02 PM
That's this proc: https://openacs.org/api-doc/proc-view?proc=ad_page_contract_filter_proc_localurl&source_p=1

It seems to check if the URL is external. I don't see any script checks there.

Collapse
11: Re: XSS / Reflection with return_url (response to 10)
Posted by Gustaf Neumann on 05/01/20 01:06 PM
This script would also be able to perform checks if every user_id has an user_id:integer data-type and add this in case of necessity

Frank, this happened a few years ago. if you find any any case of not-sufficiently checked input parameters (including all kind of ids, Booleams, returnurls, ...) in OpenACS 5.10, please let us know by writing a issue tracker entry.

See here, how returnurl should be protected in page contracts:
https://openacs.org/api-doc/content-page-view?path=packages/acs-admin/www/apm/version-reload.tcl&version_id=5443646&source_p=1

All ~100 packages in oacs-5-10 are checked frequently by us with acunetix. It is also recommended to run OpenACS 5.10 with CSP and the auto-generated security rules enabled. Also this was addressed in all packages in oacs-5-10, but will probably require some work in PO.

Yes it is true, that OpenACS 5.9 had some potential security flaws (you can say this about every web application package released a few years ago)

Collapse
12: Re: XSS / Reflection with return_url (response to 10)
Posted by Gustaf Neumann on 05/01/20 01:08 PM
CSP handles js URLS. When you try it, you will see:
Content Security Policy: The page’s settings blocked the loading of a resource at inline (“script-src”).
Collapse
13: Re: XSS / Reflection with return_url (response to 12)
Posted by Frank Bergmann on 05/01/20 01:14 PM
Thanks a lot!

I believe we'll need to quickly relase ]po[ V5.1 with OpenACS 5.10...

Cheers
Frank