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

[Security] 4.3 Always "Bad credentials." with algorithm "auto" when migrating from argoni2 #32166

Closed
rbaarsma opened this issue Jun 25, 2019 · 13 comments

Comments

@rbaarsma
Copy link
Contributor

Symfony version(s) affected: 4.3.1

Description
Previously encoded passwords with argoni2 algoritm are not verified by Symfony 4.3, although they could be automatically

How to reproduce

  • Install fresh symfony 4.3 with simple user management (ex. FOSUserBundle)
  • Use algoritm argoni2 instead of auto
  • create user with hashed password
  • switch algoritm to auto
  • can't login anymore

Possible Solution
Very similar to 1318d3b

if (0 === strpos($encoded, '$argon2i')) {
  return password_verify($raw, $encoded);
}

Additional context
Related to #31758, but made new issue because it was already closed and target to bcrypt. I'm having the same issue with argoni2

@rbaarsma
Copy link
Contributor Author

@chalasr do you have some time to add your bcrypt fix for argoni2 too? Much appreciated :)

@nicolas-grekas
Copy link
Member

Do you have libsodium? Which version? Which version of php also?

@rbaarsma
Copy link
Contributor Author

PHP 7.2.18

libsodium headers version => 1.0.11
libsodium library version => 1.0.11

Also note that I've tried the suggested solution by directly editing the symfony source code and it works.

@nicolas-grekas
Copy link
Member

Which solution? I don't know about it :)

@rbaarsma
Copy link
Contributor Author

@nicolas-grekas See the bug report under "Possible solution".

// src/Symfony/Component/Security/Core/Encoder/SodiumPasswordEncoder.php line 91
if (0 === strpos($encoded, '$argon2i')) {
  return password_verify($raw, $encoded);
}

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 25, 2019

Oh, sorry. So, libsodium is not able to verify argon2i ?! I wouldn't go with this patch. Instead, I would bump the minimum version of it in the isSupported() method.

@rbaarsma
Copy link
Contributor Author

Yup I can verify that the \sodium_crypto_pwhash_str_verify function is called and returns false, whereas password_verify returns true for the same inputs.

You expect argon2i will be verified correctly in php 7.3 or something? Not sure exactly what you mean to bump the minimum version. Minimum version of what?

@nicolas-grekas
Copy link
Member

libsodium 1.0.11 is quite old, see https://github.com/jedisct1/libsodium/releases
does it work if you upgrade to a higher version of it? (it should)
can you check which is the minimum version that works?

@nicolas-grekas
Copy link
Member

Looks like 1.0.15 could be the minimum.

@nicolas-grekas
Copy link
Member

Can you please confirm that #32169 fixes the issue (1.0.14 required there)

@rbaarsma
Copy link
Contributor Author

I've not updated libsodium yet (can do that later), but at least I've verified that these changes (specifically the isSupported changes) fix my issue.

Also looking at the code, you should consider to undo 1318d3b and similarly let bcrypt go through the NativePasswordEncoder.

@nicolas-grekas
Copy link
Member

I've not updated libsodium yet (can do that later), but at least I've verified that these changes (specifically the isSupported changes) fix my issue.

great thanks

Also looking at the code, you should consider to undo 1318d3b and similarly let bcrypt go through the NativePasswordEncoder.

I don't think so.

@rbaarsma
Copy link
Contributor Author

I've tried to update libsodium, but it's too much work. I'm using docker php:7.2-fpm image, based on debian-stretch, and in the apt package manager is no newer version. It's probably much easier for someone with a setup that already includes a newer version to check your assumption if libsodium 1.0.14 does successfully verify argoni2 encrypted passwords.

I've approved the changes, as it looks like it won't break anything new and definitely fixes the issue for older libsodium versions. Thanks!

@fabpot fabpot closed this as completed Jun 25, 2019
fabpot added a commit that referenced this issue Jun 25, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

[Security/Core] require libsodium >= 1.0.14

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32166
| License       | MIT
| Doc PR        | -

- bump libsodium to >=1.0.14
- Minimum opscost must be 3, as described in https://wiki.php.net/rfc/libsodium and in https://github.com/jedisct1/libsodium/releases/tag/1.0.15
- ParagonIE_Sodium_Compat [explicitly doesn't implement Argon2](https://github.com/paragonie/sodium_compat#features-excluded-from-this-polyfill), so it makes no sense to check for it.

Commits
-------

4fed5d3 [Security/Core] require libsodium >= 1.0.14
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

5 participants