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

Add an option to hide generated password to make it more secure #8

Closed
GoogleCodeExporter opened this Issue Jun 26, 2015 · 9 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter

GoogleCodeExporter commented Jun 26, 2015

I recently received the following enhancement request via e-mail: "Add an 
option to hide generated password to make it more secure in some sense."I 
wanted to make sure it was recorded here in the Issue log, even though I'm not 
sure yet whether or not I'll implement it.

Here's an excerpt from my response to the requester, which details the primary 
reasoning on why it was not immediately accepted:

I can see the security advantages in this, but bear in mind that Cryptnos does 
not have a concept of whether or not a generated password is "correct" since 
the generated password is never stored in any form. You can even use the same 
set of parameters (site token, hash algorithm, hash iterations, etc.) with a 
different master password to create a "family" of passwords.  For example, I 
use this tactic with my GoToMyPC account to generate not only the main password 
for my account but a unique password for each machine.

This becomes a problem, however, if you mistype your master password. If the 
generate password is never displayed, you may never notice that it was 
generated incorrectly.  While it's hard to memorize a Cryptnos generated 
password, I know I personally visually scan passwords I use often and I can 
quickly see when I've typed the master password incorrectly.  You could easily 
lock yourself out of your accounts by pasting the wrong password into the login 
form too many times.

It should be noted that Cryptnos for Windows 1.3.1 (which as of this writing 
has not yet been released) will include an enhancement that effectively 
provides similar functionality as this request. This is obviously a more 
pressing issue on a desktop or notebook computer than on a tablet or smartphone 
as the latter devices are more personal and easier to protect from casual 
observation.

Original issue reported on code.google.com by jeff.darlington@gmail.com on 21 Mar 2012 at 4:42

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

The new feature implemented in 1.3.1 perfectly resolves my concern. Thank you!

Original comment by hyptris...@gmail.com on 22 Mar 2012 at 8:39

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

Just for the sake of clarity, I wanted to point out that the new functionality 
mentioned in my last paragraph above is going into Cryptnos for *Windows* 
1.3.1, not Cryptnos for Android. I created (and immediately closed) a ticket 
identical to this one in the Cryptnos for Windows issue tracker, stating that 
it's a duplicate of the new functionality I'll be adding. This issue here 
hasn't been closed because there's no comparable functionality planned for the 
Android client (yet). I didn't want you to accidentally confuse the two.

That said, I might have a way to replicate the Windows client solution, at 
least in part. Android Activities (i.e. "pages" or "screens") have life cycle 
events, much like other application frameworks. You can read all the gory 
details here, if you wish:

https://developer.android.com/guide/topics/fundamentals/activities.html

However, what we'd be most interested in here is the "foreground lifetime", 
which occurs between calls to onResume() and onPause(). onPause() is called 
when the application loses the foreground focus, usually when you're switching 
to another activity (i.e. screen with the same application or moving to a 
totally different application); onResume() is called when you return to this 
activity. During this time, the activity and all its assets may continue to 
exist, or they may be wiped from memory to make way for another application in 
the foreground. That's controlled by the operating system, not the application, 
so there's no way to know when your app might be unloaded from active memory. 
However, I did a little multitasking test and confirmed that you can regenerate 
a Cryptnos password, switch to a different app, then switch back and see the 
passwords still on the page. (Personally, I usually Back out of Cryptnos once 
the password is in the clipboard, but I know not everyone follows that usage 
pattern.)

The "clear passwords on losing focus" functionality added in Cryptnos for 
Windows 1.3.1 should be easy to replicate by adding code to onPause() to clear 
those text boxes as the Add/Edit or Regenerate activity screen loses focus. It 
could even be added as an optional feature, much like the Windows client; I'd 
just need to add a checkbox to the Settings activity and check for its value at 
the right place. Personally, I'm inclined *not* to make it an optional setting 
and just make it the new default functionality, as it's certainly more secure. 
Then again, that might be a problem for people who depend on that ability to 
multitask as described above, so an option would be a better choice.

The caveat with this approach is that Android handles events like changing 
orientation (that is, rotating the device) by destroying then recreating the 
activity in the new layout. This new option would effectively clear the 
password boxes whenever you rotate the screen, which could be annoying. It's 
possible to catch these events and handle them accordingly (temporarily store 
and restore the passwords during rotate but clear them when losing focus), but 
it's one of those "gotchas" that Android developers have to watch for.

There's another alternative that I could also explore, but it's a bit more 
invasive and I'm less enamored with it. I could add a checkbox to the Settings 
activity to completely hide the generated password text box... *IF AND ONLY IF* 
the "copy generated passwords to clipboard" *AND* "show master passwords" 
checkboxes are also checked. If either of these checkboxes are unchecked, the 
new checkbox would become unchecked and disabled. Thus, you'd only be able to 
hide the generated password from view if the other two options are enabled.

