Make WordPress Core

Opened 18 years ago

Closed 18 years ago

Last modified 9 years ago

#2678 closed defect (bug) (fixed)

Nonces instead of referers

Reported by: ringmaster's profile ringmaster Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.0.2
Component: Administration Keywords: bg|has-patch
Focuses: Cc:

Description

The WordPress admin should use nonces instead of checking referers to prevent CSRF attacks because of the improved usabililty provided by nonces.

Patch includes replacement check_admin_referer() function that uses nonces instead of verifying referers. check_admin_referer() now accepts a nonce action as an optional parameter, which is used to verify the incoming nonce.

Several new functions in functions.php create and verify nonces and facilitate their use. For example, to modify a url to add a nonce, call wp_nonce_url($url, $action), where $action is the action to be verified by the nonce.

The patch makes modifications only to employ a nonce for deletion of posts when js is disabled on the Manage Posts page. Also, the inline-upload.php has been modified slightly so that urls it generates are more nonce-friendly. (inline-upload.php calls check_admin_referer() even when no input is expected!)

Plugins should not be affected by this change unless they call check_admin_referer(), in which case they will need to add nonces to the URLs that they generate so that they can be verified.

Note that not including a nonce does not automatically fail as with the prior code. Instead, an "Are you sure?" message appears with Yes and No options that forward the original request with a nonce attached.

Thanks to mdawaffe for the initial run at the new check_admin_referer() and masquerade for the time-based nonce code.

Please test.

Attachments (8)

nonce.2.diff (8.0 KB) - added by ringmaster 18 years ago.
Better (working) patch. I hope.
nonce.3.diff (7.9 KB) - added by masquerade 18 years ago.
A third, better patch, some changes suggested by mdawaffe
nonce.4.diff (7.9 KB) - added by masquerade 18 years ago.
Bugfix on 3 from random
nonce.diff (8.9 KB) - added by ryan 18 years ago.
Make nonce creation/verification pluggable. nonce post editing.
nonce.5.diff (14.5 KB) - added by ryan 18 years ago.
More noncification.
nonce.6.diff (28.5 KB) - added by ryan 18 years ago.
Nonce comments, pages, and options.
2678inline.diff (6.0 KB) - added by mdawaffe 18 years ago.
Better nonces for inlineuploading
2678-posts-cats.diff (2.8 KB) - added by mdawaffe 18 years ago.
Nonces for post deletion from post.php and category deletion from Manage->Categories

Download all attachments as: .zip

Change History (51)

#1 @ringmaster
18 years ago

I should clarify that not every admin form has had the nonces added, just the ones mentioned in the description above.

@ringmaster
18 years ago

Better (working) patch. I hope.

#2 @mdawaffe
18 years ago

After vetting, the ultimate goal would be to nonce all state changing admin requests. Correct?

The confirmation dialog in check_admin_referer() could supplant confirmdeletecomment and mailapprovecomment. With some effort.

#3 @ringmaster
18 years ago

Yes. All state changing admin requests (via form or link) should include nonces, and all code that performs those changes should be protected by a matching check_admin_referer(). Pages that don't perform such changes shouldn't include check_admin_referer(), since it will make it unnecessarily difficult to link to those pages.

Perhaps an optional second argument to check_admin_referer() that would help it decide what to do in the event of a failure? That way, admin panels like the comment approval/deletion could supply their own confirmation dialogs. Perhaps it could be like:

if(check_admin_referer('confirmdeletecomment', true)) {
// delete comment
}
else {
// display custom confirmation
}

Thoughts?

#4 @masquerade
18 years ago

+1 on the last idea.

#5 @mdawaffe
18 years ago

We'd get rid of confirmdeletecomment entirely:

if ( check_admin_referer( 'deletecomment', true ) )
// del0rted
else
// custom confirmation

But yes.

It would be nice, though, if check_admin_referer() could display something about the action it's checking even without a custom confirmation so that the user doesn't just see "Are you sure? [No] [Yes]".

