Fix SA-CONTRIB-2014-024

This patch looks good to me. It's well documented and I think it solves the problem well. I tested with a clean install with the content lock module (and devel so I could generate some dummy content) and made a couple of test html pages to test for protection against CSRF:

• An image file with src with something like: http://[website url]/node/12/edit
• An iframe that loads a form from another html file that auto-submits, with the action = to something like: http://[website url]/node/13/edit

I see that both resulted in Access denied without the token, so that's great! I also checked out the other areas where the node/edit would appear (Like the edit tab when viewing the node, or the edit links on /admin/content, and they all have the token appended to the url. Good job!

Looks good to me as well - tried both the img and iframe tests and simply got "You are not authorized to access this page" even though my active user had the correct role to edit - but I had no token so was denied...

I have a big objection against the approach that is taken here. It will make it impossible to access node/%/edit without passing the token. This breaks a very wide spread user pattern. People access this path all the time by entering it in the address bar of their browser. There are many users that have been trained to use this URL. Also this will break existing automated tests which access this path.

Instead, you should simply display the message about the lock not being able to be set if the token is missing. Then the CSRF vulnerability is fixed, we don't lose valuable functionality, don't have to retrain our users, and existing tests do not need to be rewritten.

To make this even better, you can provide an ajax link that will lock the node for you, something like this:

The page you are editing could not be locked automatically. Please lock the page to make sure other people cannot accidentally overwrite your changes.

I also did a code review:

