Nonces instead of referers

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

Better (working) patch. I hope.

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.

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?

+1 on the last idea.

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?

-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.

current_nonce_can('delete_post', $post_id);

:-)

@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. ;)

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. :)

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

+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.

A third, better patch, some changes suggested by mdawaffe

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).

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

Bugfix on 3 from random

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.

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".

Make nonce creation/verification pluggable. nonce post editing.

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

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.

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.

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.

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.

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.

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

More noncification.

Patch adds nonces for categories and bookmarks.

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.

Nonce comments, pages, and options.

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

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

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

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

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.

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.

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

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

[3761] passes the action to the check_admin_referer hook.

[3764]

Better nonces for inlineuploading

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.)

[3765]

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

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

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

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.

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

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

Let's call this one done.

Milestone 2.0.3 deleted