Would it be possible to standardize the actions and filenames enough so that we could say:

"You are trying to (delete|edit|switch to|...) the (post|comment|theme|...) (titled|by|...) '(post_title|comment_author|theme_name|...)'. Do you want to proceed? [Cancel] [(Delete|Edit|Switch|...) (post|comment|theme|...)]"

Writing custom dialogs for everything is annoying, but the default dialog is a little sparse right now. Is this more pain that it's worth?

#6 @mdawaffe
18 years ago

-check_admin_referer( 'deletepost' );
+check_admin_referer( 'deletepost' . $post_id );

Otherwise, if someone intercepts a nonce, that nonce is good for deleting mulitple arbitrary posts within a certain time interval.

We should restrict each nonce such that it can only authenticate one specific action.

#7 @ryan
18 years ago

current_nonce_can('delete_post', $post_id);

:-)

#8 @ringmaster
18 years ago

@mdawaffe: I think figuring out how to stuff custom dialogs into check_admin_referer() will be more trouble than it's worth. If absolutely necessary, perhaps an optional third parameter:

check_admin_referer('dosomething', false, 'do something really funky');

Resulting in:

You're trying to do something really funky.
Are you sure you want to do something really funky?

That could wreak havok on the translators, though.

There was a reason I had concocted for not passing discrete actions into check_admin_referer(), but I can't recall it now, and your code looks good. I like that idea. +1 for that.

@ryan: You're nuts, man. ;)

#9 @random
18 years ago

Can wp_create_nonce and wp_verify_nonce be pluggable? Let people just drop in sha32768 or whatever if md5 isn't enough for them. :)

#10 @random
18 years ago

I can't see where DB_PASS is defined. Should that be DB_PASSWORD?

#11 @westi
18 years ago

+1 to this.

+1 to pluggable wp_create_nonce and wp_chechk_nonce so that people can go to database stored onetime valid nonces if they want.

@masquerade
18 years ago

A third, better patch, some changes suggested by mdawaffe

#12 @masquerade
18 years ago

Attached is diff 3, which fixes DB_PASSWORD from Owen's original patch, changes some of the computational nonce code so that the scale is 12 hours to 24 hours (can't believe I didn't realize that before, mdawaffe).

#13 @random
18 years ago

Minor thing: in diff 3, wp_verify_nonce() has:

|| substr(md5(($i - 1) . DB_PASSWORD . $action . $uid), -12, 10)

instead of:

|| substr(md5(($i - 1) . DB_PASSWORD . $action . $uid), -12, 10) == $nonce

@masquerade
18 years ago

Bugfix on 3 from random

#14 @ryan
18 years ago

Looking good to me. Another +1 for making create and verify pluggable.

To ease transition for plugins, especially if this goes into 2.0.3, can we fallback to the old referrer check if an action is not specified? If an action is specified, we would insist on a nonce and only a nonce since this safeguards untrusted links present on an admin page by requiring confirmation. All checks in WP itself would specify an action, of course. Only old plugins would use the less secure fallback-to-referrer method.

#15 @ringmaster
18 years ago

ryan: As far as I looked, plugins never pass through cheack_admin_referer() unless they call it themselves. (All "page=" handling is done in admin.php and then exit()s, before the core admin page is even fully executed.) So it wouldn't make things more backwards compatible if we added that check. I had assumed that we did that, but apparently we don't. It's actually one reason why adding this patch will run more smoothly than we thought.

+1 for pluggable creates and verifies from me, too. Let the vocal non-contributors wait for someone else to write something "better".

@ryan
18 years ago

Make nonce creation/verification pluggable. nonce post editing.

#16 @ryan
18 years ago

Patch to make create and verify pluggable and nonce the post edit form.

#17 @davidhouse
18 years ago

  • Keywords bg|has-patch added

#18 @SilverPaladin
18 years ago

This solution tries to use a time check, but the logic doesn't work.

This code here is the problem:

$i = ceil(time() / 43200);

