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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hash the autologin token in the database #53

Merged
merged 2 commits into from Sep 12, 2018

Conversation

Projects
None yet
2 participants
@leofeyer
Member

leofeyer commented Sep 7, 2018

This PR implements contao/core#8888 for Contao 4.6. Thanks to our database token provider, the implementation was not too hard. 馃槃

@leofeyer leofeyer added the defect label Sep 7, 2018

@leofeyer leofeyer added this to the 4.6.4 milestone Sep 7, 2018

@leofeyer leofeyer self-assigned this Sep 7, 2018

@leofeyer leofeyer requested a review from ausi Sep 7, 2018

@aschempp

This comment has been minimized.

Show comment
Hide comment
@aschempp

aschempp Sep 10, 2018

Contributor

I'm not sure this is the correct approach. You are hashing the series which is basically the username, but we are not hasing the token (which is kindof the password).

Contributor

aschempp commented Sep 10, 2018

I'm not sure this is the correct approach. You are hashing the series which is basically the username, but we are not hasing the token (which is kindof the password).

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Sep 10, 2018

Member

The series is what is in the cookie, isn't it?

Member

leofeyer commented Sep 10, 2018

The series is what is in the cookie, isn't it?

@aschempp

This comment has been minimized.

Show comment
Hide comment
@aschempp

aschempp Sep 10, 2018

Contributor

Both are in the cookie. Series is staying consistent across logins, token is updated on every successful authentication. This way it can be detected if the token of a series has been used, and therefore a cookie has been used twice.

Contributor

aschempp commented Sep 10, 2018

Both are in the cookie. Series is staying consistent across logins, token is updated on every successful authentication. This way it can be detected if the token of a series has been used, and therefore a cookie has been used twice.

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Sep 10, 2018

Member

You are right, we have to hash both. I will update the PR.

Member

leofeyer commented Sep 10, 2018

You are right, we have to hash both. I will update the PR.

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Sep 10, 2018

Member

However, this does not work, because we need the unhashed token here:

return new PersistentToken($row->class, $row->username, $series, $row->value, new \DateTime($row->lastUsed));

So we also would have to adjust Symfony here.

Member

leofeyer commented Sep 10, 2018

However, this does not work, because we need the unhashed token here:

return new PersistentToken($row->class, $row->username, $series, $row->value, new \DateTime($row->lastUsed));

So we also would have to adjust Symfony here.

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Sep 10, 2018

Member

So in a first step, we could hash the series, which already prevents the attack. Once symfony/symfony#27910 has been merged, we can also hash the value, however, given that this is a BC break, it can only be implemented in Symfony 5.

@contao/developers What do you think?

Member

leofeyer commented Sep 10, 2018

So in a first step, we could hash the series, which already prevents the attack. Once symfony/symfony#27910 has been merged, we can also hash the value, however, given that this is a BC break, it can only be implemented in Symfony 5.

@contao/developers What do you think?

@aschempp

This comment has been minimized.

Show comment
Hide comment
@aschempp

aschempp Sep 11, 2018

Contributor

I think it can be changed in Symfony 4, but I agree we could just hash the series in our implementation to practically prevent the attach.

Contributor

aschempp commented Sep 11, 2018

I think it can be changed in Symfony 4, but I agree we could just hash the series in our implementation to practically prevent the attach.

@leofeyer leofeyer merged commit b459fbc into 4.6 Sep 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@leofeyer leofeyer deleted the hotfix/autologin-hash-46 branch Sep 12, 2018

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