search

joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.77k stars 3.65k forks source link

"Must match character set" message when logging in with 2FA enabled #12458

Closed mbabker closed 8 years ago

mbabker commented 8 years ago

Steps to reproduce the issue

Log into a site after updating to 3.6.3.

Expected result

Successful login

Actual result

Error page with "Must match character set"

System information (as much as possible)

Hit this on www.joomla.org, downloads.joomla.org, developer.joomla.org, and my own website (Rochen & SiteGround hosting platforms, PHP 5.6 and 7.0).

Additional comments

On each of the four sites listed above, my accounts have 2FA enabled. I tested on another site where I do not have this feature enabled and successfully authenticated.

mbabker commented 8 years ago

The "RC testing checklist" should be testing EVERY FREAKING FEATURE. There will never be a viable checklist because the checklist should be "test everything" for brand new installations and upgrades from 2.5 forward.

And we all know pretty well by now that very few people actually test pre-release packages. When I counted during the 3.2 release cycle, our weekly alpha/beta packages were getting 800 downloads per week and the vast majority of these were for the full/new installation packages.

People are testing new installs just fine. People are testing against the sample data. Few if any are testing with real life sites and configurations, and THAT is what has screwed us royally in the past. Remember the 3.3 release that broke pagination because nobody was testing on a dataset that caused it to be used?

mbabker commented 8 years ago

Not to mention there should be an automated testing suite validating as much of the behavior as possible. We've once again abandoned the system testing suite that at one point tested the UI interactions for a large majority of the CMS (granted it was with sample data but it was still a lot more than what's done now). And the automated testing team are too busy playing with new platforms, trying to reach 100% test coverage on weblinks (really, who cares!?) or trying to convince folks to just drop the tests we have today and start everything from scratch. The most productive thing they've done this year came from the GSoC project starting tests for the JavaScript API.

brianteeman commented 8 years ago

(can we try to keep the rants on topic please)

On 19 October 2016 at 13:47, Michael Babker notifications@github.com wrote:

Not to mention there should be an automated testing suite validating as much of the behavior as possible. We've once again abandoned the system testing suite that at one point tested the UI interactions for a large majority of the CMS (granted it was with sample data but it was still a lot more than what's done now). And the automated testing team are too busy playing with new platforms, trying to reach 100% test coverage on weblinks (really, who cares!?) or trying to convince folks to just drop the tests we have today and start everything from scratch. The most productive thing they've done this year came from the GSoC project starting tests for the JavaScript API.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/joomla/joomla-cms/issues/12458#issuecomment-254802003, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPH8XNipmGkWNLKx7jeMF2X5spjrmz-ks5q1hFlgaJpZM4KaCxb .

Brian Teeman Co-founder Joomla! and OpenSourceMatters Inc. https://brian.teeman.net/ http://brian.teeman.net/

nikosdion commented 8 years ago

@PhilETaylor There's no point making a checklist. I explicitly asked people to test TFA because I knew that if something broke with the PR that would be it.

The problem is that as @mbabker said nobody tested on a real configuration. I can tell you that 3.6.3 works fantastically on my MAMP Pro 4 installation, with YubiKey as the TFA. So did the PR when I tested it in August. I fully understand the difference between local dev and live that's why I asked people to do exactly these tests, considering that I was blindly backporting a feature from a newer FOF version and the unit tests for that seemed to fail on Travis, but not locally, on a feature subset (256-bit key AES CBC) I had never used elsewhere. I mean, FFS, I suspected a problem, I asked people to test specifically for it and here we are now discussing why this specific feature is broken.

FWIW testing TFA should be possible just fine. TOTP (Google Authenticator) can be tested by using a known key and the system time. YubiKey can be tested by implementing a fake validation server which replies true for a known prefix and false for anything else.

PhilETaylor commented 8 years ago

My point is that when announcing a RC Joomla.org is not telling people what to test - saying TEST EVERYTHING is not going to work, if @nikosdion says then 2FA needed testing then that should have been a whole paragraph in announcements like this https://www.joomla.org/announcements/release-news/5662-joomla-3-6-rc-released.html

There was nothing even mentioned about it...

wilsonge commented 8 years ago

That's 3.6.0 RC - there are no official announcements for patch releases and never have been. And when there are over 350 different pr's merged - no we cannot 'just' draw up a feature list. I'd imagine pretty much every single part of Joomla has been touched somewhere.....

