Make WordPress Core

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#16449 closed defect (bug) (fixed)

incorrect referer check in check_admin_referer()

Reported by: indie-ulf's profile indie-ulf Owned by: markjaquith's profile markjaquith
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.0.4
Component: Security Keywords: has-patch needs-testing
Focuses: Cc:

Description

The check_admin_referer() function is defined this way:

"Tests if the current request was referred from an admin page, or (given $action parameter) if the current request carries a valid nonce. Used to avoid security exploits."

The older, less secure form with no parameter ("check_admin_referer()") still works, it's not documented as deprecated anywhere, and at least one plugin with more than 190,000 downloads uses it.

The problem is that it's not secure. An attacker can fool it easily like this:

1) Put up pages "one.html" and "two.html" on evilsite.com, where "one.html" includes "two.html" in an iframe, and "two.html" performs a CSRF attack against the admin part of a plugin in Victim's WP install.

2) "one.html" should include Victim's WP admin URL in the query string for the URL given for "two.html" in the iframe ( http://evilsite.com/two.html?foobar=http://victim.vi/wp-admin/ ). The Referer check in check_admin_referer() erroneously uses strpos() !== false, so it will be fooled by a Referer that looks like this.

3) Entice a logged-in administrator to visit http://evilsite.com/one.html .

I have attached a patch that should correct this issue by changing the strpos() call. I hope it won't break anything..

Ulf Harnhammar

Attachments (3)

incorrect_referer_check.patch (536 bytes) - added by indie-ulf 14 years ago.
patch
check-admin-referer-test.php (2.6 KB) - added by markjaquith 14 years ago.
Plugin to test the patch.
check_admin_referer-notice.diff (705 bytes) - added by duck_ 14 years ago.

Download all attachments as: .zip

Change History (11)

#1 @markjaquith
14 years ago

Good catch. In the future, please send security-related items to security / wordpress / org.

The older, less secure form with no parameter ("check_admin_referer()") still works, it's not documented as deprecated anywhere

We should throw a deprecated notice if it is used without a parameter.

To be clear, this is not an issue in core, as no active code uses the function without a parameter. But it would be a security bonus to plugins that haven't been updated to use nonces.

Last edited 14 years ago by markjaquith (previous) (diff)

#2 @markjaquith
14 years ago

  • Milestone changed from Awaiting Review to 3.1

Probably should fix this in 3.1 (and 3.0). The patch looks good.

#3 @markjaquith
14 years ago

We had an old stubbed out code block that used this... it didn't do anything. I removed it.

[17381] & [17382] Remove unused/non-functional code with old-style CSRF checking. see #16449

#4 @markjaquith
14 years ago

  • Owner set to markjaquith
  • Status changed from new to accepted

Proposal: Fix it for 3.1 and 3.0. Deprecate calling it with no first parameter in 3.2.

@markjaquith
14 years ago

Plugin to test the patch.

#5 @markjaquith
14 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [17385]) Improvement to check_admin_referer() when called without first argument (PLUGIN AUTHORS: STOP DOING THAT). props indie-ulf. fixes #16449 for trunk

#6 @markjaquith
14 years ago

(In [17386]) Improvement to check_admin_referer() when called without first argument (PLUGIN AUTHORS: STOP DOING THAT). props indie-ulf. fixes #16449 for 3.1

#7 @markjaquith
14 years ago

(In [17387]) Improvement to check_admin_referer() when called without first argument (PLUGIN AUTHORS: STOP DOING THAT). props indie-ulf. fixes #16449 for 3.0

#8 @markjaquith
14 years ago

In [18195]:

Throw _doing_it_wrong() when nonce action not passed to check_admin_referer(). props duck_. see #16449

Note: See TracTickets for help on using tickets.