Ticket #3807 (closed defect: wontfix)

Opened 2 years ago

Last modified 7 months ago

Admin Functions Denying Access with "You don't have permission to do that"

Reported by: seanwedig Assigned to: anonymous
Priority: normal Milestone:
Component: Administration Version: 2.1
Severity: major Keywords: permissions has-patch 2nd-opinion
Cc:

Description

This sounds like it may be related to defect #3798, but my investigations have pointed me at other potential problems (and potential fix), so I'm submitting it separately.

I just recently did a completely new installation of Wordpress 2.1. I installed it on a local machine just fine, and ran it with a local database with no worries (I was doing theme development on a local box). On this local machine, I could log in as Admin and perform all administrative tasks I wanted with no problems, including clearing out the default blogroll items, creating new users, and whatnot.

I then went ahead and uploaded the exact same 2.1 package and installed it on a server in order to deploy the theme, but found that many of the admin functions were not working. Not all of them, but most. Those that denied access all failed with the error message "You don't have permission to do that." which I tracked down to the AJAX JS code.

I did some digging to see where in the code things were dying and causing the AJAX permission check to fail on the server (returning '-1'), and I came across code in wp-includes/pluggable.php's check_ajax_referer function. Specifically, the call to wp_login was returning false and dying with '-1', which was then denying access to execute whatever Admin function I was trying.

After debugging a little, it struck me as odd that, in order to extract the $user and $pass variables, the submitted cookie values were being manually parsed out from $_POSTcookie?. This was in check_ajax_referer.

When I replaced manual parsing with pulling USER_COOKIE and PASS_COOKIE from the $_COOKIE variable, it appears to have fixed my problem. (I apologize for not submitting a diff for WP's purposes - I'm not exactly sure how it should be generated, but I am glad to learn!)

I think it came down to the parsing based on string position of an equal sign. The hashed cookie keys may have sometimes included that equal sign, and so messed up the manual parsing; I'm not 100% sure on that - it is just speculation. I'm willing to accept that I've got it all wrong, as I do not know the WP code. :)

To be precise, I replaced lines 244 through 250 of wp-includes/pluggable.php

  $cookie = explode('; ', urldecode(empty($_POST['cookie']) ? $_GET['cookie'] : $_POST['cookie'])); // AJAX scripts must pass cookie=document.cookie
  foreach ( $cookie as $tasty ) {
	  if ( false !== strpos($tasty, USER_COOKIE) )
			$user = substr(strstr($tasty, '='), 1);
		if ( false !== strpos($tasty, PASS_COOKIE) )
			$pass = substr(strstr($tasty, '='), 1);
	}

with

  $user = $_COOKIE[USER_COOKIE];
  $pass = $_COOKIE[PASS_COOKIE];

and it appears to have fixed the problem.

-Sean

Attachments

pluggable.php.diff (1.0 kB) - added by basvd on 06/03/07 23:31:03.
Works around Suhosin cookie encryption, which is causing the problem.
3807.diff (1.1 kB) - added by basvd on 06/06/07 17:46:29.
This patch is supposed to be safer.

Change History

03/05/07 21:11:20 changed by foolswisdom

  • milestone set to 2.2.

03/27/07 18:51:40 changed by foolswisdom

  • milestone changed from 2.2 to 2.4.

06/03/07 23:31:03 changed by basvd

  • attachment pluggable.php.diff added.

Works around Suhosin cookie encryption, which is causing the problem.

06/03/07 23:31:39 changed by basvd

  • keywords changed from permissions to permissions has-patch 2nd-opinion.

06/04/07 03:28:11 changed by ryan

My memory is foggy, but I think the cookies are passed to act as a nonce. This code predates our addition of proper nonces. I think we can change this to use _COOKIE for the login auth and a separate nonce for XSRF protection. I'll check with mdawaffe.

06/04/07 20:18:44 changed by mdawaffe

It's one level of protecting against forged requests that come from the same domain. Other levels include using POSTs and kses.

If we had any GET based AJAX requests, for example, someone could write a link that when clicked would add something to the blogroll (or whatever) since the admin cookie would be there in $_COOKIE.

Requiring that the cookie also be found in $_REQUEST and only handling POST AJAX requests ensures that all requests come either from a form or from JS, both of which are removed by kses.

That's why. What you mention is the why not :)

I think we can shift to using nonces now that we have them. In fact, most of the AJAX stuff we do is parallel to normal form POSTs and so a nonce is already available for most things. Also, I think autosave has a JS function which could be mooshed into requesting fresh nonces for whatever we need. It should get a security review and be included as a method in the wpAjax JS object.

06/04/07 20:35:14 changed by mdawaffe

To clarify, switching to use only $_COOKIE authentication (i.e. without nonces or the $_POST trick above) would be less secure and could open up a hole.

06/04/07 22:17:49 changed by basvd

Alright, thanks for the clarification.

The cause of this bug is actually a PHP security patch/extension called Suhosin. I wrote about the issue in detail on my blog.

Using $_COOKIE would indeed be more risky (although I doubt it if Suhosin is running).
Anyhow, I am currently talking to the developer(s) of Suhosin about implementing a cookie decryption function (natively in Suhosin or in PHP userspace) which can be applied to any encrypted string which is known to be cookiedata.

If this is implemented in Suhosin, we could use it to further minimise this bug.

06/06/07 17:46:29 changed by basvd

  • attachment 3807.diff added.

This patch is supposed to be safer.

07/21/07 18:46:11 changed by Nazgul

Adding this change to core would benefit only a small part of the userbase (those that use Suhosin) and seeing the check_ajax_referer function is in pluggable.php, it can be overriden. Therefore I think this would be best solved as a plugin.

08/21/07 14:19:57 changed by basvd

I agree. I will try to provide a safe fix using a plugin soon.

08/21/07 18:19:07 changed by basvd

The fix is now available as a plugin.

01/24/08 19:32:42 changed by ninjaWR

  • status changed from new to closed.
  • resolution set to wontfix.
  • milestone deleted.

only an issue on certain server configs. basvd's plugin fixes the issue