It's possible to bypass the GA auth. Steps to reproduce:
1. Request new password for the GA-protected login.
2. Use the link obtained in the email to log in to the website.
3. Set new password and go to GA tab.
4. Set up new GA code (thus overriding already existing).

As you see, old GA code is not checked at any step.

CommentFileSizeAuthor
#11 ga_login_bypass.patch3.52 KBbcdev
#8 2028373-8.patch2.36 KBAnonymous (not verified)
#3 2028373-3.patch2.01 KBAnonymous (not verified)

Comments

attiks’s picture

attiks commented
Category: bug » feature

True, but this is Drupal behaviour, so not sure if this really qualifies as a bug.

Possible solutions:

  1. Add a GA login on the reset password login screen
  2. Force the user to enter his old code when creating a new one

Possible problems:
What if the user looses his phone? If we force them to enter their code they will not be able to login?

Google uses a backup by sending a txt message, but this will complicate every install. We could add an option to ga_login allowing admins to enable one of the solutions?

Feedback?

attiks’s picture

attiks commented
Priority: Critical » Normal

Changing priority

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 2028373-3.patch2.01 KB

True, but this is Drupal behaviour, so not sure if this really qualifies as a bug.

@1 Why do you say this is a Drupal behavior? You should be forced to enter the old GA code before generating a new one. I find this a critical bug (technically a "critically missing feature"), because it circumvents the purpose of GA security.

Patch with solution attached.

You can use the new function _galogin_validate_code in all your custom form API forms. It will validate the form input as a GA code. I use this for custom forms where I ask for the GA code, like withdrawing funds form a user account.

Example usage:

  global $user;
  if (user_is_logged_in() && _ga_login_account_has_code($user)) {
    $form['gacode'] = array(
      '#type' => 'textfield',
      '#title' => 'Code',
      '#description' => 'Enter your Google Authenticator code.',
      '#element_validate' => array('_galogin_validate_code'),
      '#maxlength' => 6,
      '#size' => 6,
      '#required' => TRUE,
    );
  }
Anonymous’s picture

Anonymous (not verified) commented
Version: 7.x-1.4 » 7.x-1.x-dev
Priority: Normal » Critical
Status: Active » Reviewed & tested by the community

What if the user looses his phone? If we force them to enter their code they will not be able to login?

They will need to report their code missing. The site admin can than decide to remove the GA code from the account. I don't think this can be automated. People will need to prove their identity after losing a GA code, that's the point of it.

attiks’s picture

attiks commented
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

#4 This will put a burden on all site admins, I agree there are sites that want to do this, but we should leave it to the site owner to decide which way to go: allow password resets by email (and bypassing ga_login) or force users to contact the site admin the reset their code.

If you can add this option (I'm fine with defaulting to the second behaviour, but only for new installs) to your patch, I think we're good to go. Maybe you can also add an option to do a redirect to a certain page (like a contact form) if they lost their phone?

+++ b/ga_login.moduleundefined
@@ -192,6 +192,7 @@ function _ga_login_explanation_form($form, &$form_state, $account) {
+  global $user;

why do you use user in here, $account is passed?

Anonymous’s picture

Anonymous (not verified) commented

I see, it should use $account indeed.

Makes sense to add a configuration option, or password reset indeed.

attiks’s picture

attiks commented

#6 Are you going to upload a new patch, so we can add it to the new release?

Anonymous’s picture

Anonymous (not verified) commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2028373-8.patch2.36 KB

Added the updated patch with optional feature.

However, this is still NOT secure: as per the original issue, hackers can still use the email link to login anyway, without ever entering a GA code. This beats the purpose of GA security: you should ALWAYS enter it.

Especially for financial websites like Bitcoin wallets. Hackers could steal all your Bitcoin by intecepting the email login link. I'm working on improvements to make this extension more secure.

attiks’s picture

attiks commented

#8 as said in #5 I'm fine with protecting the reset link with a token as well, but this has to be configurable so site administrators can choose to enable it or not. As a side note, google uses the same mechanism, if you lost your phone (authenticator app), you can use a backup code or a backup email address.

I assume sites using this will uses it on top op SSL.

Looking forward to your improvements.

bcdev’s picture

bcdev commented

see below.

bcdev’s picture

bcdev commented
StatusFileSize
new ga_login_bypass.patch3.52 KB

So I am pushing out a production site and this bug was just discovered on our platform as well. I don't have time to wait for a solution to be built out, so I have just patched user.pages.inc. I know, I know, you're not supposed to patch core. But this fixes the issue and will allow us to launch.

It basically adds a 2FA code field to the "I forgot my password" page (/user/password). If they don't enter their 2FA along with their username, the system won't validate their request.

attiks’s picture

attiks commented

#11 you could achieved the same using hook_form_alter

Can someone work on the patch in #8, the only thing missing is a sitewide option to enable the protection of the reset link

eanushan’s picture

eanushan commented

I think the approach in #2028373-11: Bypassing GA auth vulnerability is correct. It's simple and elegant. Do not allow the user to request a password reset e-mail if two-factor authentication is mandatory for their account.

I will refactor the patch so that it doesn't modify core. Instead, we can accomplish the same thing using a form alter hook to add the field to the password reset form. I'll add a configuration parameter to the ga_login admin page to enable/disable this additional protection as well since it will add some overhead to site administrators when enabled if a user loses their authenticator device.

eanushan’s picture

eanushan commented
Status: Needs review » Needs work
attiks’s picture

attiks commented

A quick heads up I did some coding style cleanup so you probably have to do a reroll

eanushan’s picture

eanushan commented

Depending on how #1834290: I would like to see a 2-step login page is implemented, this may work out of the box. If we implement #1834290 by hooking into the user login process, it will work for password resets as well. As soon as you click the one time link and press "Log in", you should be prompted with the second part of the login form where you enter your code. Not too sure on this. Need to try it.

attiks’s picture

attiks commented

FYI: Latest dev version now contains a form api field type 'gacode', so you could use hook_form_alter to add it to the reset login page

alexmoreno’s picture

alexmoreno commented

In my opinion, this solves the issue of changing the password, but not the fact that the "hacker" has already access to your Drupal account. Maybe an option to remove the reset link for some roles would be useful here.

cubeinspire’s picture

cubeinspire commented

Why not using hook_form_FORM_ID_alter to add an extra field at the form that ask for the email or username to send a recover link ?
The extra field would ask the GA Login code and during form validation GA Login would verify that the code is correct for that specific user.
A checkbox could be added at the settings page of GA Login to add the code at user/pass