michaelryanmcneill / shibboleth

Shibboleth plugin for WordPress
https://wordpress.org/plugins/shibboleth/
21 stars 12 forks source link

New "Automatically Create Accounts" option creates a security issue #22

Closed Androclese closed 6 years ago

Androclese commented 6 years ago

Under the new 2.0 version of the plugin, the account creation behavior has changed and it has created a security issue.

In the previous version, accounts would be automatically created if the credentials logging into the site were mapped to a role. If they were not mapped to a role, even if their credentials were accepted by the SSO authority, the account would not be created and they would be denied access to the site.

Under the current version, accounts are not created by default, even if they map to a role, unless the option is enabled; something we have to go and retroactively enable across about 100 websites. Further, once enabled, all accounts will be created, even if they do not map to a role, and it automatically maps to subscriber. This is an issue because on all our sites, we manage the roles externally in LDAP, and granting subscriber access by default gives everybody who attempts a login, access to the site. This is a big security flaw for those who do not want to grant arbitrary access to their site.

This fix is that we either have to create a custom role called “none”, push that out to all the sites, and then configure all the sites to default to that role within your plugin. (a big huge mess / hack)

…or, instead of a binary choice on that option, you can make it a drop-down choice.

– Create Account if mapped to Role, Deny Access if no mapped Role (previous behavior) – Create Account if mapped to Role, Assign to default Role if no mapped Role (current behavior option) – Do not create accounts (current behavior option)

Androclese commented 6 years ago

One additional comment, and another scenario that this unintended security issue this creates.

If a user with valid SSO credentials was assigned a valid Role for the site and then they were removed from that Role, under the old version, they lost their access to the site, even though their account remained in the WordPress database for that site. This kept their content postings active, correctly attributed, and frozen so they could not be changed from that point forward.

Under the new version, their access would be dropped to Subscriber, but they could still have access to their old postings, potentially giving them the ability to alter them should there be an issue of spite for being removed (demoted) from the site.

michaelryanmcneill commented 6 years ago

I agree that this is a regression, and I'll work to get it fixed as quickly as possible. Thanks for reporting and providing the extensive details.

michaelryanmcneill commented 6 years ago

@Androclese, I believe that I've addressed the issue. I've released version 2.0.1 to the WordPress.org repo. Can you please install it and verify it resolves your issue by checking the "Automatically create new users if they do not exist in the WordPress database" checkbox and setting the default role to "(none)"? Once you're ready to push this to all your sites, you can mass update the option by running the following WP-CLI command:

wp option update 'shibboleth_default_role' ''

You could also do direct SQL updates, but in general, directly messing with the database is not ideal.

Thanks again for reporting this.

jrchamp commented 6 years ago

@michaelryanmcneill I tried this out and it looks like setting the value to blank causes all Shibboleth accounts to stop working unless you are using an attribute mapping (see shibboleth_authenticate_user() and its use of shibboleth_get_user_role()). Instead, it looks like I can set the default role to something fake like __no_role__ and access is restored.

michaelryanmcneill commented 6 years ago

Weird...I'm not experiencing that behavior @jrchamp. When you set it to blank, can you let me know what the value of shibboleth_create_accounts is in the options table?

jrchamp commented 6 years ago

shibboleth_default_role: <blank> shibboleth_create_accounts: 1

shibboleth_get_user_role() returns none for the role when shibboleth_create_accounts is empty, which interestingly would map to a real role if there was a role with that identifier.

michaelryanmcneill commented 6 years ago

Strange...I'm going to try and reproduce this. I'll likely move to your recommended approach of __no_role__ and just add some activation logic to transition the <blank> option to __no_role__ in the DB.

michaelryanmcneill commented 6 years ago

@jrchamp, can you open a new issue on this for me describing the behavior? I was going to reopen this one, but I think it would be better to have a separate issue covering the regression.