Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#23480 closed defect (bug) (fixed)

Do Not Allow Negative IDs in wp_set_auth_cookie()

Reported by: mordauk Owned by: ryan
Milestone: 3.6 Priority: normal
Severity: major Version: 2.5
Component: Users Keywords: has-patch commit
Focuses: Cc:


After discovering a flaw in a plugin that made it possible for users to bypass the plugin's login form / user validation, I've found an issue with wp_set_auth_cookie() that I believe to be critical.

Since all user validation is (and should be) done prior to calling wp_set_auth_cookie, the function allows any user ID to be passed to it and the cookie will be set.

The user ID passed to wp_set_auth_cookie gets passed to wp_generate_auth_cookie, which then retrieves the user data from the ID with get_userdata, which goes through get_user_by and WP_User::get_data_by. The WP_User::get_data_by method runs absint on the original user ID passed to wp_set_auth_cookie.

absint will translate any negative number to its positive counter part.

The problem with this is that wp_set_auth_cookie will happily generate a valid cookie even when given an invalid user ID, such as -1.

While it is the responsibility of plugins to ensure wp_set_auth_cookie never gets called except when all user data has been fully validated, it seems crazy to me that the function will still successfully generate the auth cookies when given a negative user ID.

In the case of the flaw discovered in the plugin mentioned above, -1 was being used to indicate invalid user data (which seems fine), but then the -1 was getting passed (when it shouldn't have been) to wp_set_auth_cookie, which simply translated the user ID to 1 (almost always the admin account) and logged the invalid user in as an admin. While it is obviously a bug on the plugin's part that -1 ever reached wp_set_auth_cookie, it still should have died gracefully.

I'm proposing we for wp_set_auth_cookie to stop short if the user ID isn't an INT or if it is negative.

Attachments (3)

auth_cookie.patch (547 bytes) - added by mordauk 4 years ago.
23480.diff (441 bytes) - added by nacin 4 years ago.
23480-ut.diff (907 bytes) - added by ryan 3 years ago.

Download all attachments as: .zip

Change History (16)

#1 @chriscct7
4 years ago

  • Cc chriscct7@… added


4 years ago

4 years ago

#2 @nacin
4 years ago

  • Milestone changed from Awaiting Review to 3.6

The issue here is much deeper in the stack. wp_generate_auth_cookie() also assumes that the passed user ID is proper, and has no error conditions if the user doesn't exist. Likewise, wp_set_auth_cookie() is always going to set an auth cookie — there are no error conditions.

wp_generate_auth_cookie(), passed an invalid (positive) ID, would return the cookie's contents but without the first element of the cookie. However, it would start with a | so wp_parse_auth_cookie() would say it is valid and return an empty username. Only in wp_validate_auth_cookie() would the empty username finally be rejected.

The fix to avoid -1 from being considered 1 is 23480.diff. It would be prudent for more error handling to exist here, though.

In the future, any bug report having to do with security should first be reported privately to security@….

#3 follow-up: @mordauk
4 years ago

Thanks Nacin.

Any chance of getting http://core.trac.wordpress.org/attachment/ticket/23480/23480.diff in before 3.6?

I debated whether to report this to security@… or here and opted for here because it's not so much a security flaw as it is a flaw that could result in security flaws unknowingly in plugins (as happened to me).

#4 @dremeda
4 years ago

  • Cc dre@… added

#5 in reply to: ↑ 3 @nacin
4 years ago

Replying to mordauk:

I debated whether to report this to security@… or here and opted for here

When in doubt, opt for private disclosure, or talk to any core developer privately.

#6 @mordauk
4 years ago

Fair enough, will do.

Thoughts on putting http://core.trac.wordpress.org/attachment/ticket/23480/23480.diff in before 3.6? It's such a minor patch but takes care of a pretty significant issue.

#7 @SergeyBiryukov
4 years ago

  • Version changed from 3.5.1 to 2.5

abs( intval( $user_id ) ) was added in [6180]. Changed to absint() in [6682].

#8 @sunnyratilal
4 years ago

  • Cc ratilal.sunny@… added

#9 @aaroncampbell
3 years ago

  • Keywords commit added

Looks like 23480.diff is a nice simple patch that makes a big step forward here. I think we should push it in. I had rboren look and he had no objections either.

#10 @nacin
3 years ago

  • Keywords needs-unit-tests added

3 years ago

#12 @aaroncampbell
3 years ago

  • Keywords needs-unit-tests removed

#13 @ryan
3 years ago

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

In 24316:

In WP_User::get_data_by(), don't abs int negative IDs. Instead, return false when an ID less than 1 is passed.

Props nacin, mordauk
fixes #23480

Note: See TracTickets for help on using tickets.