1. +++ b/content_lock.module
   @@ -93,6 +93,81 @@ function content_lock_menu() {
   +function content_lock_menu_site_status_alter(&$page_callback_result, $read_only_path) {
   +  if (content_lock_is_path_protected($read_only_path)) {
   +    if (!isset($_GET['content_lock_token']) || !drupal_valid_token($_GET['content_lock_token'], $read_only_path)) {
   +      $page_callback_result = MENU_ACCESS_DENIED;
   +    }
   +  }
   +}
   

   Why call the second parameter $read_only_path? What does it mean that this is "read only"?

2. +++ b/content_lock.module
   @@ -93,6 +93,81 @@ function content_lock_menu() {
   + * Check if an internal Drupal path should protected with a token.
   + *
   

   ...should "be" protected...

3. +++ b/content_lock.module
   @@ -93,6 +93,81 @@ function content_lock_menu() {
   + * Adds requirements that certain node editing pages be accessed only
   + * through tokenized URIs which are enforced by the
   + * protection_menu_token module. This prevents people from being
   + * CSRFed into locking nodes they have access to without meaning to.
   + *
   

   This mentions the protection_menu_token module, but this is not actually being used.

4. +++ b/content_lock.module
   @@ -93,6 +93,81 @@ function content_lock_menu() {
   +  $protected = array('system' => FALSE);
   +  foreach (module_implements('content_lock_path_protected') as $module) {
   

   Why set system to FALSE? Indeed it is unlikely that the system module will ever implement this hook, but it seems a bit strange.

5. +++ b/content_lock.module
   @@ -256,11 +331,28 @@ function content_lock_form_alter(&$form, &$form_state, $form_id) {
   +          drupal_set_message(t('Unable to lock !node for editing because this page load was not protected against CSRF attacks. You still may !edit node using the normal node editing page.', array(
   +            '!node' => l(t('%node_title', array('%node_title' => $node_title)), 'node/' . $nid, array('html' => TRUE)),
   

   I wouldn't mention CSRF attacks in end-user facing messages. It is likely that they have no idea what this means. Also, this now includes a link to the regular node/%/edit page, but since we are on a custom page with a node edit form it is very likely that the developer did not intend the user to use node/%/edit in this particular situation, so don't point the user there.

6. +++ b/content_lock.module
   @@ -256,11 +331,28 @@ function content_lock_form_alter(&$form, &$form_state, $form_id) {
   +          // Finally set the lock if everthing passed.
   +          if (content_lock_node($nid, $user->uid) == false) {
   

   Now that this line is touched, you can fix the typo: everthing.

7. +++ b/content_lock.module
   @@ -256,11 +331,28 @@ function content_lock_form_alter(&$form, &$form_state, $form_id) {
   +            // could not lock node, it's locked by someone else
   +            drupal_goto($destination);
   

   Now that this line is touched, the comment should start with a capital letter and end with a period.

Still needs work, my main point is not addressed.

In reference to the strategy change to make it only a message - How would you validate the request without a token?

I think you misunderstood me. You would have a token every time a user clicks on a link leading to the node add/edit form, which is in 90% of the cases. This token is added in template_preprocess_link(). However clicking links is not the only mechanism that users employ to navigate sites, they also type in URLs directly in the address bar, use bookmarks, or keep lists of links to pages they often have to update etc. All these would stop working when this patch gets in its current form. The user would suddenly see an access denied error which is very disrupting, and they will assume something is terribly wrong.

Access to the form should not be denied just because the token is not there as there might be legitimate reasons for it to be missing. Thanks to the fallback mechanism I described above, the user can continue working and can still lock the node if so desired.

Also don't underestimate the number of automated tests that would need to be rewritten because they access the node add/edit form directly and would now suddenly land on a 403 page because the token is not yet programmed in. This module is used by thousands of websites, so that potentially means tens of thousands of tests that need to be rewritten across all those projects.

The strategy in this patch is the same as https://drupal.org/node/2207025 which required the protection_menu_token module to protect the paths.

Then that should be addressed as well. I see in that issue that the patch has never been reviewed, so don't rely on it being the one true approach ;)

I reviewed the updates you make, and all looks good, but I would tweak this message a bit further:

Unable to lock !node for editing because this page load was not protected. You still may !edit node using the normal node editing page.

This is better than the scary CSRF acronym, but "the page load was not protected" is still not very clear to the end user, how are they supposed to know what one needs to do to protect a page load? Also don't put the link to the standard node edit form there. That's just making it more confusing.

I will update the patch with my proposal. I will work on this later today or tomorrow at the latest.

Updated the patch with my proposed approach. I also implemented the ajax callback to allow people to manually lock a node if it could not be locked automatically due to a missing CSRF token.

I tested content_lock-fix-SA-CONTRIB-2014-024-2238703-6.patch and content_lock-fix-SA-CONTRIB-2014-024-2238703-11.patch against a clean install of 7.27, with Devel and Content Lock as the only contrib modules. I tested against CSRF using the same files as @mirie.

content_lock-fix-SA-CONTRIB-2014-024-2238703-6.patch worked as expected, tokens in the right places, and 403s if you tried to access node/xx/edit without a tokenized URL.

content_lock-fix-SA-CONTRIB-2014-024-2238703-11.patch worked almost as expected. tokens in the right places, tokenized URLs setting new locks, respecting existing locks, and breaking existing locks. However I received a 404 error (below), when trying to manually clicking the AJAX link set a lock on a non-tokenized URL.

An AJAX HTTP error occurred.
HTTP Result Code: 404
Debugging information follows.
Path: /ajax/content_lock/29/lock/_RySDm2D0Jyd9BYoDxOvEG4abXPiCw456YYNa3ghbK4

@pfrenssen: I get your methodology and think it has merit, however it would be great if we could get this module out of Security Hole status before implementing new features. Perhaps we could use @recrit's patch as a stepping stone, get the project out of the (literal) red, and then push @pfrenssen's idea as a feature for the next release?

Alternately, @pfrenssen, maybe you could reroll your patch without the AJAX lock feature for now. That would prevent automated tests and existing bookmarks from breaking. It would keep people from locking new nodes, but in those use cases, it is unlikely that people will encounter an edit collision - and this would be a short term issue.

Basically, move forward with patching the security issue, and suss out new features for the module later.

@milsyobtaf, did you clear your cache after applying the patch? That path is registered in hook_menu() so it should be picked up after clearing your cache.

@pfrenssen - You are correct, I failed to clear my cache fastidiously enough as I swapped git branches. With a clean cache the manual lock link is, in fact, working. It does not seem to give a message about the lock being set to the person who sets the lock, however.

Other logged in users see a message that the lock is set, but the person setting the lock doesn't get visual feedback other than the "The page you are editing could not be locked automatically" message being cleared from the page.

I think with the addition of feedback to the manual lock placement action, this would be a great patch to consider including into the module codebase.

There is a message being output, but this is injected in the page before the element with id '#main-content'. In the core themes (Garland, Bartik, Seven, etc.) and most contributed base themes this is where the messages are displayed by default.

I now realize that this is of course not enforced, themes are free to implement their own markup the way they like, and it is possible that '#main-content' is not available. I will see if this can be addressed somehow. I found an example in core that uses '#block-system-main' instead, I suppose that could be more reliable. I also could replace the messages themselves since we can be certain that they are displayed, but these are also themable so the markup may change.

Updated the patch. This should show the message on most, if not all, themes.

has someone asked to be maintener so he could apply this patch ?

Tested content_lock-fix-SA-CONTRIB-2014-024-2238703-17.patch. Functionality works fine, as before. Feedback from manual lock placement is appearing as expected in Bartik.

Looks good to me, @pfrenssen, now to see about a maintainer.

@quazardous, yes there is an issue about it in the webmaster's queue, ergonlogic applied to become a co-maintainer: #2207025: Transfer ownership of 'Content Locking' to ergonlogic.

@recrit, that seems to make sense. I did not see this in my own testing though. I probably did not have a different admin theme selected.

Finally had a second to test the latest patch, content_lock-fix-SA-CONTRIB-2014-024-2238703-20.patch. Functionality works fine, as before. Feedback from manual lock placement is appearing as expected in Bartik (as both default and admin theme) and Seven (as admin theme, with Bartik as default). Thanks for the good work, @recrit and @pfrenssen.

Tested the patch.. One strange thing happened, I don't know if related to this patch or not.

So I test with uid 1, edit a page:

This document is now locked against simultaneous editing. It will unlock when you navigate elsewhere.

Looks ok.

I take another browser, log in with another user (also administrator though):

The page you are editing could not be locked automatically. Please lock the page to make sure other people cannot accidentally overwrite your changes.

I click "lock the page":

This document is locked for editing by admin since vr, 23/05/2014 - 10:14.
Click here to check back in now.

I then click "here", get the usual "If you proceed ALL changes will be lost".
What follows then is a page with what appears to be a json string, url = 'ajax/content_lock/37946/lock/-HX7ar_hz8QbtwKpDnkknKA8OxhYQVGD_xI_IoRDhac'
Page content:
[{"command":"settings","settings":{"basePath":"\/","pathPrefix":"","ajaxPageState":{"theme":"seven","theme_token":"azppTm57jLuQLvooDzzqDQV7D8Q20VS60CPJ-KVOY4g"},"content_lock":{"nid":"37946","ajax_key":"993895141","token":"8yKZprh0Ml3fBQxmH2WIAuGz0fsFRJp6GjKpkedgoNQ","unload_js_message_enable":true,"internal_urls":"node\/37946\/edit","internal_forms":"form.node-form","unload_js_message":"If you proceed, ALL of your changes will be lost."}},"merge":true},{"command":"remove","selector":"div.messages"},{"command":"insert","method":"before","selector":"#block-system-main","data":"\u003Cdiv class=\u0022messages error\u0022\u003E\n\u003Ch2 class=\u0022element-invisible\u0022\u003EFoutmelding\u003C\/h2\u003E\n \u003Cul\u003E\n \u003Cli\u003EThere is a security update available for your version of Drupal. To ensure the security of your server, you should update immediately! See the \u003Ca href=\u0022\/admin\/reports\/updates\/update\u0022\u003Eavailable updates\u003C\/a\u003E page for more information and to install your missing updates.\u003C\/li\u003E\n \u003Cli\u003EThere are security updates available for one or more of your modules or themes. To ensure the security of your server, you should update immediately! See the \u003Ca href=\u0022\/admin\/reports\/updates\/update\u0022\u003Eavailable updates\u003C\/a\u003E page for more information and to install your missing updates.\u003C\/li\u003E\n \u003C\/ul\u003E\n\u003C\/div\u003E\n\u003Cdiv class=\u0022messages status\u0022\u003E\n\u003Ch2 class=\u0022element-invisible\u0022\u003EStatusbericht\u003C\/h2\u003E\n \u003Cul\u003E\n \u003Cli\u003EThe editing lock held by \u003Ca href=\u0022\/user\/1\u0022 title=\u0022View user profile.\u0022 class=\u0022username\u0022 xml:lang=\u0022\u0022 about=\u0022\/user\/1\u0022 typeof=\u0022sioc:UserAccount\u0022 property=\u0022foaf:name\u0022 datatype=\u0022\u0022\u003Eadmin\u003C\/a\u003E has been released.\u003C\/li\u003E\n \u003Cli\u003EThis document is now locked against simultaneous editing. It will unlock when you navigate elsewhere.\u003C\/li\u003E\n \u003C\/ul\u003E\n\u003C\/div\u003E\n","settings":null}]

@rv0. do you perhaps have javascript disabled? That might be an issue.

@pfrenssen not at all, the other ajax links work fine.

Later I ran into an error on line 462, something like "trying to access property of non object" or at least "undefined property" .. cant remember what it was, but i happened to have the file still opened with the offending line selected ($lock->ajax_key)
I could not reproduce this afterwards, so thats why I didn't post back here right away, but it may give a clue.

Tested the patch at #20, seems to work fine.
I couldn't recreate the issues mentioned in #23 (tried various users, same user, root account). Everything works as expected.

I've tested patch at #20 as well and anything works just fine. Does somone would take ownership of this module?

@sg88, here is the issue about the project ownership, you might want to follow that: #2207025: Transfer ownership of 'Content Locking' to ergonlogic.

I've also reviewed this patch and it's working as intended.

I was able to recreate the error described in #23 by applying the patch and then visiting a node/edit page without clearing caches. I think the problem was just that caches needed to be cleared. Clearing them resolved the issue for me.

Nice work on this guys :)

@jenlampton
clearing cache may have been the problem indeed. We're using the patched version in production and no issues have been reported since.

Just a quick note to future visitors, the latest patch in #20 only applies to 7.x-1.4, not dev.

Im just coming across this module now. Am I correct in assuming that once this patch is committed it will go back to becoming a supported module?

@loze, no that is a separate issue. The module is "unsupported" because it currently doesn't have a maintainer that looks after it. Some time ago there was a candidate, but that seems to have lost momentum. If you want to keep up to date when the module will become supported again you can follow issue #2207025: Transfer ownership of 'Content Locking' to ergonlogic.

+++ b/content_lock.module
@@ -256,10 +331,40 @@ function content_lock_form_alter(&$form, &$form_state, $form_id) {
+        // Refuse to lock if a CSRF token is not present. This prevents a denial
+        // of service attack if a user is tricked into visiting pages that cause

I don't think denial of service is the right term here. Its a CSRF attack.
Other than that looks good.

Good catch! Fixed that.

I'd missed that the transfer happened. Thanks for the head's up, @kattekrab.

@pfressen, if the only change in #35 is the doc string from #34, as the interdiff suggests, it should probably remain RTBC :)
However, #29 suggests that a cache clear is warranted. Perhaps we should add one in an update hook. More concerning is that #31 suggests that the patch doesn't apply against head.

Anyway, I'll work on this today, and see if we can't get a new (secure) release out shortly. Thanks for everyone's work on this.

I created a new branch (7.x-2.x) and a new release (7.x-2.0). I'm just waiting on the security team to unlock the project, so I can update the project page and so forth.

I'll be adding some co-maintainers shortly. If you're interested, feel free to contact me directly, or through an issue.

Could you test it again? I think the input format was set to "full html".

Thanks @dddave, that worked.

I'm marking this issue as fixed, since the patch was committed and a new release is just pending the security team's intervention.

FYI, the new 7.x-2.0 release is now available for download.

@ergonlogic can ask around at work to try and find someone to help with maintenance