Added TOTP verification to change master password

[ssddOnTop]

This adds additional layer of security to change master password.

[ssddOnTop]

NOTE: additional unit tests are required to test and the test_handle_update_master_password() should fail

[SonuBardai]

One thing I noticed during the little bit i tried to use this is: I'm able to update the master password from the CLI: cargo run -- update-master --master password --new-master newpassword --file-name totp-test-store. I understand that this issue might be arising from the code not being DRY enough, although the quick fix here would just be to add another check somewhere in the CLI handler: https://github.com/SonuBardai/lockbox/blob/e9c7ee0d6b0eb20256c68f9fc9526ab5b70b2a93/src/cli/mod.rs#L154-L182

[ssddOnTop]

Yes I was little confused between the methods, I thought there is some similar method but I didn't knew where to add.

There is not much of documentation so I just tested it with repl, I haven't tried it with arguments

I will update it soon, but can you please work on test_handle_update_master_password() till then

[ssddOnTop]

Also I think this is not a good practice to take passwords from cli directly, you should accept some file path that contains password, else the password will be exposed to everyone, then can simply look up for password in history

[SonuBardai]

Also I think this is not a good practice to take passwords from cli directly, you should accept some file path that contains password, else the password will be exposed to everyone, then can simply look up for password in history

Sounds fair. Thanks for the suggestion, I've made an issue for this. https://github.com/SonuBardai/lockbox/issues/82

[ssddOnTop]

@SonuBardai I am little confused.

I've fixed all tests and directly added TOTP verification to update_master_password() in cli/commands.rs but after first run, if you try to change password from arguments then it automatically updates password before running the verification code, can you explain the code flow in that case

[ssddOnTop]

let master =
                master.unwrap_or_else(|| read_hidden_input("master password", prompt_password));
            let new_master = new_master
                .unwrap_or_else(|| read_hidden_input("new master password", prompt_password));

Isn't there any password verification going on here? (in cli/mod.rs, Command::UpdateMaster)

[SonuBardai]

The password store exposes an API, ie. a bunch of methods that can be used to interact with the password store (load, store, push, pop, etc.) The commands in cli/commands.rs are used to call these API functions and perform any console writes, password generation, etc. The CLI/REPL are going to initialize a password store and use the commands to provide the UIs. That's how I intended to design this.

[ssddOnTop]

let master =
                master.unwrap_or_else(|| read_hidden_input("master password", prompt_password));
            let new_master = new_master
                .unwrap_or_else(|| read_hidden_input("new master password", prompt_password));

Isn't there any password verification going on here? (in cli/mod.rs, Command::UpdateMaster)

@SonuBardai can you look into this

[SonuBardai]

Isn't there any password verification going on here? (in cli/mod.rs, Command::UpdateMaster)

Master password verification is done in the password store initialization

https://github.com/SonuBardai/lockbox/blob/e9c7ee0d6b0eb20256c68f9fc9526ab5b70b2a93/src/cli/mod.rs#L165

[ssddOnTop]

Isn't there any password verification going on here? (in cli/mod.rs, Command::UpdateMaster)

Master password verification is done in the password store initialization

https://github.com/SonuBardai/lockbox/blob/e9c7ee0d6b0eb20256c68f9fc9526ab5b70b2a93/src/cli/mod.rs#L165

Looks like there is no password verification on PasswordStore::new however I was able to do that with load() function

[SonuBardai]

Wait scratch that.. yeah you're right. That's missing the verification. We're missing the load() call to the store that does the verification

[SonuBardai]

We're doing the load() inside the update_master_password function though, so that's what's handling verification. I think this can be moved up to catch invalid passwords early 👍 Great find @ssddOnTop. https://github.com/SonuBardai/lockbox/blob/e9c7ee0d6b0eb20256c68f9fc9526ab5b70b2a93/src/cli/commands.rs#L149-L155

[ssddOnTop]

