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

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

Closed
mbabker opened this Issue Oct 18, 2016 · 79 comments

Comments

Projects
None yet
@mbabker
Member

mbabker commented Oct 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 18, 2016

Member

From the logs:

Uncaught Exception of type Exception thrown. Stack trace:
#0 /libraries/fof/encrypt/base32.php(183): FOFEncryptBase32->toBin('')
#1 /libraries/fof/encrypt/totp.php(114): FOFEncryptBase32->decode(NULL)
#2 /plugins/twofactorauth/totp/totp.php(276): FOFEncryptTotp->getCode(NULL)
#3 [internal function]: PlgTwofactorauthTotp->onUserTwofactorAuthenticate(Array, Array)
#4 /libraries/joomla/event/event.php(69): call_user_func_array(Array, Array)
#5 /libraries/joomla/event/dispatcher.php(159): JEvent->update(Array)
#6 /libraries/joomla/application/base.php(106): JEventDispatcher->trigger('onUserTwofactor...', Array)
#7 /libraries/fof/integration/joomla/platform.php(545): JApplicationBase->triggerEvent('onUserTwofactor...', Array)
#8 /plugins/authentication/joomla/joomla.php(146): FOFIntegrationJoomlaPlatform->runPlugins('onUserTwofactor...', Array)
#9 /libraries/joomla/user/authentication.php(284): PlgAuthenticationJoomla->onUserAuthenticate(Array, Array, Object(JAuthenticationResponse))
#10 /libraries/cms/application/cms.php(833): JAuthentication->authenticate(Array, Array)
#11 /libraries/cms/application/administrator.php(324): JApplicationCms->login(Array, Array)
#12 /administrator/components/com_login/controller.php(64): JApplicationAdministrator->login(Array, Array)
#13 /libraries/legacy/controller/legacy.php(702): LoginController->login()
#14 /administrator/components/com_login/login.php(22): JControllerLegacy->execute('login')
#15 /libraries/cms/component/helper.php(405): require_once('')
#16 /libraries/cms/component/helper.php(380): JComponentHelper::executeComponent('')
#17 /libraries/cms/application/administrator.php(98): JComponentHelper::renderComponent('com_login')
#18 /libraries/cms/application/administrator.php(152): JApplicationAdministrator->dispatch()
#19 /libraries/cms/application/cms.php(261): JApplicationAdministrator->doExecute()
#20 /administrator/index.php(51): JApplicationCms->execute()
#21 {main}

@nikosdion The trace ends in FOF code. Can you look into this please?

Member

mbabker commented Oct 18, 2016

From the logs:

Uncaught Exception of type Exception thrown. Stack trace:
#0 /libraries/fof/encrypt/base32.php(183): FOFEncryptBase32->toBin('')
#1 /libraries/fof/encrypt/totp.php(114): FOFEncryptBase32->decode(NULL)
#2 /plugins/twofactorauth/totp/totp.php(276): FOFEncryptTotp->getCode(NULL)
#3 [internal function]: PlgTwofactorauthTotp->onUserTwofactorAuthenticate(Array, Array)
#4 /libraries/joomla/event/event.php(69): call_user_func_array(Array, Array)
#5 /libraries/joomla/event/dispatcher.php(159): JEvent->update(Array)
#6 /libraries/joomla/application/base.php(106): JEventDispatcher->trigger('onUserTwofactor...', Array)
#7 /libraries/fof/integration/joomla/platform.php(545): JApplicationBase->triggerEvent('onUserTwofactor...', Array)
#8 /plugins/authentication/joomla/joomla.php(146): FOFIntegrationJoomlaPlatform->runPlugins('onUserTwofactor...', Array)
#9 /libraries/joomla/user/authentication.php(284): PlgAuthenticationJoomla->onUserAuthenticate(Array, Array, Object(JAuthenticationResponse))
#10 /libraries/cms/application/cms.php(833): JAuthentication->authenticate(Array, Array)
#11 /libraries/cms/application/administrator.php(324): JApplicationCms->login(Array, Array)
#12 /administrator/components/com_login/controller.php(64): JApplicationAdministrator->login(Array, Array)
#13 /libraries/legacy/controller/legacy.php(702): LoginController->login()
#14 /administrator/components/com_login/login.php(22): JControllerLegacy->execute('login')
#15 /libraries/cms/component/helper.php(405): require_once('')
#16 /libraries/cms/component/helper.php(380): JComponentHelper::executeComponent('')
#17 /libraries/cms/application/administrator.php(98): JComponentHelper::renderComponent('com_login')
#18 /libraries/cms/application/administrator.php(152): JApplicationAdministrator->dispatch()
#19 /libraries/cms/application/cms.php(261): JApplicationAdministrator->doExecute()
#20 /administrator/index.php(51): JApplicationCms->execute()
#21 {main}

