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

Allow creating a Member record with no password #10940

Closed
4 tasks done
lekoala opened this issue Sep 8, 2023 · 3 comments
Closed
4 tasks done

Allow creating a Member record with no password #10940

lekoala opened this issue Sep 8, 2023 · 3 comments

Comments

@lekoala
Copy link
Contributor

lekoala commented Sep 8, 2023

Affected Version

4.13.14, 5.0.13

Description

This update https://www.silverstripe.org/download/security-releases/cve-2023-32302/ recently made impossible to create members in the cms without a password. Furthermore, a validation has been added so that the password field cannot be empty 7b21b38.

In some of my projects, members are created from the admin, even though they are not meant to be able to log in, or need to create their password themselves through "i lost my password" (for example, for websites without a public sign up form, where members are created from the admin, but the website owner cannot know their password).

To put it in simple words, requiring a password to avoid issues with custom login form that could "potentially" allow blank passwords seem like a really odd solution, since it affects end user admin experience and even lead to ANOTHER security risk (an admin knowing a user's password).

Steps to Reproduce

  • Try to create a member from the cms without the ability to login
  • You get a validation error

Proposal

Instead of forcing a password, we could generate a dummy random password for all members. Effectively, this means they cannot login and that the (really low) security risk is avoided. It also doesn't put the burden on the admin to manually set a password.
Or maybe even enforce at core level that it's not possible to login with a blank password ? that would be even better.

Acceptance criteria

  • When a member is created in the CMS without a password, the CMS automatically assigned a randomly generated password to the user.
  • The CMS user creating the new user is not inform of the randomly generated password.
  • The password generation logic uses a suitably secure algorithm to generate the password.
  • Developers retained the option to programmatically create passwordless users.

PRs

@GuySartorelli GuySartorelli added this to the Silverstripe CMS 5.2 milestone Sep 11, 2023
@GuySartorelli GuySartorelli changed the title Revisiting CVE-2023-32302 Allow creating a Member record with no password Sep 11, 2023
@GuySartorelli
Copy link
Member

GuySartorelli commented Sep 11, 2023

Thanks for bringing this to our attention and suggesting some alternatives. When we were looking at the security vulnerability we hadn't considered the valid use cases for creating a passwordless user that you have presented.

I'm marking this as an enhancement to consider for CMS 5.2 - though if someone raises a pull request for 4.13 that has minimal chance of regression, good unit and/or behat test coverage, and a clear indication of why they need it for 4.13, we can consider introducing it as a patch since the behaviour did change in 4.13 in a patch.

Regarding the proposed solution:

Instead of forcing a password, we could generate a dummy random password for all members. Effectively, this means they cannot login and that the (really low) security risk is avoided. It also doesn't put the burden on the admin to manually set a password.

I like this solution. It would of course need to apply only when a password hasn't been set, and only for members created through the CMS (so-as to not affect unit tests).

Or maybe even enforce at core level that it's not possible to login with a blank password ? that would be even better.

This can't be done, as authentication includes scenarios such as single-sign-on and authenticating a given request using session cookies. The low-level "log in" doesn't care how you're authenticating, so it doesn't know whether you're using a form or one of those scenarios, and in those scenarios we don't want to be throwing passwords back and forth.

@lekoala
Copy link
Contributor Author

lekoala commented Sep 11, 2023

@GuySartorelli well i'm still not sure that the fix provided really solves anything. if a custom authentication method implements wrongly checkPassword and allow blank passwords, there is not much you can do about it. It could also allow wrong passwords. It could do all sort of stupid things :-)

If anyone has the same need as me, a simple member extension with this

public function updateMemberPasswordField($password)
{
    $password->setCanBeEmpty(true);
}

works just fine as well in the meantime

@GuySartorelli GuySartorelli removed their assignment Oct 19, 2023
@GuySartorelli
Copy link
Member

PRs merged. This feature will be available in Silverstripe CMS 5.2

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

No branches or pull requests

3 participants