[#12458] - "Must match character set" message when logging in with 2FA enabled
- Closed
- 20 Oct 2016
- Medium
- Build: master
- # 12458
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.
fof/encrypt/totp.php ? paging @nikosdion :-)
I think this is a candidate for https://docs.joomla.org/Category:Version_3.6.3_FAQ
ITs a wiki - go for it
You think!? I'm locked out of 3 .org properties! (Well, not completely locked out, but the fact I now have to go into the databases of at least 4 sites, remove my 2FA configuration, and reset it is annoying to say the least)
Like I have time to learn what a wiki is and how to use one :)
Im not sure how FoF can be called 3rd party software though... its more like core Joomla now :)
Make your suggestion of edits here and I'll apply them if you don't want to directly edit the wiki
It is third party software distributed with the core Joomla package. Same with PHPMailer, or the lessc compiler.
Thanks @pe7er
This: https://github.com/joomla/joomla-cms/blob/staging/libraries/fof/autoloader/fof.php#L2-L7 should make clear it is third party software as Michael pointed out
I changed the sentence on JDocs using Michael's words
I actually think this problem is before the fof code, and in the onUserTwofactorAuthenticate method, the otp_config is not being loaded/passed in by Joomla for some reason... even further up the stack at
this might be the root issue.At the moment Joomla doesnt pass any config to the fof code to decode!
I have no yubikey here but i have two independet reports that yubikey works. So it looks like to be not a general problem with not passed configs?
Nope yubikey doesnt work - it completely ignores it and lets you login - at
least thats what it did on my upgraded site
Notice: Undefined index: yubikey in
/plugins/twofactorauth/yubikey/yubikey.php on line 93
On 18 October 2016 at 19:49, zero-24 notifications@github.com wrote:
I have no yubikey here but i have two independet reports that yubikey
works. So it looks like to be not a general problem with not passed configs?—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12458 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8b25lxbceYPyLWLSJRPrEyqaYs_qks5q1RSmgaJpZM4KaCxb
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/
@mbabker something strange happen. Can you try it again? I did have removed the 2fa from my account and created a new one. but i can't reproduce the problem with them? Also if i don't enter a code or a wrong code i get the expected warning and i can't login? I'm more than confused?
On the downloads site, I removed my otpKey and otep values from the database, deleted my old Google Authenticator config, and redid the setup for it. Logged out and able to log in again with 2FA.
tested a clean 3.6.3 and created a new user with 2FA with Google Authenticator and seems to work.
So what happen? Do you have any site with 2FA enabled without removed values from the database?
I have the same problem - I restored the site using the backup taken as part of the upgrade and still have the same problem - I'm presuming the restore has been applied over the 3.6.3 release and therefore something new is left behind from 3.6.3?
@andrepereiradasilva can you try setting up TFA on a 3.6.2 site and then
upgrading that
On 18 October 2016 at 20:15, dandpclements notifications@github.com wrote:
I have the same problem - I restored the site using the backup taken as
part of the upgrade and still have the same problem - I'm presuming the
restore has been applied over the 3.6.3 release and therefore something new
is left behind from 3.6.3?—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12458 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8Qv0W_xFdVJsdplgBB6jK2-MYYdvks5q1RrYgaJpZM4KaCxb
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/
@zero-24 I have 2FA enabled accounts on developer.joomla.org, www.joomla.org, and www.babdev.com I can still test with. But my account on downloads.joomla.org is most assuredly now completely reset and unusable to test any further with. I also have an account on seo.hdwebpros.com without 2FA but as we've isolated the issue to only the 2FA side of things that's irrelevant.
So what it seems is that something with the FOF upgrade has resulted in old data being unreadable. At least that's my first guess at a far off glance.
The root issue is we upgraded to a new version of FOF to deal with PHP 7.1 support https://github.com/joomla/joomla-cms/blob/staging/libraries/fof/encrypt/aes.php#L47
Result is that we were using FOFEncryptAesMcrypt and moved to FOFEncryptAesOpenssl - but this now means everyone on Mcrypts keys are invalid :(
Locked out of website is serious issue, and 2FA on administrator accounts of many websites is not uncommon.
Also what about other user accounts with 2FA ?
they all have to do redo 2FA configuration ? how are they going to do this ?
Maybe the distribution of J3.6.3 should be stopped ?
I have just now renamed folder,
plugins/twofactorauth/
so that users using 2FA (myself included) can login
As I said almost four years ago: encrypting the two factor authentication configuration with the site's secret key is a stupid, pointless idea. Yet the PLT had me implement it anyway, despite my objections, in order to accept the PR of the TFA feature.
The two factor authentication settings are stored in the users table. You had me encrypt them with the site's secret key. When you are trying to log in with TFA we read that data from the database and try to decrypt them using the current secret key. If there's a mismatch the decryption fails, the data is corrupt and you get this error message.
Another change made in FOF recently has to do with the way the encryption works. Up until Joomla 3.6.2 we would only use mcrypt. However, since mcrypt is now marked as dead, I have implemented decryption with OpenSSL as well and it's the preferred decryption. In all test environments I have the decryption works just fine either way. I suspect that if you are on a borked OpenSSL installation the decryption might break, throwing this error.
It all boils down to the PLT making a really wrong call despite my objections, forcing me to implement a USELESS encryption (since the key is stored in the same memory space as the encrypted data, duh!). Now excuse me, I'm in the middle of moving, packing stuff in boxes and my Internet connection will be cut off in 24 hours for a little over a week. Next time I tell you something is a bad idea just bloody trust me instead of waiting for 2 to 4 years until it blows up in your face and have me tell you "I told you so". Pretty please. We've been playing this game since 2009, it's becoming really tiresome.
@ggppdk Stopping distribution of the release fixes nothing and frankly (as someone who used to coordinate this stuff) I find it insulting when people suggest this after a release has been issued, especially given the open development process and the numerous calls for testing that go into releases. We are NOT talking about an issue that causes the site to fail over miserably. Ya, you can't log in now, but is your site experiencing fatal errors? That's the only condition I find acceptable to stop distribution.
I find it insulting when people suggest this after a release has been issued, especially given the open development process and the numerous calls for testing that go into releases. We are NOT talking about an issue that causes the site to fail over miserably. Ya, you can't log in now, but is your site experiencing fatal errors? That's the only condition I find acceptable to stop distribution.
I am sorry, i was just asking
It's not something specifically against you, but as someone who did this stuff and dealt with people crying "my site is broken, Joomla sucks, stop distributing the package", it gets annoying having to keep hearing it. Even when I haven't been involved in that process for close to two years.
I suspect that if you are on a borked OpenSSL installation the decryption might break, throwing this error
Well, glad to know that Rochen and SiteGround have borked OpenSSL installations...
@brianteeman yes, can confirm, 2FA in clean 3.6.2 work fine. upgrade to 3.6.3, login with 2FA we get the error.
aditional info: tested in CentOS 7.x (latest) with centos openssl package
Your comment and that aof andre actually confirms that it is
On 18 October 2016 at 20:38, Alexander Jochum notifications@github.com
wrote:
Currently debugging the code and at the moment to me it looks like
decoding the old TwoFactorAuthentication config doesn't work anymore in
getOtpConfig of com_user/models/users.php file.$otpConfig->config = @json_decode($decryptedConfig);
will return null and so everything afterwards fails.
Disabling 2FA temporary to login, disabling 2FA and reenabling 2FA again
worked for me.
In my opinion the problem is not the third party library for 2FA..—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12458 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8XQeXShBApjGEoHrT0XusiWhKzdBks5q1SAfgaJpZM4KaCxb
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/
@brianteeman Sorry my fault I missed the line
$aes = new FOFEncryptAes($key, 256);
after replacing the fof library folder of Joomla 3.6.3 installation with one of Joomla 3.6.2 it worked again. That's why I removed my comment again, I was too fast. Sorry
http://stackoverflow.com/questions/31520311/decrypt-mcrypt-with-openssl
If you encrypt in mcrypt without adding PKCS7 manually, mcrypt will happily pad your plaintext with NUL bytes.
OpenSSL will do PKCS7 padding for you whenever using aes-X-cbc. The unfortunate consequence of this is that if you have AES-CBC(NULL_PADDED(plaintext)) and try to decrypt it, openssl_decrypt will attempt to remove the padding and fail.
Maybe this can help more high level PHP developers than me, do we do anything with PKCS7 and mycrypt i can't find anything?
If not it sounds to me that is is not a borked (whatever borked means) OpenSSL installation
Log in, get error, click return to control panel which brings you back to log in.
Log in without putting in your secret key and you are now logged in.
? The bad part is key is still living so if you log out you can log back in without the secret key!!!? Then you can disable 2FA, then re-enable with a new key. 2FA starts working as expected again.
Thats exctly what I got on my test site
On 18 October 2016 at 21:12, Tom Hutchison notifications@github.com wrote:
1.
Log in, get error, click return to control panel which brings you back
to log in.
2.Log in without putting in your secret key and you are now logged in.
?
The bad part is a session is still living so if you log out you can log
back in without the secret key!!!?
3.Then you can disable 2FA, then re-enable with a new key. 2FA starts
working as expected again.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12458 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8eyFtgPHunWhIYqLdeHk_6MzU29sks5q1Sg6gaJpZM4KaCxb
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/
Not sure if this is really a fixable bug without some conversion if Joomla wants to use openssl instead of mcrypt in the future.
What I tried was to add a class which extends FOFUtilsPhpfunc and override the __call function to return false.
class TwoFactorAuthConversion extends FOFUtilsPhpfunc
{
public function __call($func, $args)
{
return false;
}
}
Now every creation of an FOFEncryptAes object must be modified so that we pass a new object of our new class as last parameter.
In administrator/components/com_user/models/users.php for example every
new FOFEncryptAes($key, 256);
must be changed to
new FOFEncryptAes($key, 256, 'cbc', new TwoFactorAuthConversion() );
this will then force FOFEncryptAes to use FOFEncryptAesMcrypt class as adapter instead of the openssl class. It's not really nice but this wouldn't require a modification of the FOF library.
Maybe the updater could do something like this and convert the existing OTP configurations of each user from mcrypt to openssl.
This will of course not help any user who already updated to Joomla 3.6.3..
Ok I added an automated conversion in getOtpConfig function.
You must of course add the TwoFactorAuthConversion class as stated in my previous post (only the class not the other modifications!).
But then this will automatically tries the mcrypt method for decryption and if successful calls setOtpConfig to set the configuration in OpenSSL format..
Here is the code that I modified in getOtpConfig of administrator/components/com_user/models/users.php file.
/*
* If the decryption failed for any reason we essentially disable the
* two-factor authentication. This prevents impossible to log in sites
* if the site admin changes the site secret for any reason.
*/
if (is_null($otpConfig->config))
{
$mcryptConversion = new FOFEncryptAes($key, 256, 'cbc', new TwoFactorAuthConversion());
// Decrypt the data
$decryptedConfig = $mcryptConversion->decryptString($encryptedConfig);
$decryptedOtep = $mcryptConversion->decryptString($encryptedOtep);
// Remove the null padding added during encryption
$decryptedConfig = rtrim($decryptedConfig, "\0");
$decryptedOtep = rtrim($decryptedOtep, "\0");
// Update the configuration object
$otpConfig->method = $method;
$otpConfig->config = @json_decode($decryptedConfig);
$otpConfig->otep = @json_decode($decryptedOtep);
// If it worked now and config isn't null we call setOtpConfig to do the conversion to OpenSSL else behave as before.
if (is_null($otpConfig->config))
{
$otpConfig->config = array();
}
else
{
$this->setOtpConfig($user_id, $otpConfig);
}
}
God, no! That TwoFactorAuthConversion crap makes all calls to PHP builtins to return false without executing! That screws up the decryption, essentially always returning a null reply. That's the most roundabout way you can go to disabling TFA. Better just delete the TFA plugins altogether, then!
Apparently none of you is capable of seeing the simple truth right in front of your noses. I am disappointed. Doubly for seeing the crazy workarounds which only compound the problem instead of solving it. Stop breaking Joomla, damn you!!! THINK BEFORE YOU CODE!!!
A cursory examination of FOFEncryptAesOpenssl::setEncryptionMode reveals the nature of the problem and why I'm talking about broken OpenSSL implementations. When you instantiate an AES 256-bit key CBC instance the adapter sets the target OpenSSL algorithm to aes-256-cbc. However, if the OpenSSL extension does not report that algorithm as available the $defaultAlgo is used instead. On these broken implementation you have aes-128-cbc but not aes-256-cbc. Since the wrong algorithm is used you end up with garbage decoded data which cannot be parsed as JSON, throwing the error message you get.
The only thing you could do is forcibly instantiate the FOFEncryptAesMcrypt adapter and decode the data using the same code as FOFEncryptAes::decryptString (the other methods you need are public on that class).
This is a short term solution which will throw warnings in PHP 7.1 and probably break in 7.2. Ideally you'd use the trick above to decrypt the data and then write it decrypted to the database. I could analyze the threat models and show you that encryption makes no sense for TFA configuration but I've already done that four years ago and the PLT seems to have been too dumb to even pretend to listen to me. Meh.
Now please allow me to go to sleep because I'm still in the middle of packing an entire house into boxes for moving.
My algo is aes-256-cbc in that method but I'm still experiencing the break. But as you describe the Json data is clearly garbage coming out. Still trying to investigate where exactly the problem lies and if it's something to do with the white padding of the mcrypt message as tobias' stack overflow link implies or something unrelated
OK So I'm sure that I've got the aes-256-cbc locally and @mbabker tested and got that same result on Rochen. So at least on some hosts there appears to be a deeper problem. I looked into the issue @zero-24 supplied on StackOverflow and whilst adding PKCS7 to the mcrypt appeared to make some sort of difference (it removed white space from the result) but it certainly didn't fix the issue - the decrypted data still was meaningless - just without whitespace in it.
I've set myself up a basic script of 4 files that encrypts in the FOFEncryptAes from J3.6.2 then decrypts it with FOFEncryptAes from 3.6.3 (with some hacking to remove some of the new 3.6.3 dependencies which were not required).
Will take another shot at things tomorrow. But have to say I'm running out of ideas fast :(
@nikosdion That's true but I'm pretty sure that I never said that this is a good or the final solution and I also never created a pull request for this as there are other things I don't like about this.
I just posted it that someone else with a bit more brain could find a better solution based on this, without modifying the original third party library and writing its own decryption method using FOFEncryptAesMcrypt.
This also wouldn't break TFA completely as the TwoFactorAuthConversion object is only used in FOFEncryptAes constructor to do some checks and if they fail it'll use mcrypt instead of openssl.
To not return false in general you could modify TwoFactorAuthConversion __call function so that it behaves as usual for everything but return false for a function call done in FOFs FOFEncryptAesOpenssl isSupported function, like
$phpfunc->function_exists('openssl_get_cipher_methods')
Beside the fact that my class naming is also pretty shitty I also wouldn't add this piece of crap code in that way into standard Joomla code as final solution. Also I would only add somekind of conversion functionality (not my crappy solution) in Joomlas upgrade process and not its standard code.
In my opinion this problem shouldn't have found its way into the final release at all. Maybe it would have been better to use the previous version of FOF library until there was a good solution for this problem but hey I'm just someone who writes shitty code so what do I know.
@wilsonge Don't just revert FOF to the previous version and release Joomla again. As we already established, FOF 2 was not PHP 7 compatible.
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?
How to do it. Read the config. Split it on : as we do now which leaves you with the plugin name and the config data. Check if there's a { in the config data and try to parse it as JSON (the config data is always an object). If the former is false or the latter yields null the data is encrypted. Decrypt it and retry. Upon success save it back to the db. It's not rocket science.
Finally, let me please note that FOF 2 is END OF LIFE. I'm done trying to maintain it for Joomla 3. I tried really hard to backport necessary changes to support PHP versions that FOF 2 was never intended to support but there's only so much I can do and nobody seems to give a flying damn about doing proper testing even when I explicitly ask you to. I don't see the point of assuming all risk of a release and having zero control over it so from now on you're strictly on your own.
Then when about when upgrading from J3.6.2 , reconfiguration of TFA is forced my Joomla ?
I mean if upgrading from <=J3.6.2, then existing data are cleared, and a new message appears in backend dashboard about the fact that data had been cleared, and need to reconfigure it
In my opinion this problem shouldn't have found its way into the final release at all.
You're right. It shouldn't have. The fact it did means that not enough folks tested either the pull request updating FOF (in August) or after its merge updating sites to the current development head with 2FA enabled. There's no point in further beating the dead horse, stuff happens.
Then when about when upgrading from J3.6.2 , reconfiguration of TFA is forced my Joomla ?
Unacceptable.
Can we start a "joomla RC testing checklist" then? (said it before... said it again) so that we can add things like this to a solid checklist so that this "feature" is always tested by testers ?
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?
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.
(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
#12458 (comment),
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/
@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.
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...
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
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.
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.
It was stated right in the pull request which should have been fully tested before merging. #11673
Otherwise what you're expecting is that an announcement goes out stating:
- Validate that every JSON data source is correctly read via the
RegistryAPI (in the case of ajoomla/registryupdate) - Validate 2FA works
- Validate the post-install component works
- Resave the global configuration and validate that when a language other than English is selected and the docs site has pages for your language that the translated content is returned
- The media player displaying video in the Media Manager is still functional
- Each WYSIWYG editor is functional and no integrations have been broken
- The cache API continues to function as expected (define expected) with each supported handler and caching completely disabled
- Attributes on a
JDocumentErrorinstance created by the global error handler match those set to anyJDocumentinstance created byJFactory - You can access the debug user and group ACL screens by manually typing in the URL without debug mode being enabled
- Marks from the
JProfilerAPI use the correct data type - jQuery Autocomplete still works right
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.
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" :-)
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.
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.
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.
@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?!
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.
Just a couple of sources to share:
- https://wiki.php.net/rfc/mcrypt-viking-funeral - The PHP RFC where mcrypt was proposed for deprecation and moving to PECL
-
https://marc.info/?l=php-internals&m=147569474024259&w=2 - Thread on PHP's Internals mailing list where they've started discussing setting up the standalone repository for
ext/mcrypt
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.
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.
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.
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.
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-10-20 21:16:37 |
| Closed_By | ⇒ | rdeutz |
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):
- 1st Factor (the password, hated or beloved) = we properly take care of the password storage in the database - it is being properly salted, hashed with a decently modern password hashing algo with key-stretching in place, before being saved to the database.
- 2nd Factor (one time password, OTP) = we encrypted the shared secret key, up until now (thanks, PLT). But, suddenly, we don't care about the secrecy and security of the OTP secret keys. We will now store secret key as a plain text.
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 :
- username
- plain-text password
- Jconfig $secret (access to the filesystem)
- salt (access to database)
- encrypted OTP data (access to database)
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.
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.
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.
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 #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).
It's v1.1 of the original library
ok, tnx for the additional info
Thank you @rdeutz
On 20 October 2016 at 22:16, Robert Deutz notifications@github.com wrote:
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12458 (comment), 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/
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.
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.
From the logs:
@nikosdion The trace ends in FOF code. Can you look into this please?