@nikosdion The trace ends in FOF code. Can you look into this please?

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Oct 18, 2016

Contributor

Confirmend maybe realted to the untested for update?

#11673

Contributor

zero-24 commented Oct 18, 2016

Confirmend maybe realted to the untested for update?

#11673

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor Oct 18, 2016

Contributor

fof/encrypt/totp.php ? paging @nikosdion :-)

Contributor

PhilETaylor commented Oct 18, 2016

fof/encrypt/totp.php ? paging @nikosdion :-)

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor
Contributor

PhilETaylor commented Oct 18, 2016

I think this is a candidate for https://docs.joomla.org/Category:Version_3.6.3_FAQ

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Oct 18, 2016

Contributor

ITs a wiki - go for it

Contributor

brianteeman commented Oct 18, 2016

ITs a wiki - go for it

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 18, 2016

Member

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)

Member

mbabker commented Oct 18, 2016

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)

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor Oct 18, 2016

Contributor

Like I have time to learn what a wiki is and how to use one :)

Contributor

PhilETaylor commented Oct 18, 2016

Like I have time to learn what a wiki is and how to use one :)

@zero-24

This comment has been minimized.

Show comment
Hide comment
@Sandra97

This comment has been minimized.

Show comment
Hide comment
@Sandra97

Sandra97 Oct 18, 2016

As usual, thank you @zero-24!

Sandra97 commented Oct 18, 2016

As usual, thank you @zero-24!

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor Oct 18, 2016

Contributor

Im not sure how FoF can be called 3rd party software though... its more like core Joomla now :)

Contributor

PhilETaylor commented Oct 18, 2016

Im not sure how FoF can be called 3rd party software though... its more like core Joomla now :)

@Sandra97

This comment has been minimized.

Show comment
Hide comment
@Sandra97

Sandra97 Oct 18, 2016

Make your suggestion of edits here and I'll apply them if you don't want to directly edit the wiki

Sandra97 commented Oct 18, 2016

Make your suggestion of edits here and I'll apply them if you don't want to directly edit the wiki

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 18, 2016

Member

It is third party software distributed with the core Joomla package. Same with PHPMailer, or the lessc compiler.

Member

mbabker commented Oct 18, 2016

It is third party software distributed with the core Joomla package. Same with PHPMailer, or the lessc compiler.

@pe7er

This comment has been minimized.

Show comment
Hide comment
@pe7er

pe7er Oct 18, 2016

Contributor

thanks @zero-24, I've added some category info to the bottom your wiki addition.

Contributor

pe7er commented Oct 18, 2016

thanks @zero-24, I've added some category info to the bottom your wiki addition.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Oct 18, 2016

Contributor

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

Contributor

zero-24 commented Oct 18, 2016

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

@Sandra97

This comment has been minimized.

Show comment
Hide comment
@Sandra97

Sandra97 Oct 18, 2016

I changed the sentence on JDocs using Michael's words

Sandra97 commented Oct 18, 2016

I changed the sentence on JDocs using Michael's words

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor Oct 18, 2016

Contributor

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

if (!array_key_exists('otp_config', $options))
this might be the root issue.

At the moment Joomla doesnt pass any config to the fof code to decode!

Contributor

PhilETaylor commented Oct 18, 2016

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

if (!array_key_exists('otp_config', $options))
this might be the root issue.

At the moment Joomla doesnt pass any config to the fof code to decode!

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Oct 18, 2016

Contributor

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?

Contributor

zero-24 commented Oct 18, 2016

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?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Oct 18, 2016

Contributor

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/

Contributor

brianteeman commented Oct 18, 2016

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/

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Oct 18, 2016

Contributor

@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?

Contributor

zero-24 commented Oct 18, 2016

@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?

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 18, 2016

Member

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.

Member

mbabker commented Oct 18, 2016

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.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Oct 18, 2016

Contributor

tested a clean 3.6.3 and created a new user with 2FA with Google Authenticator and seems to work.

Contributor

andrepereiradasilva commented Oct 18, 2016

tested a clean 3.6.3 and created a new user with 2FA with Google Authenticator and seems to work.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Oct 18, 2016

Contributor

So what happen? Do you have any site with 2FA enabled without removed values from the database?

Contributor

zero-24 commented Oct 18, 2016

So what happen? Do you have any site with 2FA enabled without removed values from the database?

@dandpclements

This comment has been minimized.

Show comment
Hide comment
@dandpclements

dandpclements Oct 18, 2016

Contributor

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?

Contributor

dandpclements commented Oct 18, 2016

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?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Oct 18, 2016

Contributor

@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/

Contributor

brianteeman commented Oct 18, 2016