3.6.3 works fine @nikosdion on my localhost for new user accounts however accounts set up on 3.6.2 and then upgraded to 3.6.3 do not for google

mbabker commented 8 years ago

We can't tell people explicitly what to test though, and again even when they are what is the number of people who are making these tests against a production configuration?

Take for example a pull request which updates the joomla/registry package; everything in Joomla damn near has a dependency to the Registry class. How do you write instructions that tell people to test every possible interaction with that API?

If we're looking for a master release checklist and a basically confirmed stable "gold version", we will never have a release. As long as nobody tests against production configurations and the teams charged with helping to ensure the code's stability are wasting their time on tasks that do little to support the CMS then we're just going to keep going in massive circles accomplishing nothing. When someone grows the backbone to put the teams to task and get them to produce results in the right places, maybe things will get better.

PhilETaylor commented 8 years ago

I just think when someone says:

I explicitly asked people to test TFA

But yet no one was told to test this, then something, somewhere, the process, has failed...

And this is the result.

mbabker commented 8 years ago

It was stated right in the pull request which should have been fully tested before merging. https://github.com/joomla/joomla-cms/pull/11673

Otherwise what you're expecting is that an announcement goes out stating:

Most of this are things a "normal" end user wouldn't understand or know how to test. So there's no point in creating an announcement listing every single changed functionality and what should be explicitly tested. The people that understand the depth of those changes are mostly in this thread already.

PhilETaylor commented 8 years ago

I have been asked to shut up. So I will, but I see no reason that list could not have been published. Its a lot less than "test everything" :-)

mbabker commented 8 years ago

https://github.com/joomla/joomla-cms/milestone/16?closed=1 lists all 352 items that were marked as closed for the 3.6.3 release. You try telling a user to test each of those 352 items and see what kind of response you get.

DarkCloud14 commented 8 years ago

I apologize in advance as this comment isn't helpful for solving the original issue but maybe you can save some code + possible errors and drop the encryption/decryption of TFA config in future releases as @nikosdion already said/proposed.

Unless there is a good reason to keep TFA config encryption I fully agree with @nikosdion as it's useless to encrypt something someone else could get access to or override if he can run arbitrary PHP or SQL code on the affected site.

mbabker commented 8 years ago

Even if we do that (which I honestly agree we should), we're still left in a position where we have to either be able to decrypt the existing data to store it in plain text or resort back to the earlier suggestion of just wiping it all out and having users set it up again fresh. So it solves potential future issues but doesn't really help bring existing data forward to that state.

DarkCloud14 commented 8 years ago

@mbabker Ok that's correct, I just asked as from nikosdion comment, where he wrote that he proposed that already earlier, it sounded to me like there is/was a good reason why it wasn't done yet (beside the conversion of existing data).

What should a possible solution look like? Should it definitely work with PHP7.1/PHP7.2 and later or just with PHP7 and earlier?! Should a conversion only be done during Joomla upgrade or everytime a user tries to login and some kind of check detects that a conversion is necessary?!

I mean for PHP version < 7.1 a conversion shouldn't be such a huge problem as @nikosdion already proposed a possible solution using FOFEncryptAesMcrypt adapter and use it to do whatever FOFEncryptAes::decryptString does to do the decryption.

You might also try to force FOFEncryptAes (only for the conversion part) to use FOFEncryptAesMcrypt instead of FOFEncryptAesOpenssl (at least as long as this is possible) and save writing the extra decryption code

For PHP 7.1 this might also still work but for PHP7.2+ you would need something else if you want to guarantee the decryption of old data. But maybe you decide to drop that conversion support for incompatible PHP versions and propose in such case a temporary PHP version switch to do the conversion or inform the user about a forced reconfiguration of TFA?!

mbabker commented 8 years ago

So one misconception to clear up.

PHP's ext/mcrypt isn't going away completely. As of 7.1 it does emit deprecation notices and the current plan is to no longer include it in PHP core as of 7.2. However, it will still be installable via PECL as an extension (similar to Memcached or APCu as examples). So it's not like anything encrypted with mcrypt will magically no longer be decryptable.

Similarly, PHP's ext/openssl isn't compiled into the default PHP distribution either. You have to explicitly compile PHP with the right config switches in the ./configure command (and inherently have it installed on your system).

