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

FIX Generate salt if needed #11158

Merged
merged 2 commits into from
Mar 4, 2024
Merged

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented Feb 26, 2024

Description

Some Members may not have a Salt defined. Yet, it's possible to call encryptWithUserSettings nonetheless and the framework will assume a Salt exist. This is not correct, since it will fail to encrypt things properly.
I don't see any reason to assume anything and not generate a Salt if needed.

Alternative option if we don't want to generate a salt: at least throw a proper exception explaining that you cannot encrypt with an empty salt instead of assuming there is a valid value there.

Manual testing steps

  • On a Member without a password, but with a PasswordEncryption
  • Call $member->encryptWithUserSettings('somestring')
  • You get a blowfish exception because it could not encrypt with an empty salt

Issues

#11159

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@GuySartorelli
Copy link
Member

Please create an issue for this and link to it (I've added that section back to the PR template for you), so we can track this. We track issues, not PRs.

If this is indeed a bug and affects CMS 4, please target the 4.13 branch so it can be fixed there as well.

@GuySartorelli
Copy link
Member

In your reproduction steps you have noted "On a Member without a password" - is that the only scenario where a member doesn't have a salt?
If there are scenarios where a member who does have a password doesn't have a salt, will this change stop them from logging in until they change their password?

@lekoala
Copy link
Contributor Author

lekoala commented Feb 29, 2024

i added the issue

#11159

i didn't dive further than my use case, but from the code, it's clear enough:

  • either a proper exception is thrown (instead of waiting bcrypt to fail because of the missing salt)
  • a salt is generated

i think both options make sense, but i'd rather generate a salt unless there is a good reason not to do it there

@GuySartorelli
Copy link
Member

I've looked a little deeper into this.

If a user does not have a salt, one is generated for them in this flow when saving the record:

  1. call $member->write()
  2. in $member->onBeforeWrite() if the password has changed or this is a new record, call $member->encryptPassword()
  3. Member's salt is cleared, and a new one is created via Security::encrypt_password()
  4. New salt is set to $member->Salt

Because of this flow, a member with no salt should only occur if that member has been created programatically and has not yet been saved to the database.

What is your use case where the above flow doesn't occur? Since there's a write() in the implemented solution it seems like you're expecting the member to be in the database, so it might just be that you need to call write() earlier than you currently are? It feels (based on the above flow) that this PR is more of a workaround than a fix but that's without knowing what you're specifically trying to achieve.

@lekoala
Copy link
Contributor Author

lekoala commented Mar 4, 2024

@GuySartorelli well, the issue was with my admin user in dev mode, so while I fully agree that this should not happen, it technically can. I don't see the advantage of "assuming" there is a salt: either an exception is thrown (there is no salt, and it should not happen) or a salt is generated

@GuySartorelli
Copy link
Member

By admin user do you mean the default admin account?
I'll see if I can figure out what's causing that - there might be an underlying problem to solve there.

As far as the change this PR is making, it seems like if anything should happen it should throw an exception - having a write there seems unexpected, and setting a salt seems like it's just hiding an underlying problem.

@lekoala
Copy link
Contributor Author

lekoala commented Mar 4, 2024

agreed, i updated the PR. Actually by reading the comment above, it seems the original intent is to check that both the PasswordEncryption AND the Salt are available, so that's what I did

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I can't find what might have caused the salt to be missing on default admin... unless I guess it's on a fairly old project or old DB?
In either case this seems like what the condition should have been in the first place.

Thanks for making this change and for your patience.

@GuySartorelli GuySartorelli merged commit dba8734 into silverstripe:5.1 Mar 4, 2024
15 checks passed
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

Successfully merging this pull request may close these issues.

2 participants