@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/

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 18, 2016

Member

@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.

Member

mbabker commented Oct 18, 2016

@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.

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Oct 18, 2016

Contributor

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 :(

Contributor

wilsonge commented Oct 18, 2016

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 :(

@ggppdk

This comment has been minimized.

Show comment
Hide comment
@ggppdk

ggppdk Oct 18, 2016

Contributor

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

Contributor

ggppdk commented Oct 18, 2016

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

@nikosdion

This comment has been minimized.

Show comment
Hide comment
@nikosdion

nikosdion Oct 18, 2016

Contributor

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.

Contributor

nikosdion commented Oct 18, 2016

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.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Oct 18, 2016

Contributor

@wilsonge so we can workarrund the FOF B/C break by changing the order of the method tests? So if mycrypt is there use it over openSSL?

Contributor

zero-24 commented Oct 18, 2016

@wilsonge so we can workarrund the FOF B/C break by changing the order of the method tests? So if mycrypt is there use it over openSSL?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Oct 19, 2016

Contributor

(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/

Contributor

brianteeman commented Oct 19, 2016

(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/

@nikosdion

This comment has been minimized.

Show comment
Hide comment
@nikosdion

nikosdion Oct 19, 2016

Contributor

@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.

Contributor

nikosdion commented Oct 19, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor Oct 19, 2016

Contributor

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...

Contributor

PhilETaylor commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Oct 19, 2016

Contributor

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

Contributor

wilsonge commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 19, 2016

Member

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.

Member

mbabker commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor Oct 19, 2016

Contributor

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.

Contributor

PhilETaylor commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 19, 2016

Member

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 Registry API (in the case of a joomla/registry update)
  • 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 JDocumentError instance created by the global error handler match those set to any JDocument instance created by JFactory
  • You can access the debug user and group ACL screens by manually typing in the URL without debug mode being enabled
  • Marks from the JProfiler API 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.

Member

mbabker commented Oct 19, 2016

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 Registry API (in the case of a joomla/registry update)
  • 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 JDocumentError instance created by the global error handler match those set to any JDocument instance created by JFactory
  • You can access the debug user and group ACL screens by manually typing in the URL without debug mode being enabled
  • Marks from the JProfiler API 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.

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor Oct 19, 2016

Contributor

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" :-)

Contributor

PhilETaylor commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 19, 2016

Member

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.

Member

mbabker commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@DarkCloud14

DarkCloud14 Oct 19, 2016

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.

DarkCloud14 commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 19, 2016

Member

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.

Member

mbabker commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@DarkCloud14

DarkCloud14 Oct 19, 2016

@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?!

DarkCloud14 commented Oct 19, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 19, 2016

Member

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.

Member

mbabker commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 19, 2016

Member

Just a couple of sources to share:

Member

mbabker commented Oct 19, 2016

Just a couple of sources to share:

@Bakual

This comment has been minimized.

Show comment
Hide comment
@Bakual

Bakual Oct 19, 2016

Contributor

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.

Contributor

Bakual commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 19, 2016

Member

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.

Member

mbabker commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@nikosdion

nikosdion Oct 19, 2016

Contributor
Contributor

nikosdion commented Oct 19, 2016

@slibbe

This comment has been minimized.

Show comment
Hide comment
@slibbe

slibbe Oct 19, 2016

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.

slibbe commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz Oct 20, 2016

Contributor

closing this one because we have a PR #12497

Contributor

rdeutz commented Oct 20, 2016

closing this one because we have a PR #12497

@rdeutz rdeutz closed this Oct 20, 2016

@btoplak

This comment has been minimized.

Show comment
Hide comment
@btoplak

btoplak Oct 22, 2016

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.

btoplak commented Oct 22, 2016

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.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 22, 2016

Member

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.

Member

mbabker commented Oct 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@btoplak

btoplak Oct 23, 2016

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.

btoplak commented Oct 23, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Oct 23, 2016

Member

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).

Member

mbabker commented Oct 23, 2016

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).

@btoplak

This comment has been minimized.

Show comment
Hide comment
@btoplak

btoplak Oct 23, 2016

It's v1.1 of the original library

ok, tnx for the additional info

btoplak commented Oct 23, 2016

It's v1.1 of the original library

ok, tnx for the additional info

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Oct 25, 2016

Contributor

Thank you @rdeutz

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

Closed #12458 #12458.


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/

Contributor

brianteeman commented Oct 25, 2016

Thank you @rdeutz

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

Closed #12458 #12458.


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/

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Oct 25, 2016

Contributor

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.

Contributor

brianteeman commented Oct 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@btoplak

btoplak Oct 26, 2016

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.

btoplak commented Oct 26, 2016

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.

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