#2678 closed defect (bug) (fixed)
Nonces instead of referers
Reported by: |
|
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)
Change History (51)
#2
@
19 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
@
19 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?
#5
@
19 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
@
19 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.
#8
@
19 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
@
19 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. :)
#11
@
19 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.
#12
@
19 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
@
19 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
#14
@
19 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
@
19 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".
#18
@
19 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
@
19 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
@
19 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
@
19 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
@
19 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.
#25
@
19 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.
#26
@
19 years ago
nonce.6.diff adds comments, pages, and options.
TODO: Moderation, files, themes, plugins, users, import.
#29
@
19 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
@
19 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
@
19 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.
#35
@
19 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
- check_admin_referer() only on actions that need it (delete and save).
- Remove some unnecessary wp_nonce_url()s
- Add nonces to file deletion and upload.
- (Clean up some echos as a side effect of poking around.)
#37
@
19 years ago
nonce's need to be added the the "Write Page" and "Write Post" delete links. Just incase you didn't realise.
#38
@
19 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
- Nonces for category deletion from Manage->Categories.
- 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.
#40
@
18 years ago
- Milestone set to 2.0.3
- Version changed from 2.1 to 2.0.2
Let's call this one done.
I should clarify that not every admin form has had the nonces added, just the ones mentioned in the description above.