Noticed by @alpritt at #647084-153: Missing shortcut edit and delete operations on shortcuts admin - and as far as I can tell, there isn't an existing issue for it:
The overlay seems to bypass any filtering of page titles for XSS.
Possible steps to reproduce (there are others):
1. Go to admin/structure/menu
2. Add a new menu with title <script type="text/javascript">alert('xss');</script>
3. Save it
With the overlay, you get a JavaScript alert box. With the overlay disabled, you don't - the page title is shown with check_plain() escaping.
I haven't looked carefully but it seems like the culprit is probably here:
function overlay_preprocess_html(&$variables) {
if (overlay_get_mode() == 'child') {
// Add overlay class, so themes can react to being displayed in the overlay.
$variables['classes_array'][] = 'overlay';
// Do not include site name or slogan in the overlay title.
$variables['head_title'] = drupal_get_title();
}
}
Also, not realizing the above, but seeing this security issue with the Shortcut module, I put in a fix for this in the wrong place in the patch that was committed at #647084: Missing shortcut edit and delete operations on shortcuts admin. That fix was here:
function shortcut_set_title($shortcut_set) {
return check_plain($shortcut_set->title);
}
So as part of this issue, we should remove that check_plain() call, since it leads to double-escaping.
| Comment | File | Size | Author |
|---|---|---|---|
| #43 | overlay-headtitle.patch | 2.28 KB | casey |
| #36 | overlay-headtitle.patch | 1.48 KB | casey |
| #34 | overlay-headtitle.patch | 667 bytes | casey |
| #33 | overlay-page-title-725734-33.patch | 4.58 KB | David_Rothstein |
| #30 | 725734_strip_tags.patch | 691 bytes | aspilicious |
Comments
Comment #1
David_Rothstein commentedAdding the security tag.
Comment #2
alpritt commentedThis is a bit of a strange one!
Since D7, titles are escaped automatically in drupal_set_title(). And so, if you print out $variables['head_title'] from overlay_preprocess_html() (see above) you will see it is indeed already escaped.
So what's happening?
Well it looks like it is un-escaping itself again!
I've not quite got my head around this, but I guess what is happening is this line...
var iframeTitle = self.$iframeDocument.attr('title');at line 430 of overlay-parent.js is grabbing the rendered title (i.e. the head title) instead of the raw HTML for the title. And of course the escaped but rendered HTML looks like the original UNescaped HTML.
Using check_plain() on this line -
$variables['head_title'] = drupal_get_title();would correct the issue, but that would essentially double escape the title before unescaping once. It would be good to avoid such a dance!Comment #3
heine commentedIn a normalized DOM document (ie upon load), the value and nodevalue attributes of AttributeNodes (Attr) are the normalized values returned by the parser. So "<foo" will be normalized to "<foo".
The remedy is simple; Use Drupal.checkPlain on the value before reinserting as a string.
Comment #4
alpritt commentedUsing Drupal.checkPlain will be buggy in some situations where we want some HTML to be rendered. One such example is the node edit page which puts em tags around the content type (e.g.
<em>Edit Article</em> Example node title). Once normalized we have no way of knowing what should be rendered and what should be escaped because<foogets parsed as<fooand<fooALSO gets parsed as<foo.Escaping twice by doing
$variables['head_title'] = check_plain(drupal_get_title());solves that problem in a pretty ugly way. But I don't understand the JS well enough to know if there is a cleaner way of getting the title.Comment #5
recidive commentedThe js code is getting the title from 'title' attribute of the iframe document, which is returning the unescaped string.
Instead, it should get the content of the
<html>element.The attached patch changes to this, and apparently fixes the issue.
Comment #6
alpritt commentedUnfortunately that also normalizes the output. However, it does the opposite of using attr; i.e. both
<fooand<foois retrieved as<foo. So we end up with the same problem as if we used Drupal.checkPlain. (Again, edit a node in the overlay for an example of the problem.)Comment #7
pwolanin commentedWould it make sense to put the title as a JS setting?
Comment #8
recidive commented"Would it make sense to put the title as a JS setting?"
I think so. I've started doing that this way, but didn't get the chance to finish this yet. I should get a working patch this week, or during the weekend.
Comment #9
jpmckinney commentedAre we considering adding a JS setting like in my first attached patch? All that just to avoid double-escaping to get around the browser "feature" of unescaping attribute values? I much prefer the second attached patch. The second patch introduces a little 'wtf?', while the first patch introduces a big 'WTFH?!' in my opinion. I strongly favor the second patch.
Comment #10
jpmckinney commentedComment #11
David_Rothstein commentedThanks for starting on this.
Personally I think the current overlay behavior of hijacking the <title> tag just to pass something along to the parent window, and then grabbing it in the parent window and trying to add more stuff to it is the actual WTF :) By getting rid of that and using a JS setting, we can cleanly pass all the HTML we want to the parent window, as in the attached patch. However, we still need to keep the old behavior around as a fallback in case the JS setting isn't there, and in that case we need to run it through checkPlain().
Note that another advantage of using a JS setting is that as a followup patch (in a separate issue) we could pass not just the title itself but also its associated HTML (e.g., the title prefix/suffix, which contains among other things the "+" icon for the shortcut module) so that the parent window can cleanly display that as well. Currently, that is done via some sketchy-looking shortcut-module-specific code in overlay-parent.js, all of which would be able to go away as a result of using a JS setting.
Comment #12
jpmckinney commentedSounds good. However, you can't do the escaping on the JS side with checkPlain, you need to do it on the PHP side (see #4 above).
Comment #13
David_Rothstein commentedNo, in this case we need to do it on the JS side (since this is the fallback behavior which only runs in the edge case where we have something unrecognized displaying in the overlay - i.e., it might even have come from a totally different server).
In the normal case, though, this patch already relies on the PHP side to do the escaping, since it just reuses $variables['title'] which has already been filtered by Drupal in preparation for display.
Comment #14
kiphaas7 commentedSounds sane to me. Using a var actually makes sense, instead of hijjacking the title with jquery. It should also provide a tiny speed increase, since it's 1 DOM manip less. Yay!
Comment #15
recidive commentedThe patch seems to work as expected.
Comment #16
dries commentedHowever, we still need to keep the old behavior around as a fallback in case the JS setting isn't there, and in that case we need to run it through checkPlain().
David, I'm not sure I understand. Why wouldn't the JS setting be always there?
Comment #17
David_Rothstein commentedThe best example I can think of is something like authorize.php (which doesn't bootstrap to a high enough level to ever put the JS setting there) - see #679190: Overlay breaks the Update manager workflow
I can certainly imagine other possibilities in contrib; e.g., the much-discussed "drupal.org module browser" for plugin manager - I don't know if that would be implemented with the overlay, but it certainly might be. The parent window should always be prepared to provide a functional, safe fallback regardless of what is displaying in the child window.
Comment #18
sunThis patch is touching essential Overlay functionality and I don't see ksenzee anywhere in this issue.
The @-placeholder already invokes Drupal.checkPlain().
113 critical left. Go review some!
Comment #19
sunThis means that when no title is passed via JS, the title will contain a slogan.
Powered by Dreditor.
Comment #20
David_Rothstein commented#18 is a nice catch, although essentially a preexisting bug; title attributes should not be run through checkPlain() at all in the first place, but rather something like strip_tags() - do we have a JavaScript function that can do that? (If not, we could maybe pass the title attribute along as a JS setting too, and just do our best in the other cases.)
#19 would only happen in a maintenance page type of situation as far as I know... but I guess that does include the authorize.php example. We could look at extending http://api.drupal.org/api/function/overlay_preprocess_maintenance_page/7 or similar to deal with this, I guess.
Also, I realized that my "drupal.org module browser" example in #17 is probably a bad one, since I think that browser security features (for cross-domain iframes) will prevent grabbing the <title> tag in that case anyway... :( But the authorize.php example, error page example, as well as any non-Drupal page hosted on the same domain - all are still valid.
Comment #21
gábor hojtsyTo give some perspective, I think we originally intended to use unescaped / stripped tag values of the page title, so we used the document title but intended to not include the slogan/etc, so we did override the value in the template. However, critical escaping is indeed missing from the original code. The overriden head_title does have the page title strip-tagged, the site name check_plained and the slogan filter_xss_admin-d. I believe our original intention was to only get the page title without slogan and site name but omitted the string tagging for some reason. There have been some shuffling around in this area of escaping these items, but that does not make it less of an XSS issue.
Comment #22
sunSee also #655742: HTML HEAD title cannot be safely output differently -- a super-simple patch that allows to retrieve individual parts of 'head_title' and remix them without performance or security penalties.
Comment #23
ksenzeeThe JS setting approach seems fine to me. I wouldn't worry about #19 until we actually start putting authorize.php in the overlay. We can fix details then. I do think David's fallback code is a good idea though, so we don't code ourselves into a corner. So IMO all we need now is a fix for #18.
Comment #24
redndahead commentedFor #18 don't we still need the checkPlain because of this?
Powered by Dreditor.
Comment #25
David_Rothstein commentedAs I said above, I think #18 is a preexisting bug. We should not be check-plaining a title attribute at all, so (very very rarely) adding a second check-plain on top of the one that is already there hardly counts as a new bug when the first one shouldn't be there anyway :)
As shown in #20 and #24, the fix for this has to be somewhat independent anyway. I don't see it as a reason to hold this patch up.
Comment #26
David_Rothstein commented#19, however, may be directly caused by this patch... and may be relevant for error pages (not just hypothetically for authorize.php in the future). It's still a pretty small bug compared to the security issue, though, so I'm leaving this at "needs review" :)
Comment #27
pwolanin commentedpatch still applies (with offset).
Comment #28
casey commentedWhen #668640: Overlay shouldn't be based on jQuery UI Dialog lands, we only need to add strip_tags() to "$variables['head_title'] = drupal_get_title();" in overlay_preprocess_html().
Comment #29
ksenzeeRe #28: #668640: Overlay shouldn't be based on jQuery UI Dialog is in. Looking forward to that patch. :)
Comment #30
aspilicious commentedLike this?
Comment #31
casey commentedYes
Comment #32
David_Rothstein commentedI'm 99% sure that #668640: Overlay shouldn't be based on jQuery UI Dialog fixed the security issue (will double check), so lowering the priority here.
However, if we're going to keep this open for the remaining cleanup, we should do the whole thing. This is missing some stuff from above, plus if we're going to fix some part of this, let's just have overlay stop messing with the head title altogether rather than tinkering with how it does it :) I think the lesson from above is that it should not do that - it can only lead to trouble, plus interferes with anyone else who needs to access the original document title (e.g., #762348: Update the document title using jQuery while using the overlay window).
Will look at this more in a bit.
Comment #33
David_Rothstein commentedOK, I think something like this is what we want. I left out the part of the previous patch that changed the way the Overlay (actually Seven) hides the page title via CSS vs in a process function, since that can better be handled in a different issue, and isn't limited to just the page title anymore.
I think with this patch there are still situations where check-plained data will show up in the iframe title attribute, since even though we strip tags, drupal_get_title() may have already run check_plain() for us. Not sure if we want to deal with that.
Comment #34
casey commentedLOL, now you proposed this I realized we didn't even need to override the head_title :)
The head_title is being used for the <title> of the overlay iframe; this isn't visible anywhere. Why don't we just stop overriding it?
Comment #35
David_Rothstein commentedIt's actually also being used as the title attribute of the iframe itself (i.e.,
<iframe title="@title dialog">) for accessibility reasons - i.e., mainly for screen readers. For that purpose the un-overridden head title doesn't work well.I do wonder whether that title attribute is needed anymore (or will be once other accessibility issues are addressed), though. It looks like it was added in response to this review from a long time ago (http://drupal.org/node/517688#comment-2000950). I don't know enough about how screen readers interact with iframes in general to be able to know if it's still important, but I would assume that it is.
Comment #36
casey commentedI don't really like a JS setting for this. What about using jQuery .text() on #overlay-title?
Comment #37
David_Rothstein commentedI think that would make the functionality somewhat too dependent on the theme.
Comment #38
casey commentedWe do that all the time in Javascript; using id's and classes and expecting them to exist.
Comment #39
matt2000 commented#36 protects us from the security concern, since the unescaped title won't be displayed in the case of a non-compliant theme, and that's the core of this issue and what is blocking us from moving ahead. It seems reasonable to me, especially if there's precedent in core.
Comment #40
matt2000 commented[edit - dbl post]
Comment #41
matt2000 commentedWhile reviewing, we discovered that other menu titles in the overlay are vulnerable, not just the #overlay-title.
Example steps to reproduce:
1.) admin/structure/menu/manage/management
2.) Edit the 'System' link under Configuration, adding to the title
alert('xss');3.) From toolbar, with overlay enabled, click Configuration.
Results: XSS pop-up
So the sub-menu item titles are also affected.
#la-drupal code sprint
Comment #42
matt2000 commentedhmm. The above issue is not specific to overlay; it triggers with overlay off. Maybe it doesn't matter, since it requires administer menus to exploit? The concern would be if a user generated node were displayed in an overlay, so menu items are not especially relevant. Do I understand the issue correctly?
Comment #43
casey commentedtoolbar module was the bad apple.
Comment #44
matt2000 commentedA patch for the general menu title issue is here: #825982: admin menu blocks have XSS vulnerability
Comment #45
matt2000 commentedWith the patch in #43 and #825982: admin menu blocks have XSS vulnerability I cannot reproduce the original issue. Looks good to me.
#la-drupal code sprint
Comment #46
dries commentedCommitted to CVS HEAD. Thanks.