That takes the number of seconds since January 1 1970 00:00:00 GMT and counts how many 30 day chunks there have been. The nonce evaluation simply checks to see if you are in the same 30 day chunk of time.

It does not say "this nonce is valid for 30 days." In fact, if you visit this site with only one second left in the 30 day chunk, you will have 1 second in which to do all of your work.

Additionally, all days within that thirty day chunk evaluate as being the same chunk. So, as salt, $i really does nothing.

In security applications, Nonces should be valid for as short a time as possible. When used as a session key, this normally means a nonce is valid for minutes, not days.

#19 @SilverPaladin
18 years ago

This solution tries to use a time check, but the logic doesn't work.

This code here is the problem:

$i = ceil(time() / 43200);

That takes the number of seconds since January 1 1970 00:00:00 GMT and counts how many 30 day chunks there have been.

The nonce evaluation simply checks to see if you are in the same 30 day chunk of time. It does not say "this nonce is valid for 30 days." In fact, if you visit this site with only one second left in the 30 day chunk, you will have 1 second in which to do all of your work.

Additionally, all days within that thirty day chunk evaluate as being the same chunk. So, as salt, $i really does nothing right now. So I don't think a solution of "just check the next chunk too" is a good one. A nonce that can be valid for two months is not really time based, imho.

Generally speaking when used for security purposes in applications, Nonces should be valid for as short a time as possible. When used as a session key, this normally means a nonce is valid for minutes, not days.

#20 @masquerade
18 years ago

This solution tries to use a time check, but the logic doesn't work.

Sure it does

This code here is the problem: >$i = ceil(time() / 43200);
Not quite, keep reading

That takes the number of seconds since January 1 1970 00:00:00 GMT and counts how many 30 day chunks there have been.

No, since when was 43200 seconds 30 days? Last I checked, 43200 / 60 (seconds) / 60 (minutes) = 12 hours.

The nonce evaluation simply checks to see if you are in the same 30 day chunk of time. It does not say "this nonce is valid for 30 days." In fact, if you visit this site with only one second left in the 30 day chunk, you will have 1 second in which to do all of your work.

You didn't look at the logic which checks, which also checks $i-1 against the hash, making the lifetime of the hash 12 hours to 24 hours.

Additionally, all days within that thirty day chunk evaluate as being the same chunk. So, as salt, $i really does nothing right now. So I don't think a solution of "just check the next chunk too" is a good one. A nonce that can be valid for two months is not really time based, imho.

Months are far too long, which is why no nonces last more than a day.

Generally speaking when used for security purposes in applications, Nonces should be valid for as short a time as possible. When used as a session key, this normally means a nonce is valid for minutes, not days.

43200/120 minutes is much better than your accused 30 days.

#21 @ringmaster
18 years ago

By my calculations, $i would change every 12 hours, not every 30 days. That makes the window 24 hours when checking "this nonce" and "the one before this one".

For example:

$t = strtotime('2006-04-24 12:00:00');
$i = ceil($t / 43200);
echo "$i\n";
$t = strtotime('2006-04-25 00:00:00');
$i = ceil($t / 43200);
echo "$i\n";

Output:

26526
26527

We can (should) make 43200 into a constant and then you would be able to change the window as preferred.

#22 @markjaquith
18 years ago

If you can only control the 43200 number, you're stuck with accepting a range of X to 2X. How about making the code so that if can decrement $i a specified number of times, as well as being able to set the 43200 number? That way, you could decrease the denominator, but increase the number of decrementats, so you could set up a nonce that expires in 30 to 31 minutes (denominator of 60, 30 decrements). Obviously this would make it slower, but it'd be nice to give people the option. I'm fine with the default 12-24 as the standard.

#23 @ryan
18 years ago

The functions are pluggable. Plugins can replace them as they see fit.

@ryan
18 years ago

More noncification.

#24 @ryan
18 years ago

Patch adds nonces for categories and bookmarks.

#25 @SilverPaladin
18 years ago

43200/720 minutes minutes is much better than your accused 30 days.

