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

[4.0] Deleting SHA256 password support #23872

Open
wants to merge 5 commits into
base: 4.0-dev
from

Conversation

6 participants
@Hackwar
Copy link
Member

commented Feb 10, 2019

I'm proposing to delete SHA256 password support here and now in 4.0. Yes, it has been marked for removal in 5.0, but I think it needs to go now.

SHA256 support was introduced in Joomla 3.2 and was part of probably the worst release that Joomla ever had. 3.2 claimed to introduce bcrypt hashing for passwords, but actually failed spectacularly. Accounts that were newly created with a 3.2.0 release had unreadable password hashes and thus were pretty much DOA, accounts that changed their password on systems that ran with PHP <5.5 silently used SHA256 to hash the passwords, instead of using a proper fallback. So the people who used the SHA256 algorithm were people that had an account on a website that ran on PHP 5.3 and changed their password in the slim timeframe that 3.2.0 was released and 3.2.1 wasn't released amd which since then have never again logged in. Because in 3.2.1 we introduced a feature that rehashed a password if it wasn't hashed with bcrypt.

3.2.0 has been released about 5 years ago. If a website hasn't updated from 3.2.0 since then, they have quite other problems. If you did update since then and have users who logged in during that very short time and haven't logged in since, it would be very safe to assume that they are not active users. Thus this code should be removed now.

Hackwar added some commits Feb 10, 2019

@mbabker

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Personally, I'd say either drop all the deprecated hashes with 4.0 or keep them all until 5.0. IMO there is no point in dropping one and keeping the rest (especially as those are even older than the SHA256 mechanism). Lucky enough, if the DI system and the UserHelper class are set up correctly, it should not be difficult to write a plugin that restores these deprecated algorithms if support is really needed (at least, that was the intent of moving things to use the DI container).

@Hackwar

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

That would mean dropping PHPass and MD5. I have no idea how widely used PHPass is, but as far as I can see, PHPass could still be actively used on 3.x sites on old hosts, right? Regarding MD5: Yes, it is not safe to use for normal usage, but I really like that I can do UPDATE #__users SET password = MD5('password') WHERE id = 42; to reset my password via the DB. I don't know if that is (reliably) possible for bcrypt. The password is rehashed as bcrypt as soon as you log in the next time.

@mbabker

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

I have no idea how widely used PHPass is, but as far as I can see, PHPass could still be actively used on 3.x sites on old hosts, right?

PHPass, like MD5 and SHA256, are only used for validating hashes of accounts that have not been logged into since the release of Joomla 3.3, five years ago (because the PHP minimum is high enough that BCrypt is reliably available in all environments in the expected format, PHPass was the stop-gap to get from 3.2.1 to 3.3 without screwing up too much more than had already been done).

You don't have the easy-to-use UPDATE users SET password = HASH('thing') query with BCrypt, IMO that's an acceptable tradeoff to removing an insecure algorithm (you change that to a query you copy from somewhere that already has "admin" or "password" or whatever already hashed).

@mbabker

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Err, crap. Just remembered PHPass is also the algorithm used toward the end of 2.5 (upgraded from MD5). So I guess realistically if you've got sites still migrating from 2.5 they'd be on either MD5 or PHPass. *sigh*

@Hackwar

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2019

So then this PR would indeed be the way to go for now. 😉

@wilsonge

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

#12333

We went down this path a whilst back. We need to be careful. I'm not sure if there's an easy way we can add a check into Joomla Update for this (i.e. warn people before they upgrade) or force password resets but I'm in favour of killing off sha256 right now

@HLeithner

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

The only thing to consider is that importing users from other system will not work any longer.

For example I migrated a typo3 installation with 1000+ users without any password rehashing, that was a nice experience. Auto rehash has done the rest. Maybe we can find a way to support old passwords with a plugin?

@Hackwar

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@wilsonge What do you want to do? As I said, a user who still has a SHA256 password, hasn't logged in for 5 years. All others would already be converted. I don't see this being a bigger issue than any other B/C break that we have. Document it and that's it.

@HLeithner I understand that, but you could create your own authentication plugin. In this case I strongly want to push to remove this, because SHA256 does not seem to be secure AND we are not using it in any way in our core. Keeping it in core to me sounds like an encouragement to use it and that is something that we definitely shouldn't do. Thus if you want to do a plugin to support this, I see that plugin in the 3PD domain. Removing this algorithm to replace it with a complete plugin in core sounds like a very, very bad idea.

@HLeithner

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

I don't mean a core plugin, I only wanted to be sure that its possible to create a plugin that can add a easyway to migrate existing accounts. I'm all for it to remove all legacy algos that are no longer consider to be secure as michael suggested.

@mbabker

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

In the current state of affairs, UserHelper::verifyPassword() can easily support password handlers added by plugins. Same with UserHelper::hashPassword(). Where you run into trouble right now is the password.handler.chained service is protected so a plugin can't decorate/replace that service in any way (#16953 for that issue) but can at least add a handler via Factory::getContainer()->get('password.handler.chained')->addHandler($handler);.

What's tricky though is UserHelper is all static. While the uses of UserHelper::hashPassword() to generate hashed tokens in the database (i.e. for the privacy requests or password reset system) can more easily be locked to a single hash, the calls in the User class do need some way to be able to specify the hash otherwise you're ending up with multiple hashing and saving operations to get a password with the right hash into the database (assuming the cleartext password is included in plugin events anyway, otherwise you're kind of SOL).

@mbabker

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

Oh, the other issue with UserHelper::hashPassword() is that the list of supported algorithms is hardcoded in the class if you don't pass it a container service ID as the algorithm to use. And that creates potential issues as algorithms come and go.

@HLeithner

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

So adding a hash for checking is easy and J! rehashes automatically if its not our default. That would be enough in my opinion.

Merge branch '4.0-dev' of https://github.com/joomla/joomla-cms into p…
…atch-38

# Conflicts:
#	libraries/src/Authentication/Password/SHA256Handler.php
@Hackwar

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

So to recap: This is got the thumbs up of all participants in here so far. Waiting for tests.

@franz-wohlkoenig franz-wohlkoenig added this to In progress in [4.0] PHP via automation Apr 4, 2019

@Hackwar

This comment has been minimized.

Copy link
Member Author

commented May 26, 2019

Can we merge this now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.