@SonuBardai test_run_cli needs a fix, I don't know what's wrong, I don't think there's an issue from my side

[codecov-commenter]

Codecov Report

Merging #81 (bda2025) into 2fa (e9c7ee0) will decrease coverage by 1.68%. The diff coverage is 87.09%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##              2fa      #81      +/-   ##
==========================================
- Coverage   92.69%   91.01%   -1.68%     
==========================================
  Files           9        9              
  Lines        1587     1681      +94     
==========================================
+ Hits         1471     1530      +59     
- Misses        116      151      +35     

Files	Coverage Δ
src/cli/io.rs	96.94% <ø> (ø)
src/store/mod.rs	99.13% <100.00%> (+0.04%)	:arrow_up:
src/crypto/mod.rs	92.64% <87.50%> (-7.36%)	:arrow_down:
src/repl/mod.rs	92.38% <87.71%> (+0.36%)	:arrow_up:
src/cli/commands.rs	88.26% <79.24%> (-3.76%)	:arrow_down:
src/cli/mod.rs	77.94% <72.34%> (-9.87%)	:arrow_down:

[ssddOnTop]

@SonuBardai Well I think the users should be guided to some auth app (one can make a readme file containing various totp apps such as google/microsoft auth, etc).

And

• What if I lose the code?

Well for that I have already planned of giving backup codes, we can give a list of backup codes, and each backup code can only be used once.

I am bad with such documentation, I would need external help to guide/explain users the functionality of the code.

And I think this isn't sufficient AT ALL. As it's just taking master password, hasing with sha256 and using that hash as secret key.

If master pw is compromised already, then attacker can hash the pw himself and bypass TOTP as well.

For that I think it will be good idea to ask for a separate password for TOTP. The password will be asked only once.

This will also help existing to migrate to TOTP.

BUT

Storing the TOTP password will be a HUGE issue. Currently you store AES key and IV as first few bytes in the file, but we can NOT store it as a part of it, as if attacker remove those password bytes then there will be no way to verify TOTP.

Solution

1(Not so secure). As mentioned in #12 manage permissions of file.

2. Each OS comes with some secure way to store passwords for e.g. Window's Credential manager, MacOS's Keychain, GNOME's Keyring, etc.

So (I think) you need admin permission to store/read/delete the password.

3. I think TOTP should be mandatory to access the passwords along with master password, that will make the application almost completely secure IMO.

[ssddOnTop]

@SonuBardai I still suggest to use something better for encryption, storing a few bytes (of IV and password) in the beginning of the store file isn't secure at all, you need to come up with something better

[SonuBardai]

I still suggest to use something better for encryption, storing a few bytes (of IV and password) in the beginning of the store file isn't secure at all, you need to come up with something better

Do you have any suggestions? Given that we want the password manager to be functional even in a completely localized environment, one solution I'm able to think of is to set a lifetime for the master password and force the user to update it. Having a strong 2fa alongside this would be ideal, and this PR is a great step towards that

[ssddOnTop]

Do you have any suggestions? Given that we want the password manager to be functional even in a completely localized environment, one solution I'm able to think of is to set a lifetime for the master password and force the user to update it. Having a strong 2fa alongside this would be ideal, and this PR is a great step towards that

No I think forcing user to update password regularly isn't bad idea but storing 28 bytes in front isn't a great of an idea. Anyone would be able to remove the 28 bytes or even 1 byte and that will completely lock out the user.

We somehow need admin access, to store the file in read-only mode.

Maybe we can store the password in some secure folder with read only rights in.. say /etc/.lockbox and we can store the encrypted password on several locations in the pc with some error correction methods like hamming/raptor codes (but even this idea is bad)

The only secure way I'm able to think of right now is, run a "server" as service and make client for cli, all the passwords will be managed by server, you use the cli app to just generate and show the passwords (i.e. interact with the user)

Server will have root access so the passwords can be managed efficiently. (We can even setup custom PKI for secure communication b/w server and client)