google / password-alert

A Chrome Extension to help protect against phishing attacks.
Apache License 2.0
306 stars 92 forks source link

Updated for new login page and two factor #107

Closed Rickerd0613 closed 6 years ago

Rickerd0613 commented 6 years ago

Looks like Google updated their login page again and hashes for new users were not being saved. In our testing "gaia_loginform" seems to not exist on the login page. Additionally, we had problems with it saving the hash when two factor was enabled. To solve for these problems, we changed the way the Google login form is identified and store the username/password in temporary variables that are retrieved on form submit.

Tested with both consumer and enterprise modes.

Credit to Jacob Rickerd jacobrickerd@gmail.com, Zack Hardie zackhardie@gmail.com, and Duo Security.

Thanks for making an awesome tool!

googlebot commented 6 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Rickerd0613 commented 6 years ago

I had a different email set in git commit when I made the commit (whoops). Zack Hardie and I have signed the CLA. I can submit another PR with the proper email set if you'd like. Let me know if there are any more issues.

googlebot commented 6 years ago

So there's good news and bad news.

:thumbsup: The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

:confused: The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Rickerd0613 commented 6 years ago

We agree that the domContentLoaded changes are not the best but it was the only way we could detect when a user moved to the password prompt in the login flow (see comment above). We more than welcome a better solution to any of these patches that we wrote.

On another note, we have a company kickoff out of town next week and probably won't be able to look at this too much while we are away but don't be afraid to reach out.

We'll follow your lead with what changes you think should be made to the PR before it's ready. Let us know if you are having trouble reproducing our original issues. We could probably provide screenshots and/or a video if that'd help.

@zhardie feel free to jump in if you had any other thoughts.

Thanks for helping us fix this!

adhintz commented 6 years ago

I'll merge this PR as-is and then do another change on top of it.

Thanks again for pointing out this issue and the PR. Enjoy the company trip!

zhardie commented 6 years ago

@adhintz have you tried it with 2FA enabled? We've found that those elements are removed when the 2fa page is loaded.

On Feb 6, 2018 1:43 PM, "Drew Hintz" notifications@github.com wrote:

@adhintz commented on this pull request.

In chrome/content_script.js https://github.com/google/password-alert/pull/107#discussion_r166421213:

   if (loginForm && document.getElementById('Email')) {

loginForm.addEventListener( 'submit', passwordalert.saveGaiaPassword_, true); } else if (document.getElementById('hiddenEmail') && document.getElementsByName('password')){

  • document.getElementById("passwordNext").addEventListener('click',function (){

In my testing, we do not need the tempuser and temppass. The window.onbeforeunload handler (saveGaia2Password_) can still see document.getElementById('hiddenEmail') and document.getElementsByName('password'). This makes it so that we do not need to have the click handler and that onbeforeunload will handle all navigation, including pressing enter.

If I'm missing a case where the tempuser is needed, please let me know.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/password-alert/pull/107#discussion_r166421213, or mute the thread https://github.com/notifications/unsubscribe-auth/AGpBNSGWckSXypyNn4f7O5Gaq1e-woRAks5tSKttgaJpZM4R2Phv .

adhintz commented 6 years ago

@adhintz have you tried it with 2FA enabled? We've found that those elements are removed when the 2fa page is loaded.

Good point -- thanks for noticing that! Instead of using the temp variables, we can have the click event on Next call saveGaia2Password while the element still exist.

Rickerd0613 commented 6 years ago

Cool! Sounds like it should work to me. 👍

adhintz commented 6 years ago

I committed the discussed changes in https://github.com/google/password-alert/commit/6be180fa045932a511ef7d867020c3f86edb6e18 Please feel free to take a look.

There's also a new dogfood version of the extension with these changes at https://chrome.google.com/webstore/detail/password-alert/cdifbineneocgpdanggoehaloibgccll Please feel free to test it out.

Rickerd0613 commented 6 years ago

@adhintz Looks like everything is working! I tried it with both the consumer and enterprise modes and it correctly saved the hashes with 2FA enabled.

The one error I did have was when the enterprise mode goes to communicate with Google's app server when a password alert is triggered: bad client id: APP_ID_OR_ORIGIN_NOT_MATCH. I assume this is just because the dogfood version has a different extension id and this error should resolve itself once these changes are pulled mainline.

Thanks again for all your help on this!

adhintz commented 6 years ago

Thanks for testing the dogfood version!

bad client id: APP_ID_OR_ORIGIN_NOT_MATCH

Yes, that should go away with the production version. I'll release the new production version now.

adhintz commented 6 years ago

New version released and I've confirmed the error does not happen on prod when a password alert is triggered in enterprise mode.

Rickerd0613 commented 6 years ago

@adhintz We installed the new production version of the extension and almost everything seems to be working! One thing we did notice was that the password was not updated when a user was forced to change their password at next login. Is that a bug on our end or is the extension not checking for that?

Rickerd0613 commented 6 years ago

@adhintz Never mind, turns out it was our fault. Thanks again for all your help with this!

bazikov2403 commented 6 years ago

здраствуйте, как решить проблему с 2fа, пароль сгенерировался в почтовом ящикеЁ который дается в началеЁ а телефон обновилсяЁ тех поддержка не отвечает что можно сделать