Make WordPress Core

Opened 3 years ago

Closed 2 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 3 years ago.
23480.diff (441 bytes) - added by nacin 3 years ago.
23480-ut.diff (907 bytes) - added by ryan 2 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 @chriscct73 years ago

  • Cc chriscct7@… added


@mordauk3 years ago

@nacin3 years ago

comment:2 @nacin3 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@….

comment:3 follow-up: @mordauk3 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).

comment:4 @dremeda3 years ago

  • Cc dre@… added

comment:5 in reply to: ↑ 3 @nacin3 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.

comment:6 @mordauk3 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.

comment:7 @SergeyBiryukov3 years ago

  • Version changed from 3.5.1 to 2.5

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

comment:8 @sunnyratilal3 years ago

  • Cc ratilal.sunny@… added

comment:9 @aaroncampbell2 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.

comment:10 @nacin2 years ago

  • Keywords needs-unit-tests added

@ryan2 years ago

comment:12 @aaroncampbell2 years ago

  • Keywords needs-unit-tests removed

comment:13 @ryan2 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.