[ZCM] [ZC] 813/26 Comment "host header as a vector for cross site scripting"
Collector: Zope Bugs, Features, and Patches ...
zope-coders-admin@zope.org
Thu, 13 Mar 2003 16:59:50 -0500
Issue #813 Update (Comment) "host header as a vector for cross site scripting"
** Security Related ** (Public)
Status Pending, Zope/bug critical
To followup, visit:
http://collector.zope.org/Zope/813
==============================================================
= Comment - Entry #26 by leper on Mar 13, 2003 4:59 pm
> This proposed suite of patches isnt the fix that I was expecting to
> see to this problem. These patches work under the assumption that
> URL1 etc can be subverted by a malicious user, and need to be quoted
> everywhere that it is used.
Correct, because *URL*, BASE*, etc. can be subverted and need to be quoted.
> Has consideration been given to the option of getting ZPublisher to
> validate the host header, and quote all urls before passing them to
> the application?
Yes. You can't do that. That would be making the assumption that you know the context the variable will be rendered in, and thats not a valid assumption. It is, at first, the most obvious solution, sadly its not an acceptable one.
> Also, I think many of those calls to cgi.quote should really be
> urllib.quote. right?
Not really. This, again, is an issue of context. urllib.quote can be used in a variety of non-presentation contexts when you know exactly what you're quoting (path segments, query string variables, etc.). It can also be used in presentation contexts, but only if what your quoting hasn't already been quoted, and only if you're selective about the parts of the URI you quote.
By the time you've reached presentation context its common that you know longer now the exact nature of the URI (it may have already undergone a degree of urllib.quote()ing, which makes it unsafe to use urllib.quote. cgi.escape OTOH is of a different nature. You can't use cgi.escape in non-presentation contexts because it doesn't make any sense to do so. (places where it is used anyway are generally bugs) By the time your variables have trickled down to the presentation layer (Woah Reaganomics!) you're able to use contextual quoting to ensure special characters don't funge anything. In the case of HTML thats cgi.escape. (Although, IMO, "cgi" isn't the correct module, an HTML module would be better, but then HTML modules tend to carry more baggage.)
________________________________________
= Comment - Entry #25 by efge on Mar 13, 2003 12:15 pm
FWIW I agree with the sample of patches I've read (it's a bit of a
PITA to read them all) and the approach taken.
I don't like "magic" quoting in ZPublisher.
________________________________________
= Unrestrict_pending - Entry #24 by htrd on Mar 13, 2003 11:44 am
This proposed suite of patches isnt the fix that I was expecting to see to this problem. These patches work under the assumption that URL1 etc can be subverted by a malicious user, and need to be quoted everywhere that it is used.
Has consideration been given to the option of getting ZPublisher to validate the host header, and quote all urls before passing them to the application? Applications could continue using BASEPATH1 (etc) in the traditional manner.
(Note I am not arguing that this would be better. I havent thought about it enough to reach a conclusion. Im just stating that this approach was unexpected)
Also, I think many of those calls to cgi.quote should really be urllib.quote. right?
________________________________________
= Comment - Entry #23 by leper on Mar 6, 2003 5:00 am
Uploaded: "badaqueduct2.diff"
- http://collector.zope.org/Zope/813/badaqueduct2.diff/view
Finished the grep audit for PATH_INFO, HTTP_HOST, PATH_TRANSLATED, HTTP_REFERER, SERVER_NAME, and QUERY_STRING. Only found one instance of HTTP_REFERER in aqueduct that got past me the first time. This is the last grep audit I'm conducting for this issue.
Given the results of this restricted mini-audit, I think its fair to say Zope could use a serious full-scale audit with respect to contextual escaping.
A while back I wrote a long message to the Zope list about how to use a VHM to protect against this vulnerability, and what the scope of the problem is. You can find that at http://marc.theaimsgroup.com/?l=zope&m=104639584701163&w=2 should there be any doubts as to why this bug was filed.
________________________________________
= Comment - Entry #22 by leper on Mar 5, 2003 9:08 pm
Uploaded: "sillyzcatvocab.diff"
- http://collector.zope.org/Zope/813/sillyzcatvocab.diff/view
*URL* fallout final
patches prefixed with "silly" don't address security problems, just needless dtml-var usage when entity quoting is technically required
It has now been 3 weeks since I opened this bug and I haven't heard a thing in response to it. Why?
________________________________________
= Comment - Entry #21 by leper on Mar 5, 2003 9:05 pm
Uploaded: "sillyzcatview.diff"
- http://collector.zope.org/Zope/813/sillyzcatview.diff/view
*URL* fallout
________________________________________
= Comment - Entry #20 by leper on Mar 5, 2003 9:04 pm
Uploaded: "sillyscripttry.diff"
- http://collector.zope.org/Zope/813/sillyscripttry.diff/view
*URL* fallout
heh, just when you think you an entity quoted context is what you want...
________________________________________
= Comment - Entry #19 by leper on Mar 5, 2003 9:01 pm
Uploaded: "sillypivocab.diff"
- http://collector.zope.org/Zope/813/sillypivocab.diff/view
*URL* fallout
________________________________________
= Comment - Entry #18 by leper on Mar 5, 2003 8:59 pm
Uploaded: "badvhosting.diff"
- http://collector.zope.org/Zope/813/badvhosting.diff/view
*URL* fallout
________________________________________
= Comment - Entry #17 by leper on Mar 5, 2003 8:59 pm
Uploaded: "badtuttopic.diff"
- http://collector.zope.org/Zope/813/badtuttopic.diff/view
*URL* fallout
________________________________________
= Comment - Entry #16 by leper on Mar 5, 2003 8:57 pm
Uploaded: "badfindresult.diff"
- http://collector.zope.org/Zope/813/badfindresult.diff/view
*URL* fallout
________________________________________
= Comment - Entry #15 by leper on Mar 5, 2003 8:55 pm
Uploaded: "baddtui.diff"
- http://collector.zope.org/Zope/813/baddtui.diff/view
*URL* fallout
This file also uses <!--#var __str__--> in the midst of a textarea.
I figure thats bad, but exactly what __str__ was in this context was a little hard to discern, and I'm not even sure it works. Anyway, it probably needs to be switched entity syntax too... but my patch doesn't make that change.
________________________________________
= Comment - Entry #14 by leper on Mar 5, 2003 8:52 pm
Uploaded: "baddtin.diff"
- http://collector.zope.org/Zope/813/baddtin.diff/view
*URL* fallout
________________________________________
= Comment - Entry #13 by leper on Mar 5, 2003 8:51 pm
Uploaded: "badcdefrep.diff"
- http://collector.zope.org/Zope/813/badcdefrep.diff/view
*URL* fallout
________________________________________
= Comment - Entry #12 by leper on Mar 5, 2003 8:49 pm
Uploaded: "badaqueduct.diff"
- http://collector.zope.org/Zope/813/badaqueduct.diff/view
*URL* fallout
this whole file is begging for review/reworking - I only fixed the dtml misuse, the string interpolation bits are still a complete mess
________________________________________
= Comment - Entry #11 by leper on Mar 5, 2003 8:45 pm
Uploaded: "badappmgr.diff"
- http://collector.zope.org/Zope/813/badappmgr.diff/view
OK, grep audit for *URL* is done. First some general comentary, then the patches.
lib/python/HelpSys/dtml/button.dtml - this jams insufficently untainted data into the midst of some javascript, I didn't want to deal with it, because the easiest way to fix it just remove all javascript, but I figure I'll face opposition if I submit a patch that does so.
lib/python/Shared/DC/ZRDB/dtml/customDefaultZPTReport.dtml - this is a page template, yet its named .dtml, silly
lib/python/TreeDisplay/TreeTag.py - the anchors this makes (around line 346) don't really undergo correct contextual quoting, but I'm not sure if things have come to rely on that or not, thus, no patch.
I skipped the webdav library entirely, for reasons stated earlier. There are multiple areas where urls are thrown into the protocol output with no attempt at escaping, and a few where an attempt is made. That should be fixed.
________________________________________
= Comment - Entry #10 by leper on Mar 3, 2003 8:53 pm
Uploaded: "badmessagedialog.diff"
- http://collector.zope.org/Zope/813/badmessagedialog.diff/view
Tangentially related to all this is the misuse of the MessageDialog class. Before we get back to the meat of the issue, here is a bonus patch that addresses said misuse.
________________________________________
= Comment - Entry #9 by leper on Mar 2, 2003 3:12 am
Uploaded: "badsimptree.diff"
- http://collector.zope.org/Zope/813/badsimptree.diff/view
BASE* fallout final
________________________________________
= Comment - Entry #8 by leper on Mar 2, 2003 3:11 am
Uploaded: "badapplication.diff"
- http://collector.zope.org/Zope/813/badapplication.diff/view
BASE* fallout
________________________________________
= Comment - Entry #7 by leper on Mar 2, 2003 3:10 am
Uploaded: "badmanagement.diff"
- http://collector.zope.org/Zope/813/badmanagement.diff/view
Finished the "grep audit" for BASE* and DestinationURL.
DestinationURL came up clean, patches for BASE* attached.
________________________________________
= Comment - Entry #6 by leper on Mar 1, 2003 6:47 am
Uploaded: "badsearch.diff"
- http://collector.zope.org/Zope/813/badsearch.diff/view
absolute_url fallout final
________________________________________
= Comment - Entry #5 by leper on Mar 1, 2003 6:45 am
Uploaded: "badramcachemgr.diff"
- http://collector.zope.org/Zope/813/badramcachemgr.diff/view
absolute_url fallout
________________________________________
= Comment - Entry #4 by leper on Mar 1, 2003 6:44 am
Uploaded: "badhelpsys.diff"
- http://collector.zope.org/Zope/813/badhelpsys.diff/view
absolute_url fallout
________________________________________
= Comment - Entry #3 by leper on Mar 1, 2003 6:42 am
Uploaded: "badaccelcachemgr.diff"
- http://collector.zope.org/Zope/813/badaccelcachemgr.diff/view
I've finished a mini-"grep audit" for absolute_url abuse.
This is, of course, only one of the *many* places tainted host information (and more) can come from. There's still URL*, BASE*, BASEPATH*, plus secondary functions like DestinationURL--the list is seemingly endless. woo. Nevertheless, this kind of thing has to be done sooner or later. Even after some form of hostname canonicalization mechanism is inplace (which I believe is inevitable) raw URIs being placed into contexts that require certain quoting is bad and must be fixed.
There are certain absolute_url abuses I'm not going to issue patches for, and those are:
lib/python/OFS/Image.py in Image.tag() - Image.py is mess, I refuse to touch it, sorry.
lib/python/DocumentTemplate/DT_Var.py in render() - this code is hard to follow, I think its safe, but I'm not 100% sure.
lib/python/OFS/PropertySheets.py in dav__source() - definately unsafe, but the XML context of a WEBDAV transaction is a bit outside of my familiarity - I'm pretty sure a simple entity quoting like:
- url=urlbase(vself.absolute_url())
+ url=escape(urlbase(vself.absolute_url()))
using escape from cgi will be adequate but a second set of eyes on that would be good.
lib/python/Products/PythonScripts/www/default_py - this is the default text of a new script (python) instance, and its broken, it can't decide what context it wants to return, its either markup or not, but just quoting script.title doesn't cut it, either quote all the variables or none of them. Anyway, pick one and stick with it.
The rest of the problems I found I'm attaching patches for. Patches are made against CVS, but apply to 2.6.1 without problems.
________________________________________
= Comment - Entry #2 by leper on Feb 26, 2003 12:00 am
Uploaded: "badbase.diff"
- http://collector.zope.org/Zope/813/badbase.diff/view
Here's the simple fix for the base href. As base injection doesn't get cached by sites using RAM caches they were immune to the xss, however fronting proxy caches that index on URI and don't include the Host header in the key data would be vulnerable (in theory).
Sloppy use of URLx variables and such are still problematic, but thats not the zope machinery's fault. What remains zope's fault are things like Image.py and lord knows what other modules, that are allowing untainted data to be injected into any part of a page which *IS* cached by RAM caches.
Again, please make this bug visible to all, while all the code hasn't been fixed yet, Zope users need to be aware of the work-around techniques they can use to protect their installations.
________________________________________
= Request - Entry #1 by leper on Feb 18, 2003 4:38 am
Please leave this issue public, its come up on the zope lists already, hiding it now is pointless. (subject: "Zope & SSL & IE6 redirection bug" on zope list)
HTTP host headers may be used as an injection point for cross site scripting code. This is a critical problem for sites utilizing caches.
Information used for generating absolute URLs is obtained from the host header, in at least one case (that of the base href tag frequently injected into published html documents by ZPublisher) this information does not undergo the requisite contextual quoting, its possible there are more. For sites which rely on caches to improve performance this means its possible to poison a cache with a document containing malicious markup.
Workarounds:
Using a VirtualHostMonster with a proxy server, hardcode the host information into the URI used for the proxy request. The VHM will override the Host header with the hardcoded data. (tested with Zope 2.6.1 and Apache+mod_rewrite+mod_proxy)
==============================================================