Make WordPress Core

Opened 17 years ago

Closed 16 years ago

#3807 closed defect (bug) (wontfix)

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

Reported by: seanwedig's profile seanwedig Owned by:
Milestone: Priority: normal
Severity: major Version: 2.1
Component: Administration Keywords: permissions has-patch 2nd-opinion
Focuses: 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 (2)

pluggable.php.diff (1.0 KB) - added by basvd 17 years ago.
Works around Suhosin cookie encryption, which is causing the problem.
3807.diff (1.1 KB) - added by basvd 17 years ago.
This patch is supposed to be safer.

Download all attachments as: .zip

Change History (13)

#1 @foolswisdom
17 years ago

  • Milestone set to 2.2

#2 @foolswisdom
17 years ago

  • Milestone changed from 2.2 to 2.4

@basvd
17 years ago

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

#3 @basvd
17 years ago

  • Keywords has-patch 2nd-opinion added

#4 @ryan
17 years ago

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.

#5 @mdawaffe
17 years ago

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.

#6 @mdawaffe
17 years ago

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.

#7 @basvd
17 years ago

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.

@basvd
17 years ago

This patch is supposed to be safer.

#8 @Nazgul
17 years ago

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.

#9 @basvd
17 years ago

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

#10 @basvd
17 years ago

The fix is now available as a plugin.

#11 @ninjaWR
16 years ago

  • Milestone 2.5 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.