So there really isn't a need to look at this from a PHP version standpoint other than being aware of the deprecation notices that will be emitted in 7.1.

mbabker commented 8 years ago

Just a couple of sources to share:

Bakual commented 8 years ago

To my knowledge it isn't even sure yet it is removed with 7.2. It could as well be removed with 8.0 (which would be following SemVer). That's yet to be decided as far as I know.

mbabker commented 8 years ago

Them moving an extension to PECL is similar to us having moved weblinks out of our core package. It's one thing to move it out of the distro, it's another to remove the code completely (like was done for the old ext/mysql). Funny enough even that's debated on internals whether PHP's release policy allows that.

nikosdion commented 8 years ago

If you wipe everything out you might just as well remove TFA completely. The right way is to decrypt using mcrypt and store unencrypted. This gives people at least one year (realistically much more) to upgrade their TFA configuration before they start using a PHP version without mcrypt built in or an mcrypt version throwing warnings.

slibbe commented 8 years ago

If I may comment on Phil's checklist idea for testing, I would recommend it. Normally when Beta's or RC's come out I test two different set-ups: installing the new Full and upgrading from some recent older version. Usually I test this on MySQL & PostgreSQL which some here will remember ;) I would really appreciate if a number of test scenario's would be described (and possibly one could also share results of these test scenario's). 2FA I can add to my personal checklist, still leaving out lots of other usefull cases. The situation now is that lot's people test a fresh install and most of them have no idea what else to test. Some developers will try their extensions, but that's another story.

rdeutz commented 8 years ago

closing this one because we have a PR #12497

btoplak commented 8 years ago

TL;DR Edit: In the aspect of dropping the encryption of the DB-stored OTP secret keys, the accepted solution isn't good at all. It's not just any "OTP config" in that field, guys, it's OTP secret keys we are talking about. Secret keys have to be encrypted, no exceptions. And the existing encryption schema can and should be improved. All the reasoning below. Or Joomla project could and probably will be ridiculed by the websec and crypto society, again.

I insist that keeping the useless encryption of the TFA config makes no sense. Do you want me to spell it out for you? If I can run arbitrary PHP or SQL code on your site (the only ways to read or affect the config data in the db) I can just CHANGE your password and REMOVE TFA with a simple SQL statement. Encryption makes no sense whatsoever. I have told you so 4 years ago. Will you FINALLY listen to me?

Sorry, but this is definitely a wrong point (of view). According to that "theory" - it is even pointless in having the username/password. If I can run arbitrary code on your site I don't need to even bother to log in and, consequently, don't need your credentials at all !! I can do whatever I want with your files, database or hosting resources... So why are we then bothering the users with credentials anyway? Since given a proper vulnerability - any credentials are useless. Seriously!(?)

As a Joomla! and a web security enthusiast I have to express my honest concerns on this subject and the accepted solution.

Just because the encryption wasn't implemented properly doesn't mean a (proper) encryption isn't needed at all. Quite the contrary, it is all too necessary!

If the encryption wrapper function is all of the sudden broken, it should be fixed. Encryption is rarely (if ever) useless when implemented properly. And given the security is a concern taken seriously.

Different approaches comparison

Let me just make a quick comparison of what I see we are doing now.

The Two-Factor Authentication (abbr. 2FA, not TFA):

And if we now discard the encryption - in any event of the SQLi, LFI, RFI, RCE, or any other method of DB dump retrieval (eg backups) - the shared secure key is served on a golden platter. Bye bye 2nd factor, the attacker can now generate one-time passwords at will. Thereafter, only the password has to be cracked, or social engineered, or attacker knows it already and just needed to circumvent the 2nd Factor... And, what's even worser - the average site owners will probably have no clue that their key is leaked and their website can be unauthorizedly accessed. They trust their Joomla still has a proper 2FA implementation!

We are undermining the 2FA security, which is clearly marketed as one of the security key points of the Joomla 3. Mere mortals trust that their Joomla authentication is much more secure with 2FA, so they can even use less secure passwords as well - OTP will prevent it to be circumvented, they told them.

A shared secret

I think that anyone could guess that something called a "secret key" should be kept as secret as possible.

The RFC 6238 specification, which defines Time based One-Time Password (TOTP), (basis of the Google Authenticator and majority of other modern one-time password implementations) leaves no doubt about it. https://tools.ietf.org/html/rfc6238#section-5.1

"... We also RECOMMEND storing the keys securely in the validation system, and, more specifically, encrypting them using tamper-resistant hardware encryption and exposing them only when required: for example, the key is decrypted when needed to verify an OTP value, and re-encrypted immediately to limit exposure in the RAM to a short period of time. The key store MUST be in a secure area, to avoid, as much as possible, direct attack on the validation system and secrets database ..."

We surely can't use the hardware encryption, but we can encrypt it properly (explained later). But it can't be more clear that the accepted patch solution is a breakout from the RFC specification.

"Thank You" goes to the PLT for a smart decision to compel the developer to encrypt the keys in the initial implementation. Yes, it is true - the implementation was flawed. Using a Joomla Config $secret as an encryption key is, undoubtedly, a flaw. Encryption key shouldn't be stored together with the encrypted data. Encryption 101. Nevertheless, the sensitive data was still encrypted and protected to a degree. But leaving the secret keys laying around as a plain-text is far worse than encryption with a theoretical flaw (which most of the attackers won't be able to circumvent anyway). It's not only wrong, I'll call it irresponsible. Period!

