Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in basic auth implementation causes WordFence to flag normal users as brute force attempts #954

Closed
justinmaurerdotdev opened this issue Feb 11, 2019 · 5 comments

Comments

@justinmaurerdotdev
Copy link

I have Event Espresso running on a staging site with HTTP authentication (nginx basic auth) set up to protect it from public viewing. For example purposes, we'll say the username for this authentication is "staging". This staging site also uses WordFence to provide brute force protection from repeated login attempts under the same username. Unfortunately, Event Espresso appears to be breaking this brute force protection.

I believe it is because of some code in this file: https://github.com/eventespresso/event-espresso-core/blob/master/core/third_party_libs/wp-api-basic-auth/basic-auth.php. On lines 45-47, the HTTP credentials are retrieved in place of standard login credentials. Then, on line 87, there is an attempt to authenticate with these credentials using wp_authenticate(). So, with all of these things combined, when using the staging site, any non-logged-in page views are flagged by WordFence as login attempts with the username "staging".

I noticed that this substitution is not done in the original library being used here: https://github.com/WP-API/Basic-Auth/blob/master/basic-auth.php. Is there any reason to try to use HTTP credentials as a substitute for proper WP login credentials?

@joshfeck
Copy link
Contributor

joshfeck commented Feb 12, 2019

I believe so, yes, the comments in Event Espresso's version of basic auth explain:

//account for issue where some servers remove the PHP auth headers
//so instead look for auth info in a custom environment variable set by rewrite rules
//probably in .htaccess
//and, as a last resort, look in the querystring

What we had found is many servers would not allow basic auth to authenticate with the site when using the EE4 mobile apps. So the workarounds that you noticed were added in, starting with some code that was offered in WP-API/Basic-Auth#1 and iterated on later.

What you could do is install the original basic-auth plugin and the function_exists() checks in Event Espresso 4 core will make sure EE's version doesn't load.

@joshfeck joshfeck removed their assignment Feb 12, 2019
@justinmaurerdotdev
Copy link
Author

I understand that this was done for a reason, but I think it was a mistake to close this bug. This issue causes WordPress to attempt to authenticate a server-level user over, and over, and over again on every page load. That's abusive behavior in terms of server resources, and has potential security implications as well, since a user could authenticate simply using HTTP authentication, while bypassing WP's normal cookie + POST data system. What if my HTTP auth credentials happened to be the same as one of my actual WP users? Then we'd be in real trouble.

I'd request that this issue be re-opened and treated as a security complaint.

@joshfeck
Copy link
Contributor

Hi,

There's already discussion/planning elsewhere to:

  1. Disable the Auth mechanism by default
  2. Add an option for enabling it
  3. If possible, limit the auth to only REST requests
  4. Move away from using Basic Auth entirely

So that's why I closed this issue.

There is a developer tasked with the above items. Since you need a fix for this today, you could either:
a) install the original basic-auth plugin
or
b) add this to a site functions plugin:
https://gist.github.com/joshfeck/8b586e670cf120fc40a803daa6d8b205

@mnelson4
Copy link
Contributor

Hey @360Zen, thanks for sharing your use-case and findings.

So as I understand it, the remaining issues are:

  • because EE allows for authentication (within WordPress) via basic auth, it's another attack vector for brute force attacks (one which security plugins are probably not checking for)
  • we are checking the basic auth credentials against WordPress on every request, which is inefficient (although this is just the case for sites setup like yours, which have nginx or apache setup for basic auth)

Regarding it being another attack vector, yes, enabling another way to authenticate (in order to enable using the mobile apps) does add another attack vector. I created #974 in order to somewhat reduce that, but the only real fix to removing that attack vector is to disable basic auth login for WordPress entirely. The following code snippet does that:

add_action('AHEE__EE_System__load_espresso_addons__complete', function(){
    remove_filter( 'determine_current_user', 'json_basic_auth_handler', 20 );
});

My pull request will also reduce that unnecessary checking basic auth's credential's against WordPress on all normal requests. (Although I don't think that server load would have been onerous, but may as well remove it in this case).

So I don't think that code snippet and pull request will entirely answer your concerns, but I hope they help. We've always been intending for our usage of Basic Auth to be a temporary solution until WordPress comes with a built-in way for authentication.

@justinmaurerdotdev
Copy link
Author

Thanks to both of you for the reply. I'm glad to see that it's being addressed and I appreciate the help with a temporary solution. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants