Support encryption - Githubissues

Rayquaza01 / authenticator

Firefox addon that generates TOTPs for 2 factor authentication

https://addons.mozilla.org/en-US/firefox/addon/two-factor-authenticator/

MIT License

34 stars 7 forks source link

Support encryption #10

Closed Rayquaza01 closed 6 years ago

Max1Truc commented 6 years ago

@Rayquaza01 I added encryption with https://github.com/max1truc/crypt and hashing with https://caligatio.github.io/jsSHA/, my code is at https://github.com/Max1Truc/authenticator.

If you think I should add or modify something please answer me.

Rayquaza01 commented 6 years ago

@Max1Truc Hey, sorry for not answering sooner. Thank you for doing this for me. I've been busy with other things recently.

I believe I've read in a few places that SHA-1 is no longer safe for hashing passwords. I'm not too familiar with hashing, but I assume it would be safer to switch to a stronger version of SHA? Or maybe that isn't necessary given that the keys are not uploaded anyway.

Other than that, I think your changes seem fine and I'll merge them soon.

Max1Truc commented 6 years ago

@Rayquaza01 Hi, do you think I should use sha-256 instead of sha-1?

EDIT: I have just modified code to use SHA-256.

Max1Truc commented 6 years ago

Hey @Rayquaza01 Sorry about don't waiting for your answer, it was too tempting... Hum, so I think I should add salting for the hash of the master password with navigator.platform and navigator.hardwareConcurrency because these values are constants and specifics to user's platform even if much people will have, for example, Win64 and 4. It will increase a bit the difficulty to crack the hash because the attacker will have to know some more information about victim's PC. Do you think it's a good idea (I'm not very familiar with hashing, too) ?

Rayquaza01 commented 6 years ago

Salting would definitely be safer for hashing. I'm not sure about using navigator.platform and navigator.hardwareConcurrency for it though because if it's the same for most people it might not have much of an effect, though it couldn't hurt.

Max1Truc commented 6 years ago

Hi @Rayquaza01, I proposed you to use navigator.platform and navigator.hardwareConcurrency because these values won't change for the user even if he upgrades his system OS or modify Firefox settings. I say it because, for example, navigator.oscpu (Windows NT 10.0; Win64; x64 for me) contains the Windows version of user's platform. However, we can also use:

navigator.language for user's main language because if think user have no reason to change Firefox's language.
navigator.mediaDevices.enumerateDevices() for a list of user's microphone(s) and camera(s) names but user can connect a new webcam or change it and extension won't work.

Rayquaza01 commented 6 years ago

What are your thoughts on using the extension's UUID as the salt? It can be pulled from the result of runtime.getURL and it's different for each install.

Max1Truc commented 6 years ago

Good idea ! I think it's the best option even if it can be found in [Profile folder]/extensions.json because the UUID is hard to find in this file. I'll start coding it when #12 solved.

Max1Truc commented 6 years ago

Hi, I'm a bit late... I had not much time since the last time I coded.

So, I won't give Google Auth database import to this extension (#13) but Pull Request will be ready soon. I'll just change encryption library's version so that it is more secure for alpha-numerical strings.

Max1Truc commented 6 years ago

@Rayquaza01 Ok, #11 is ready for merging.

Rayquaza01 commented 6 years ago

@Max1Truc Ok, I merged #11 and made a couple of changes.

I changed the indentation to 4 spaces again and replaced == with ===. It was my fault for not including any instructions for code style in the repo before. I hope you don't mind, I just want to make sure everything is in a similar style, and it was all 4 spaces before. I've included a .eslintrc.json in the repo for future use.
I initialized fontColor and backgroundColor with values in authenticator-bg.js. In my opinion, having code like if (res.backgroundColor !== undefined) feels a bit hacky, and I'd rather make sure that backgroundColor and fontColor always have a value.
Made a function, generateElementsVariable, to create an object of elements from an array of ids. So instead of password.value, you would use DOM.password.value. This way, it's obvious which variables are elements on the page.
The password prompt autofocuses and you can submit the password by pressing Enter.
Added a keyboard shortcut (Alt+Shift+A) to open the popup.

Max1Truc commented 6 years ago

@Rayquaza01 Hi, I don't mind if you change the code. It's your extension ! Happy to know that my code is published, it's the biggest project I worked on for now.

Edit: I think you can close this issue, now.

Rayquaza01 commented 6 years ago

@Max1Truc Ok, I've submitted the new version to AMO. I'll close this when I get the confirmation that it was approved.

Edit: I got the confirmation. Closing issue.