Web security theory considerations

Web Security has no one-to-rule-them-all solution. Web Security is a process of setting more and more layers of security measures. As much layers of security as possible. To make attacker's life miserable, because after one puzzle to solve there is another, and another, and another ... until the attacker eventually gives up and goes somewhere else. So we shouldn't discard any of the layers of security. We should try to add more of them. I hope at least some of the people (which I honestly respect because of their contributions) in this thread know that very well. So don't give up on the right idea.

Another important security related fact to keep in mind is the fact that anyone would (most probably) like to be aware, as soon as possible, that their website, or their credentials have been breached/leaked. If the attacker tampers the saved credentials data (it is really stupid to do that, but it happens often) then the site owner will notice that his credentials don't work anymore, something is wrong. If the attacker is able to get the plain-text password and the plain-text OTP secret key, the credentials tampering is not needed at all. He knows them. And the unauthorized access can go on for a long time before being detected.

A proposed alternative solution

pseudocode: OTP_secret_data_encryption_key = bcrypt(12, password_salt, username + plain_text_password + JConfig_secret)

This would force the attacker to gain access to :

Any of those missing - no cracking of encrypted OTP secret key possible, and therefore, no undiscovered unauthorized access to the website possible.

If developers of FOF external library aren't willing or able to implement the needed crypto wrapper library adoptions, then I humbly think Joomla doesn't need to be kept a hostage. A compatible native plugin can be built to be dependent only on the necessary crypto libs. I don't see why FOF is needed there anyway (no hard feelings). It is very easy to make small mistakes in the crypto code, which result in a big crypto flaw. And the internet is full of poorly written tutorials/examples. That is why I strongly discourage anyone from trying to reinvent the wheel. It is so easy to make a mistake if you are not experienced in cryptography. Not understanding, eg., the difference between CBC and EBC mode, and this one letter difference can inevitably lead to a vulnerability. (EBC mode is insecure and shouldn't be used)

Unfortunately, most of the users don't have the luxury of their own customizable PHP setup, to use the glorified "libsodium". I'd suggest to implement a PHP crypto lib wrapper "php-encryption" https://github.com/defuse/php-encryption. It requires PHP >= 5.4, with dependency only on the OpenSSL extension (which is "on" by default). If really necessary, we can make a fallback for PHP < 5.4. This high-level lib takes the guesswork out of a proper encryption with PHP by hiding the low-level sensitive encryption config and procedures and offering the clean understandable high-level methods anyone can use. "php-encryption" offers only secure encryption types (eg. only CBC mode is implemented), so no special cryptography knowledge is required. It is very well audited and regularly maintained lib. Both of the authors are experienced and known cryptographers, and they plan to maintain the lib at least until the start of 2019.

Conclusion

IMHO we firstly need a quick-fix which doesn't dump the encryption, to patch the current encryption incompatibility and leave the encryption key as is (JConfig::secret). For the next version, the compatible native plugin can be built to replace the old one. I can contribute parts of the code if needed.

