WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#21111 closed enhancement (fixed)

Make nonce unique for users AND non-users

Reported by: sc0ttkclark Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4
Component: Security Keywords: has-patch
Focuses: Cc:

Description

Currently, wpnonce works from the nonce tick + action + current user ID. That means, nonce is only guaranteed unique for the current user and all other non-users can potentially share the same nonce.

As a solution to this problem, I'm requesting we add additional unique-ness for non-users. See below for my suggestion, it would go directly below the $uid variable set, within wp_create_nonce and wp_verify_nonce.

    if ( empty( $uid ) )
        $uid = uniqid( 'wpnonce_', true );

The use case for this addition, is for usage within a theme for public forms and other actions that do not require a logged in user.

Attachments (3)

21111.diff (617 bytes) - added by sc0ttkclark 9 years ago.
21111.2.diff (635 bytes) - added by sc0ttkclark 9 years ago.
Duh, add $action to filter too
21111-ip-useragent.diff (651 bytes) - added by sc0ttkclark 9 years ago.
Here's another potential fix, which doesn't use a filter, and instead works off of REMOTE_ADDR + HTTP_USER_AGENT

Download all attachments as: .zip

Change History (13)

#1 @scribu
9 years ago

Have you actually tested this? Wouldn't you end up with a different value when checking from the value initially generated?

#2 @sc0ttkclark
9 years ago

Right, hold tight, working on a patch that addresses that.

#3 @nacin
9 years ago

I don't see how this would work. uniqid() uses the current time in milliseconds, which means that a nonce generated to be given to the user, and a nonce generated to be compared to the nonce provided by the user, will never match.

@sc0ttkclark
9 years ago

#4 @sc0ttkclark
9 years ago

  • Keywords has-patch added

No way for me to modify the original description now, but attached a solution in the form of running a filter if $uid is empty. Please excuse my original solution example, I'm working on a low amount of sleep at the moment trying to get a project finished.

With the patch, plugin authors are free to interact with sessions/cookies to create unique IDs for non-users and continue using wp nonce functions instead of having to roll their own, which means they either have to use their own plus wp nonce functions for logged in users, or their own entirely. This solves that issue.

#5 @nacin
9 years ago

Makes sense; the $action should probably also be passed.

@sc0ttkclark
9 years ago

Duh, add $action to filter too

@sc0ttkclark
9 years ago

Here's another potential fix, which doesn't use a filter, and instead works off of REMOTE_ADDR + HTTP_USER_AGENT

#6 @toscho
9 years ago

  • Cc info@… added

$_SERVER[ 'REMOTE_ADDR' ] can change on every request, it is not reliable. And the UA string can contain malicious code, it should be escaped.

#7 @sc0ttkclark
9 years ago

@toscho That extra patch was an idea, the current one on the table to be referenced should be 21111.2.diff at http://core.trac.wordpress.org/attachment/ticket/21111/21111.2.diff

#8 @sc0ttkclark
9 years ago

  • Keywords dev-feedback added

#9 @nacin
9 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 3.5

Patch looks good. Filter could probably use a better name, that's it.

#10 @nacin
9 years ago

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

In [21837]:

Add nonce_user_logged_out filters to wp_create_nonce() and wp_verify_nonce() for when there is no user ID. Provides plugins the ability to tie a nonce to some other characteristic of the session. props sc0ttkclark, fixes #21111.

Note: See TracTickets for help on using tickets.