Critical vulnerability in Multisite installations

The value the plugin stores (the site ID) will be predictable, so a cookie probably wouldn’t be a good method, since it could be forged; so I was thinking about storing it in a usermeta field instead. Then check it during an authenticate callback, so that you could easily prevent the user from logging in if the check fails.

Now that I think about it a bit more, I don’t think we even need to store the site id; we just need the usermeta field to exist. It’s existence represents the fact that the user logged in with a 2FA token, so they should be allowed to access any sites that have 2FA enabled.

It does need to be cleared out whenever a user logs out manually or their auth cookie expires, though; otherwise the valid user would login the first time and it would be set, and then any time after that an attacker could login to a site without GA and still be able to access sites that do have GA.

So maybe it should be a transient with the user’s ID and the same expiration as the auth cookie, rather than a usermeta field.

Some scenarios:

Normal use case for valid user:

1. User isn’t logged in
2. User successfully logs into site with GA active
3. GA creates the transient
4. User visits admin page on different site that has GA active
5. GA checks transient. It exists, so the user is allowed to continue.
6. User visits admin page on different site that doesn’t have GA active
7. GA doesn’t (can’t) do anything, they’re authenticated by their auth cookie

Attacker tries to bypass 2FA by logging into site where GA isn’t activated:

1. Attacker isn’t logged in.
2. Attacker logs into site without GA active
3. The transient doesn’t get set, because GA isn’t running
4. Attacker visits admin page on different site with GA active
5. GA checks the transient. It doesn’t exist, so the callback returns false and the user is auth_redirect()'d to the login screen.

Here’s one scenario where it would break down: An attacker tries to bypass 2FA while valid user is logged in:

1. Neither are logged in.
2. Valid user logs in to site with GA active, transient is set.
3. Attacker logs into site without GA, they get auth cookie
4. Attacker visits admin page on site with GA active, they’re allowed because the transient exists

There’s probably a good way to fix that, though. Maybe generate a nonce and store it in a cookie that is set to expire when the auth cookie expires. At the same time, store that value in a usermeta field. When they try to visit an admin page, check if they have the cookie and if the value matches the one in their usermeta. That way GA could confirm that the user requesting the page is the same one that originally logged in with the token, and it would automatically expire at the right time.

There are probably some other edge cases that need to be considered, but I think that’s a good starting point.

(That got a bit lengthy and was kind of me thinking outloud. So, to summarize, I think the nonce-cookie-plus-usermeta method at the end would probably work best, rather than only a usermeta field, or only a transient.)

If you place the code inside an authenticate callback, then you can just return false if it fails, and they’d be logged out and redirected to the login page, right? That’d also cover POSTs to admin-ajax.php, which should cover any front-end things like comments (or p2, Front End Editor, etc).

I’m pretty sure authenticate runs during every request to wp-admin. The reason people don’t see the login screen each time is because they authenticate via the auth cookie instead of their username/password, but both of those functions are just callbacks to authenticate.

Doh, you’re right, i was remembering it wrong.

Here’s a breakdown of what happens when a user is already logged in and they request an admin page:

1. User visits a page in /wp-admin/
2. That page includes admin.php
3. admin.php calls auth_redirect()
4. auth_redirect() calls wp_validate_auth_cookie()

This is what happens when a user logs in for the first time:

1. wp-login.php calls wp_signon()
2. wp_signon() calls wp_authenticate()
3. wp_authenticate() runs $user through the authenticate filter
4. wp_authenticate_username_password() and wp_authenticate_cookie() are callbacks for theauthenticate filter.
5. wp_authenticate_cookie() calls wp_validate_auth_cookie()

AJAX request:

1. admin-ajax.php calls is_user_logged_in()
2. is_user_logged_in() calls wp_get_current_user()
3. wp_get_current_user() calls get_currentuserinfo()
4. get_currentuserinfo() calls wp_validate_auth_cookie()

XMLRPC request:

1. xmlrpc.php calls wp_xmlrpc_server::serve_request()
2. serve_request() calls IXR_Server::IXR_Server()
3. IXR_Server() calls IXR_Server::serve()
4. serve() calls IXR_Server::call() with the requested method
5. call() calls the requested method
6. If the requested method requires authentication (e.g., wp_deletePost), then it calls wp_xmlrpc_server::login()
7. login() calls wp_authenticate(), and then that follows the same steps as when a user logs in through their browser

The new JSON API also calls wp_authenticate() in conjunction with basic HTTP auth, just like XMLRPC. I think it’ll eventually support oAuth too.

So, you’re right, hooking into authenticate wouldn’t work. wp_validate_auth_cookie() doesn’t have a filter we could use to make it return false, so I think hooking into the auth_cookie_valid action and then doing a wp_safe_redirect() might be the best option.

For posting comments, it works similar to AJAX requests. The comment form POSTs to wp-comments-post.php, which calls wp_get_current_user(), and that eventually calls wp_validate_auth_cookie().

I think a lot of other stuff on the front end calls is_user_logged_in(), which calls wp_get_current_user() too. Pretty much anything that requires authentication, even on the front end, would need to pass through wp_get_current_user() and therefore wp_validate_auth_cookie().

It definitely needs to be tested thoroughly and audited, though. When I released the Per User Prompt extension, I offered a security reward if anyone could find a bug in it, and that worked pretty well. It’s not enough money to interest a professional firm, but for friends/colleagues/hobbyists who are interested in security, it can sometimes be enough to motivate their curiosity 🙂 I’d be happy to pitch in for something like that if you want to offer one.

Just requiring network activation would definitely be easier and more full-proof, though. I think some users might want the ability to have it enabled on some sites and disabled on others, but probably a small percentage.

At the very least, that may be a good temporary solution until a more robust one is developed. It’ll be interesting to see how Duo Security and the other 2FA plugins handle it (especially the ones that have paid developers working on it full-time).

These may become relevant:

http://journal.ryanmccue.info/311/custom-auth-tokens/
https://core.trac.wordpress.org/ticket/26706

It sounds like that would allow us to just return false for $user instead of having to redirect, which would be a cleaner approach.

But I agree, I don’t think there’s a compelling reason against simply having the plugin require network-wide activation, at least until there’s time to fully vet and test a more robust fix.