So please, for the sake of all the millions of average users which started using 2FA and trust in the Joomla security agenda - stop this security degradation from being included in the official version. It is not too late. And I think there is no need to rush with the fix which will make stuff even worse. Let's fix it properly. Given the fact that Production and Security teams are really serious about web security of Joomla editions. And don't want the project to be ridiculed (again!) in the web security (and cryptography) circles. I think there was a fair share of it in the history of Joomla, and I wouldn't want to see that happening again.

mbabker commented 8 years ago

I can't say I can be bothered to read most of the wall of text. But have you considered the fact that the reason he's arguing to no longer encrypt the data might be because there's reason to do so to begin with? Because I could very easily spin around and mandate that the database API encrypt/decrypt data when issuing queries so that nothing is stored plaintext to the database.

I'd suggest to implement a PHP crypto lib wrapper "php-encryption"

This caught my eye. If you paid any attention you'd know that an older version of that library (compatible with PHP 5.3) has been in Joomla and usable via the JCrypt API since 3.5's release.

btoplak commented 8 years ago

I can't say I can be bothered to read most of the wall of text. But have you considered the fact that the reason he's arguing to no longer encrypt the data might be because there's reason to do so to begin with?

If there is a reason beyond what was written (in imperatives) here I would be very happy to know.

Yeah I know, it is really a wall of text. It took me quite some time to write it down. But with the intenton to back up my points with the reasoning descriptions. With the intention to cover most of the questions and explanations beforehand, to avoid the lenghty FAQ debate. I really didn't think anyone would read it all, it's a bit technical. I expect it will be read by the ones who (should) care about security. Please don't get me wrong. I know the time is a valuable asset, I myself have had waaay too little time for my Joomla duties lately because some private stuff.

Because I could very easily spin around and mandate that the database API encrypt/decrypt data when issuing queries so that nothing is stored plaintext to the database.

OK. And the encryption key would be what? Stored where? If you decide to read my wall of text you will note that I bothered to emphasize the important issue with an encrpytion key being stored on the same server as the encrypted text. And we are back on the begining of the encryption weakest point in this subject. Actually there are two weaknesess, first is that $secret is stored (in most cases) on the same server as the database. The second one is that the encryption key is always the same for any user. It shouldn't be.

This caught my eye. If you paid any attention you'd know that an older version of that library (compatible with PHP 5.3) has been in Joomla and usable via the JCrypt API since 3.5's release.

You are right, my fault. I was looking into /libraries/vendor/ folder, I'd expect it to lay there with other stuff. Mea culpa! As I said above, I was absent for some time. But then again, it's only one class of several in the original lib... There, we already have everything necessary to replace the whaky encryption stuff in the subject plugin.

mbabker commented 8 years ago

You are right, my fault. I was looking into /libraries/vendor/ folder, I'd expect it to lay there with other stuff. Mea culpa! As I said above, I was absent for some time. But then again, it's only one class of several in the original lib...

It's v1.1 of the original library (see https://github.com/joomla/joomla-cms/pull/8406 for the pull request adding it). That was the last version PHP 5.3 compatible and it also wasn't installable through Composer, hence it's not in the libraries/vendor directory. All production code from the upstream library is included in libraries/php-encryption (which really is one file).

btoplak commented 8 years ago

It's v1.1 of the original library

ok, tnx for the additional info

brianteeman commented 8 years ago

Thank you @rdeutz

On 20 October 2016 at 22:16, Robert Deutz notifications@github.com wrote:

Closed #12458 https://github.com/joomla/joomla-cms/issues/12458.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/joomla/joomla-cms/issues/12458#event-831249865, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPH8RRKLdCyu5eGP0SE7HY2a8I8fEDgks5q19o2gaJpZM4KaCxb .

Brian Teeman Co-founder Joomla! and OpenSourceMatters Inc. https://brian.teeman.net/ http://brian.teeman.net/

brianteeman commented 8 years ago

And I think there is no need to rush with the fix which will make stuff even worse

I am not going to explain why there is a need but anyone who has tested the current situation will not agree with this conclusion.

btoplak commented 8 years ago

I am not going to explain why there is a need but anyone who has tested the current situation will not agree with this conclusion.

I poorly expressed myself. Yes, absolutely, I agree that the situation with 2FA in 3.6.3 was not acceptable. I had to reset my own 2FA settings on tens of Joomla installations. What I wanted to say was that a fix made in a rush to patch the issue shouldn't skip to validate the decision to drop the secret key encryption. The rest of the @rdeutz patch is fine with me.