The reasoning for requiring all three options would be what I described above; 
if you can't tell that you typed the master password correctly, there's no way 
to know if the generated password is correct. If you can see the master 
password, then you'd know the generated password was created successfully, even 
if you can't see it. However, if we hide the generated password box, there's no 
way to access its contents. That's where the "copy to clipboard" option comes 
in. Showing the master password and copying the generated password to the 
clipboard solves both problems without ever displaying the generated password 
to the screen.

The caveat here, of course, is that the master password is visible to anyone 
looking over your shoulder, and its easy to argue that revealing the master 
password is a far greater risk than an individual generated one. (I didn't want 
to add the option to reveal the master passwords myself, but there was a 
compelling argument for it when the request was made.)

I'm more inclined to go with the first option (clearing the passwords when 
losing focus), in part because it's easier to implement and it's also more in 
line with the approach the Windows client took for the same problem. Does 
anyone have any additional thoughts to share on this one?

Original comment by jeff.darlington@gmail.com on 27 Mar 2012 at 4:24

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

OK, just a quick note to say that onPause() is not the place to put this; the 
state-saving code gets called after onStop(), so clearing the boxes in 
onPause() clears them no matter what. However, I think I can put a flag in the 
state saving object to say "don't clear the boxes", then default the flag to 
"clear the boxes" unless that flag resets it. An initial test seems to work; 
rotating the device successfully preserved the passwords in the boxes, but 
multitasking to another app then returning to Cryptnos successfully cleared the 
boxes.

I need to do more testing, of course, and I haven't implemented a preference in 
the Settings activity yet. That said, it looks like this should be doable. I'm 
going to go ahead and mark this as accepted and slate it for the 1.3.1 release.

Original comment by jeff.darlington@gmail.com on 27 Mar 2012 at 8:37

  • Changed state: Accepted
  • Added labels: Milestone-1.3.1
@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

This seems to be shaping up nicely. For the morbidly curious, the final 
solution will be to have an option in the Settings activity to let the user 
choose whether or not passwords will get cleared when Cryptnos loses focus 
(that is, goes into the background, such as when you launch another app). By 
default, the setting will be turned off, which replicates the current behavior. 
If Cryptnos goes into the background while the user is in either the Add, Edit, 
or Regenerate activities and they then return, the user's preference is queried 
and the password boxes are cleared accordingly. This takes place in the 
onResume() life cycle event, rather than onPause(), onStop(), or any of the 
other events.

If a configuration change occurs (the screen is rotated, the physical keyboard 
is slide out, etc.), the activity is destroyed and recreated as described 
above. However, when the state of the UI is restored, the user's preference 
here is overridden and the password boxes are *NOT* cleared. Thus you should be 
able to rotate the screen or slide in and out a physical keyboard without 
losing the passwords. Once the screen has been restored, the user's preference 
is reinstated so it will act as expected if Cryptnos goes back into the 
background.

I've got this working well in the Regenerate activity, so all I need to do is 
propagate it to the Add/Edit activity. (These are actually the same form, only 
the Site Token box is disabled in Edit mode.) Then it's just a matter of 
updating the Upgrade Manager and help documentation.

Original comment by jeff.darlington@gmail.com on 29 Mar 2012 at 4:02

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

Implemented with Revision 64

Original comment by jeff.darlington@gmail.com on 29 Mar 2012 at 8:33

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

OK, note to self: this one isn't complete yet. While the code is technically 
working, the setting isn't getting saved. When you exit the app and wait long 
enough for the OS to reclaim its resources, the setting gets cleared. Confirmed 
on both Gingerbread and ICS. I probably missed something when I copied code 
from another setting's methods. I'm still working on some code clean-up and 
Lint suggestions, so 1.3.1 isn't ready yet anyway, but this definitely needs 
fixed before release.

Original comment by jeff.darlington@gmail.com on 31 Mar 2012 at 3:15

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

OK, found the problem. All the code to set the flag is in place, I just forgot 
to add a step to read the flag from the preferences file when the app starts. 
Oops. My dev machine is powered down right now so I have to leave myself a note 
on what to fix for now.

Original comment by jeff.darlington@gmail.com on 31 Mar 2012 at 9:08

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

Should be fixed with Revision 69.

Original comment by jeff.darlington@gmail.com on 31 Mar 2012 at 10:15

@GoogleCodeExporter

This comment has been minimized.

GoogleCodeExporter commented Jun 26, 2015

Released with Version 1.3.1

Original comment by jeff.darlington@gmail.com on 16 Apr 2012 at 2:41

  • Changed state: Fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment