My patch that was committed to 6.x attached (only affects book module and should be RTBC). Also attached is my patch to form.inc. This needs more detailed consideration and a possible backport to 6.x if it goes into 7.x
original report:
While working on a fix for #277620: Cannot have double quotes in book page title (title disappears when editing book hierarchy) I found out that the title is used as part of the form IDs on the outline reordering page, without any checks. I realize now that a malicious user with the rights to create children for books, can therefore insert any HTML into that page obtaining for example the adminisdtrators session key.
The behavior is in D6 (tested with 6.5) and D7, and has to do with the new menu system introduced in D6. It is not present in D5 (tested with 5.10), as far as I have tested.
How to reproduce
-------------------------
- Create a book
- Create a child page with a malicious title, e.g. '">alert("XSS");' (without the single quotes, but with the leading double quote)
- Go to 'edit order and titles' for that book. (?q=admin/content/book)
- behold: XSS attack!Where is the problem?
-------------------------------
On line 174 of book.admin.inc you have the following line
$form[$key] = array(The $key is changed from node id to elaborate string via _menu_tree_check_access() called in menu_tree_check_access(), which in turn is called in book.module's book_menu_subtree_data() on line 1090. _menu_tree_check_access() sets the $key to (50000 + $item['weight']) .' '. $item['title'] .' '. $item['mlid']. As you can see $item['title'] isn't escaped or anything. Normally that wouldn't be a problem, as the key wasn't designed to be visual to the user, but the Book module does use that key as it's form elements keys.
How to solve?
-------------------
Either escape the $key in _menu_tree_check_access(), in _book_admin_table_tree. Or replace the $key in _book_admin_table_tree entirely by something unique like the node id (e.g. the mlid). This last behavior is what _menu_overview_tree_form() does, a form similar to the form at admin/content/book/1. It is also the way D5 does it.If you need more information or have other questions, please don't hesitate to reply.
Sincerely,
Maarten van Grootel (maartenvg on d.o)
--
Comment | File | Size | Author |
---|---|---|---|
#2 | book-fix-sdo545-7x-1.patch | 837 bytes | pwolanin |
form-fix-sdo545-2.patch | 1.31 KB | pwolanin | |
book-fix-sdo545-1.patch | 836 bytes | pwolanin | |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedComment #2
pwolanin CreditAttribution: pwolanin commentedre-rolled for 7.x
Comment #3
webchickOk, committed patch in #2 to HEAD. Thanks!
I've asked chx to chime in on the second approach. This might fall under the "babysitting" clause, or it might be a good idea. It seems like we need something more stringent than check_plain though, like form_clean_id()?
Comment #4
pwolanin CreditAttribution: pwolanin commentednew issue to discuss the forms.inc patch: http://drupal.org/node/331879
Comment #6
pwolanin CreditAttribution: pwolanin commentedtagging