Oh, you're right. Brainfreeze. I recognized the 720 number and forgot there was an extra /60 to do. 30 days = 720 hours. A 12-24 hour range is MUCH better.

By my calculations, $i would change every 12 hours, not every 30 days.
That makes the window 24 hours

Yes, anywhere from 24 hours down to 12 hours and 1 second. I guess that long of a period is good if this figure can't be customizable. If you get smaller than that, people will expect a more precise logout time. They'll want 10 minutes to mean 10 minutes.

@ryan
18 years ago

Nonce comments, pages, and options.

#26 @ryan
18 years ago

nonce.6.diff adds comments, pages, and options.

TODO: Moderation, files, themes, plugins, users, import.

#27 @ryan
18 years ago

Core nonce functions committed in [3758]. The rest is on the way.

#28 @ryan
18 years ago

[3759] adds nonce checks all over wp-admin.

#29 @ryan
18 years ago

link-import and user-edit need attention.

Let's test this sucka. Turn of JavaScript when testing so that list additions and deletions use the nonced link rather than AJAX.

#30 @denney
18 years ago

It seems adding that patch has some problems with some plugins. I'm using the WPG2 plugin and the following problem occurs (because it calls "check_admin_referer()"):

When I click yes to the confirmation, it just redirects me to a blank page with "wp-admin/admin.php" in the URL. Going back to the WPG2 plugin page indeed shows that nothing was actually saved or done.

I've tried adding a nonce action to the urls using wp_nonce_url() and adding that nonce action to the call to check_admin_referer() but I still get the confirmation page. There is a _wpnonce= in the URL though.

#31 @denney
18 years ago

Disregard my last comment. If I had of read the modified code a little better I would have realised what I was doing wrong.

#32 @ryan
18 years ago

[3760] takes care of user-edit and link-import.

#33 @ryan
18 years ago

[3761] passes the action to the check_admin_referer hook.

@mdawaffe
18 years ago

Better nonces for inlineuploading

#35 @mdawaffe
18 years ago

Currently, file uploading and deleting is not possible with inline uploading. check_admin_referer() is used universally in inline-uploading.php, but the important actions don't send the nonce. Deleting is technically possible with the confirmation, but uploading is impossible since the confirmation does not preserve $_FILES.

2678inline.diff

  1. check_admin_referer() only on actions that need it (delete and save).
  2. Remove some unnecessary wp_nonce_url()s
  3. Add nonces to file deletion and upload.
  4. (Clean up some echos as a side effect of poking around.)

#36 @ryan
18 years ago

[3765]

And [3766] to fix the oops on [3765].

#37 @denney
18 years ago

nonce's need to be added the the "Write Page" and "Write Post" delete links. Just incase you didn't realise.

@mdawaffe
18 years ago

Nonces for post deletion from post.php and category deletion from Manage->Categories

#38 @mdawaffe
18 years ago

Currently, category deletion from Manage->Categories and post deletion from post.php fail the nonce check. Deleting posts is particularly annying since the user is sent through both the JS confirmation and the check_admin_ref confirmation.

2678-posts-cats.diff

  1. Nonces for category deletion from Manage->Categories.
  2. Nonces for post deletion from post.php. Uses JS to update the _wpnonce field if the button is pressed and the JS confirmation dialog is approved. If the user does not have JS capabilities, the nonce will fail and they will have to go through the check_admin_ref confirmation. Either way, the user will see one (and only one) confirmation for post deletion now.

#39 @ryan
18 years ago

[3774] - Fallback to old ref check if action not specified.

[3778] - mdawaffe's cat and post delete fixes.

#40 @ryan
18 years ago

  • Milestone set to 2.0.3
  • Version changed from 2.1 to 2.0.2

Let's call this one done.

#41 @ryan
18 years ago

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

#42 @(none)
17 years ago

  • Milestone 2.0.3 deleted

Milestone 2.0.3 deleted

This ticket was mentioned in Slack in #core by garyj. View the logs.


9 years ago

Note: See TracTickets for help on using tickets.