Do Not Allow Negative IDs in wp_set_auth_cookie()

[mordauk]

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.

[chriscct7]

+1

[nacin]

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

[mordauk]

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

[nacin]

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.

[mordauk]

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.

[SergeyBiryukov]

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

[aaroncampbell]

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.

[ryan]

​[UT1288]

[ryan]

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