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

[Security] validate empty passwords again #23507

Merged
merged 1 commit into from Jul 17, 2017

Conversation

Projects
None yet
7 participants
@xabbuh
Member

xabbuh commented Jul 14, 2017

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23341 (comment)
License MIT
Doc PR

It looks like this part of #23341 causes serious security issues for some users who rely on the validator to also compare the empty string with their user's password (see for example #23341 (comment)). Thus I suggest to revert this part of #23341.

@szymach

This comment has been minimized.

Show comment
Hide comment
@szymach

szymach Jul 14, 2017

But this can cause a warning in sf2.8+ and break your app if you provide an empty string. If this needs to work as previously, why not simply change this to:

if (null === $password || '' === $password) {
    // add violation that the password cannot be empty
    return;
}

That way there is no exception and no security issue.

szymach commented Jul 14, 2017

But this can cause a warning in sf2.8+ and break your app if you provide an empty string. If this needs to work as previously, why not simply change this to:

if (null === $password || '' === $password) {
    // add violation that the password cannot be empty
    return;
}

That way there is no exception and no security issue.

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Jul 15, 2017

Contributor

That way there is no exception and no security issue.

WAT 😐 this means that when you provide an empty string it's considered valid, which is not the case at all. When you use this constraint you want to check if the provided password matches the current one. Leaving this empty means the user is bypassing security!

If you want to make this optional use validation-groups.

Contributor

sstok commented Jul 15, 2017

That way there is no exception and no security issue.

WAT 😐 this means that when you provide an empty string it's considered valid, which is not the case at all. When you use this constraint you want to check if the provided password matches the current one. Leaving this empty means the user is bypassing security!

If you want to make this optional use validation-groups.

@sstok

sstok approved these changes Jul 15, 2017

@szymach

This comment has been minimized.

Show comment
Hide comment
@szymach

szymach Jul 15, 2017

@sstok I am not sure we both understand each other, but my proposition is to automatically put a constraint violation if the password is empty. In newer implementations of the password checker, a null value raises a warning and thus breaks the app, hence why ignoring empty passwords was done in the first place.

EDIT To clarify - initially the PR simply removed the if statement, so it would not work properly still. It has been changed since then and all is OK now.

szymach commented Jul 15, 2017

@sstok I am not sure we both understand each other, but my proposition is to automatically put a constraint violation if the password is empty. In newer implementations of the password checker, a null value raises a warning and thus breaks the app, hence why ignoring empty passwords was done in the first place.

EDIT To clarify - initially the PR simply removed the if statement, so it would not work properly still. It has been changed since then and all is OK now.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Jul 17, 2017

@fabpot

fabpot approved these changes Jul 17, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jul 17, 2017

Member

Thank you @xabbuh.

Member

fabpot commented Jul 17, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit 878198c into symfony:2.7 Jul 17, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jul 17, 2017

security #23507 [Security] validate empty passwords again (xabbuh)
This PR was merged into the 2.7 branch.

Discussion
----------

[Security] validate empty passwords again

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23341 (comment)
| License       | MIT
| Doc PR        |

It looks like this part of #23341 causes serious security issues for some users who rely on the validator to also compare the empty string with their user's password (see for example #23341 (comment)). Thus I suggest to revert this part of #23341.

Commits
-------

878198c [Security] validate empty passwords again

@xabbuh xabbuh deleted the xabbuh:pr-23341 branch Jul 17, 2017

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Jul 17, 2017

Member

In theory, wasn't an empty password '' potentially valid before? Now it's never valid. Looks like a BC break to me.

Member

Tobion commented Jul 17, 2017

In theory, wasn't an empty password '' potentially valid before? Now it's never valid. Looks like a BC break to me.

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Jul 18, 2017

Contributor

@Tobion I agree this is a BC break, but for security reasons this is more logical.

Contributor

sstok commented Jul 18, 2017

@Tobion I agree this is a BC break, but for security reasons this is more logical.

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