Last Comment Bug 592772 - Fennec should offer to use master password
: Fennec should offer to use master password
Status: VERIFIED FIXED
[sec-assigned:yvan]
: uiwanted
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Alex Pakhotin (:alexp)
:
Mentors:
: 540769 (view as bug list)
Depends on: 686387 751361 624552 624570 658691 679331 697980 697991 698368 698396 698444
Blocks: 678412 690754 657744
  Show dependency treegraph
 
Reported: 2010-09-01 11:21 PDT by Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
Modified: 2013-12-19 10:15 PST (History)
31 users (show)
dveditz: sec‑review? (yvanboily+mozbugmail)
mozaakash: in‑testsuite?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wip-0.1 (12.69 KB, patch)
2010-09-01 11:21 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
screenshot 1 (55.23 KB, image/png)
2010-09-01 11:23 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details
screenshot 2 (50.31 KB, image/png)
2010-09-01 11:24 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details
screenshot 3 (49.13 KB, image/png)
2010-09-01 11:24 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details
Patch (15.07 KB, patch)
2010-09-02 03:54 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Splinter Review
screenshot - first time (31.98 KB, image/png)
2010-09-20 08:09 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details
unlock addition to master-password-question.png (1.15 MB, image/png)
2010-12-23 22:55 PST, silverwav
no flags Details
Patch - updated on the trunk (12.77 KB, patch)
2010-12-27 07:57 PST, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
mark.finkle: review-
Details | Diff | Splinter Review
Clear Master Password (61.95 KB, image/png)
2011-01-08 03:54 PST, silverwav
no flags Details
[WIP] Patch (33.23 KB, patch)
2011-05-16 19:41 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Patch v2 (38.20 KB, patch)
2011-05-20 13:51 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Patch v3 (28.05 KB, patch)
2011-05-20 18:59 PDT, Alex Pakhotin (:alexp)
dougt: review-
Details | Diff | Splinter Review
updated dialog patch (14.43 KB, patch)
2011-06-22 13:36 PDT, Mark Finkle (:mfinkle) (use needinfo?)
wjohnston2000: review+
Details | Diff | Splinter Review
Patch v4 (38.89 KB, patch)
2011-06-28 19:53 PDT, Alex Pakhotin (:alexp)
blassey.bugs: review+
wjohnston2000: review-
Details | Diff | Splinter Review
Patch v5 (39.21 KB, patch)
2011-07-07 18:49 PDT, Alex Pakhotin (:alexp)
mark.finkle: review+
Details | Diff | Splinter Review
Patch v6 [WIP] (36.66 KB, patch)
2011-07-16 00:24 PDT, Alex Pakhotin (:alexp)
no flags Details | Diff | Splinter Review
Patch v7 (40.01 KB, patch)
2011-07-19 01:15 PDT, Alex Pakhotin (:alexp)
blassey.bugs: review+
brian: review-
mark.finkle: review+
Details | Diff | Splinter Review
UI-only patch (15.16 KB, patch)
2011-08-11 17:10 PDT, Alex Pakhotin (:alexp)
wjohnston2000: review+
Details | Diff | Splinter Review
UI-only patch v2 (15.22 KB, patch)
2011-08-15 17:27 PDT, Alex Pakhotin (:alexp)
alex.mozilla: review+
Details | Diff | Splinter Review

Description Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-01 11:21:13 PDT
Created attachment 471180 [details] [diff] [review]
wip-0.1

It still misses some UI love but it does pretty much all we want.
Comment 1 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-01 11:23:53 PDT
Created attachment 471183 [details]
screenshot 1
Comment 2 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-01 11:24:18 PDT
Created attachment 471184 [details]
screenshot 2
Comment 3 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-01 11:24:41 PDT
Created attachment 471185 [details]
screenshot 3
Comment 4 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-02 03:54:42 PDT
Created attachment 471456 [details] [diff] [review]
Patch

The patch implements the screenshot which are a very simple UI model:
 - a UI to set up a password
 - a UI to remove a password

There is not UI for changing the password, if the user want to he could follow this step to reproduce:
 - delete it
 - set up a new password

IMO the master password is not changed each day so there is not reason for doing a much complex UI.
Comment 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-02 18:35:33 PDT
Matt, I've seen your name in this project here:
https://wiki.mozilla.org/Mobile/Projects/MasterPassword

Do you have any suggestions or feedback?
Comment 6 Matt Brubeck (:mbrubeck) 2010-09-02 19:44:10 PDT
Looks good to me!  I'll put your name on that wiki page instead - I hadn't started working on this at all.
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-20 08:09:29 PDT
Created attachment 476808 [details]
screenshot - first time
Comment 8 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-20 08:10:03 PDT
My recommendation would be to delay use of the master password until we can fold it into some much better UI around user identity. It will only increase your testing surface, and require you to pay attention to user experiences like requests for passwords in background tabs causing master password dialogs to appear at odd times or in places that lock up the UI, etc.I'm not confident there's a lot of value to this for this release, and would recommend waiting until some later release.The screenshots shown look fine, but there's bits missing: - this presumes that someone using master password on desktop wants it on their sync'd device as well, which I'm not sure is true - what's the experience like the first time the master password is required? how long does that "unlock" last, especially in a world where the user might leave Firefox running for days in the background?
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-20 08:14:14 PDT
I am not sure why all the linefeeds were removed from my previous comment :/

The first time screenshot looks fine, although I hate that string (it's the same as we use in the desktop version, doesn't make it any better).

Still need to answer the "does master password on desktop mean always use on other devices" question. I don't think it does, but perhaps that's a sync question more than anything else.
Comment 10 Zandr Milewski [:zandr] 2010-09-23 09:58:10 PDT
There are some unpleasant interactions with sync that make this a broader conversation (where?).

Since you cannot control sync engines on a per-device basis now[1], having Fennec sync without any capability for a master password means that either:

1) your passwords will be copied to your mobile device and stored in the clear
or
2) You cannot use sync for passwords on any device.

It means for the short term, we're depending on the device lock to protect the users passwords. I'm not terribly happy with that, but others might be.

[1] Not that it makes much difference: If someone stole your phone, they could turn on password sync. It would require connecting again, but that's probably not a huge risk for the attacker.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2010-09-28 20:12:44 PDT
We would take a patch that handles master password in Fennec 2.0, but it seems we need more thought about this, especially relative to Sync. I'm not sure who is going to do the research and provide insights so we can't block Fennec 2.0 on this.
Comment 12 silverwav 2010-10-10 15:03:33 PDT
Sorry for the spam but this is important.

Please allow us the option of using a master password, make the default "off" by all means, but empower your users with the ability to control their security and privacy.

I have my whole internet life in those passwords and deliberately chose to protect them on the desktop, this beta1 just blew a huge whole in my security.

At a minimum you need to advise users of the security implications so they can make an informed choice and not get carried away with the coolness of the beta.
Comment 13 Michael Coates [:mcoates] (acct no longer active) 2010-11-10 14:05:18 PST
I'd like to reopen the discussion on this issue.  I understand we may not be willing to block fennec for the master password. However, I think we do need to carefully make a few design decisions in the interest of security.

Current Sync Deployment:
1. Any sync'ed passwords (from desktops/other devices) will be sync'ed to the mobile
2. Passwords on the mobile are stored in clear text
3. A phone without builtin encryption (e.g. most of them) provides zero defense from an attacker inspecting and removing the passwords (either via running script or forensic inspection)
4. Mobile phones are much more likely to be lost or stolen then laptop/desktop computers which increases the risk of unencrypted stored passwords.

Possible Solutions
1. Master password (as discussed)
2. Default to not sync passwords to mobile devices at all
3. Idea #2 by default plus user option to enable password sync with a huge warning message on the risk they're about to accept

We need to adopt some sort of solution to mitigate this risk before we launch fennec with sync. Otherwise our users will be caught of guard by the large security risk they've unknowingly assumed.I also fear we will receive significant backlash from media/security/privacy groups.  Mobile banking apps are already receiving bad press for poor security practices on mobile devices and hackers are definitely targeting mobile devices.


Banks Rush to Fix Security Flaws in Wireless Apps
http://online.wsj.com/article/SB10001424052748703805704575594581203248658.html?mod=WSJ_Tech_LEFTTopNews

Hackers take control of 1 million mobile phones
http://www.shanghaidaily.com/article/?id=454047&type=National#ixzz14uwGtrGE
Comment 14 Matt Brubeck (:mbrubeck) 2010-12-03 11:25:01 PST
*** Bug 540769 has been marked as a duplicate of this bug. ***
Comment 15 Mike Connor [:mconnor] 2010-12-14 11:19:24 PST
For those following this bug, we had Sync/Mobile/Sec/Infrasec people meet to discuss this, and concluded that we want to explore better UX and security here, but that it will not block Fx4 for mobile.
Comment 16 John Hogenmiller 2010-12-22 19:50:33 PST
Does anyone else feel that Mozilla should tone down their "safe" blurb on the main page?   http://www.mozilla.com/en-US/mobile/sync/

--
Enjoy the benefits of sharing personal info across all your devices will still maintaining military-grade security and privacy.
--

Under the current deployment, your encrypted passwords on your desktop/laptop would be synced to the mobile device and stored in an unencrypted format.

Unless of course, this bug has been fixed and just not updated?
Comment 17 silverwav 2010-12-23 22:55:53 PST
Created attachment 499633 [details]
unlock addition to master-password-question.png

>how long does that "unlock" last, especially in a world where the user might leave Firefox running for days in the background?

How about:

Ask again in; 1 Hour; 2 Hours; 3 Hours; 4 Hours; 1 Day.
Comment 18 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-12-27 07:16:28 PST
I think adding duration for asking the master password again is an other bug, this one is about adding a master password UI to mobile-browser.
Feel free to open a new one depending on this bug, or to create an addon to experiment with the provided concept of master password "session"
Comment 19 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-12-27 07:57:02 PST
Created attachment 499830 [details] [diff] [review]
Patch - updated on the trunk

The patch is updated to work on trunk, the UI is the same as the previous screenshot except that the style is the new one.
Comment 20 Matt Brubeck (:mbrubeck) 2011-01-07 11:16:15 PST
I took Vivien's patch and made it into an add-on, with essentially no modifications:
https://addons.mozilla.org/en-US/mobile/addon/270907/

Please try out this add-on if you would like to have Master Password support.  Any feedback or bug reports on the add-on will help when we incorporate the feature into the browser itself.
Comment 21 silverwav 2011-01-07 12:15:18 PST
(In reply to comment #20)
> I took Vivien's patch and made it into an add-on, 

All working fine on initial testing so far.

Note Firefox Start:

If I tap the greyed out "Tabs from your other computers" I should be asked for my Master Password.

Workaround go to site that need a pw.

Cheers and thanks for the fix :-)
Comment 22 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2011-01-07 12:29:52 PST
One piece of feedback: it'd be nice to have a means to re-lock your master password, at least on Android, since there's no explicit way to quit Firefox there.

Once I've typed in my master password, my passwords become unlocked and stay unlocked as long as Fennec continues running in the background (until I turn off my phone, potentially).

This kind of breaks the point of having a master password, because as long as I've unlocked my master password sometime in the last $UNIT_OF_TIME*, a phone thief will have access to my unlocked profile and there's nothing I can do about it.

($UNIT_OF_TIME = however long it takes for Fennect to quit on its own)
Comment 23 John Hogenmiller 2011-01-07 15:49:18 PST
While having a master password timeout would be a nice feature, it's not essential.  Just like the desktop's lock screen, you would want to utilize your phone's lock feature requiring either a pin or pattern in order to unlock it.

Remember, the desktop version doesn't have a master password timeout either, though there are extensions to implement that feature.  On my desktop, I leave Firefox running for 3-4 days at a time, just locking the desktop when I leave my desk.

So the only thing we'd have to be careful of is if we left the phone on a media or car dock that keeps the screen from locking in the first place.
Comment 24 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2011-01-07 16:56:02 PST
> While having a master password timeout

I wasn't asking for a timeout -- I was asking for a means to manually re-lock your master-password-protected data.

> Remember, the desktop version doesn't have a master password timeout either,

Right -- (I'm not asking for a timeout) -- but on the desktop, you can quit Firefox and be secure in the knowledge that your master-password-protected data is safe.  On the phone, there's no such ability right now.

> Just like the desktop's lock screen, you would want to utilize your
> phone's lock feature requiring either a pin or pattern

#1, unlock patterns tend to be easy to guess by looking for smudges on the phone screen.  I wouldn't trust my bank passwords to my phone's unlock pattern.

#2, the phone's lock feature might be a good layer of defense for fennec users in general -- not just master-password users.  However, users who elect to use a master password presumably want an additional layer of protection *beyond* the lock screen.
Comment 25 silverwav 2011-01-08 03:54:53 PST
Created attachment 502216 [details]
Clear Master Password

(In reply to comment #22)
> One piece of feedback: it'd be nice to have a means to re-lock your master
> password,

How about a setting "Clear Master Password".
Comment 26 Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) 2011-02-26 12:21:53 PST
Exactly - that's about what I had in mind.
Comment 27 Stuart Parmenter 2011-02-26 13:23:38 PST
What I would like to see us do is just (always?) auto-generate a master password and add it to the Android system wide key store which holds things like Wifi passwords.  Then ask for it from there as necessary and use it in place of the user needing to type in to our master password dialog
Comment 28 Mark Finkle (:mfinkle) (use needinfo?) 2011-02-26 14:35:20 PST
(In reply to comment #27)
> What I would like to see us do is just (always?) auto-generate a master
> password and add it to the Android system wide key store which holds things
> like Wifi passwords.  Then ask for it from there as necessary and use it in
> place of the user needing to type in to our master password dialog

Kinda like bug 496660 would do for Mac?
Comment 29 Mike Connor [:mconnor] 2011-02-27 16:21:08 PST
IIRC, the current MP impl makes that hard, but I know dolske had some ideas on how we could do that.
Comment 30 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-11 22:24:49 PDT
Comment on attachment 499830 [details] [diff] [review]
Patch - updated on the trunk

r- because I don't think we really want to use this approach. It might be better to try to integrate the master password (or all passwrods) into the device password storage, like Firefox wants to do on Mac OS keyring, maybe Gnome keyring too.
Comment 31 Patrick 2011-04-11 22:47:00 PDT
I think, now that Fennec is no longer beta, Mozilla should inform all users ASAP, that all their passwords (even those that they never entered on their mobile device - copied to the mobile device via Sync) are stored in clear text on the mobile device, when they do not use the Master password extension.
Comment 32 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-13 12:56:46 PDT
The latest design for this feature is to auto-generate a master password and store the password in the Android password system. We need to determine how the edge cases work, but in general, we'd like to keep the master password hidden.
Comment 33 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-05-12 07:50:50 PDT
Comment on attachment 471185 [details]
screenshot 3

Removing ui-review since there is a new approach.

Alon, 
do you want to take this bug?
Comment 34 Alon Zakai (:azakai) 2011-05-12 09:20:41 PDT
I'm not sure how we would save the master password. A quick look through the API doesn't show anything that is meant for saving passwords, and I assume everything else is not secured,

http://developer.android.com/guide/topics/data/data-storage.html

Other apps that do similar things appear to require the master password to be entered every time, like

http://code.google.com/p/secrets-for-android/

Are we ok with storing the password in non-secured storage, like SharedPreferences?
Comment 35 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-12 21:22:37 PDT
(In reply to comment #34)
> I'm not sure how we would save the master password. A quick look through the
> API doesn't show anything that is meant for saving passwords, and I assume
> everything else is not secured,

Hmm. I guess we assumed there would be a password manager in the OS. Doesn't seem to be the case.

> Other apps that do similar things appear to require the master password to
> be entered every time, like

We would like to avoid this, if possible

> Are we ok with storing the password in non-secured storage, like
> SharedPreferences?

SharedPreferences is sandboxed away from other apps. If the phone is rooted and someone gets your phone, the data could be accessed - but you have lost completely if someone gets your rooted phone.

SharedPreferences has the benefit of always being sandboxed, even when the app + profile is moved to the sdcard. Let's get some feedback from other people about this idea:

Use SharedPreferences to store an auto-generated master password. Use the master password to auto-login into Mozilla's master password system on launch. Mozilla should then encrypt the password file in the profile, no matter where it is stored (main memory or sdcard). If the sdcard is lost, the passwords are still encrypted.

Thoughts?
Comment 36 Patrick 2011-05-12 21:39:42 PDT
(In reply to comment #35)
> Hmm. I guess we assumed there would be a password manager in the OS. Doesn't
> seem to be the case.

What's wrong with the Android credential storage?
Comment 37 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-12 22:01:54 PDT
(In reply to comment #36)
> (In reply to comment #35)
> > Hmm. I guess we assumed there would be a password manager in the OS. Doesn't
> > seem to be the case.
> 
> What's wrong with the Android credential storage?

If you mean Account Manager, yes - that's a better choice:
http://developer.android.com/reference/android/accounts/AccountManager.html
Comment 38 Alex Pakhotin (:alexp) 2011-05-12 23:01:38 PDT
Hmm, I was sure I already took this bug, apparently not.

(In reply to comment #37)
> If you mean Account Manager, yes - that's a better choice:
> http://developer.android.com/reference/android/accounts/AccountManager.html

Yes, this is what we'll probably use. Brad already mentioned this in our chat today.

I had an idea of using ANDROID_ID as a password for encryption (it's unique for the device and changes on a factory reset), but apparently this is not safe enough. Even though it doesn't seem to be directly accessible on a device locked with a password, it should be possible to install an app remotely, and get this ID.

Another idea was to use the KeyStore, but that itself requires a password.

I'll try AccountManager tomorrow, and if it will work, may submit a WIP patch.
Comment 39 Alex Pakhotin (:alexp) 2011-05-16 19:41:32 PDT
Created attachment 532843 [details] [diff] [review]
[WIP] Patch

Silently generate and use master password.

This version stores the password in the system using Android AccountManager. Unfortunately this approach doesn't seem to work for us. According to the documentation the caller is required "to have the same UID as the account's authenticator". But in my test I could get access to the stored password using a separate application. It looks like that UID is not actually a unique ID of the app. I have to do more research on this.
Comment 40 Alex Pakhotin (:alexp) 2011-05-18 16:07:27 PDT
After more research I still don't see a way to store the password so that it cannot be accessed by other applications. Account Manager checks the UID only when the app connects to the authenticator service, and if the service is implemented in the same app, the check succeeds.

But even if that check would work, it wouldn't really help on a rooted device. The password is stored by the Account Manager as a plain text in a regular database. Other authenticators (for example for Google accounts) do not store the password itself, but rather an authentication token. Still those tokens are easily accessible if one has root access. I could just copy the database from my rooted Nexus One, and get all those tokens. I believe it's possible to use those tokens in a properly formed request from another application to Google server, and get access to the account data.

So if I'm not mistaken, there seems no way to secure data completely on a rooted phone. If we target only non-rooted phones, the password could be stored just in a local file normally accessible only by the application.

Anyone has any thoughts?
Comment 41 Richard Soderberg [:atoll] 2011-05-18 16:32:03 PDT
ConnectBot handles SSH key passwords by asking the user to enter the key password, and then caching it in memory.  As long as an SSH is running, the key is cached.  Once all SSH sessions are disconnected, the application permits the OS to background kill it as needed.  Once a background kill occurs, the key must be re-entered.

This results in entering my password a bit more often than is desirable, but is also the only secure option I am aware of at this time.
Comment 42 Alex Pakhotin (:alexp) 2011-05-18 16:45:41 PDT
(In reply to comment #41)
> ConnectBot handles SSH key passwords by asking the user to enter the key
> password, and then caching it in memory.  As long as an SSH is running, the
> key is cached.  Once all SSH sessions are disconnected, the application
> permits the OS to background kill it as needed.  Once a background kill
> occurs, the key must be re-entered.
> 
> This results in entering my password a bit more often than is desirable, but
> is also the only secure option I am aware of at this time.

Actually the master password in Firefox uses a similar approach. It's been asked only once, and stays active while the application is running. The main problem, as far as I understand, is the proper UI design and implementation, for example in a situation when several tabs at the same time require to enter a password. Another problem is related to the lifetime of the embedded password manager. Fennec frequently closes when in background, so the password will need to be re-entered on the next start.

I was thinking about a solution with an Android service, which would ask the user to enter a password, and cache it. The service should keep running separately from the Fennec process.
Comment 43 Richard Soderberg [:atoll] 2011-05-18 16:52:11 PDT
Additional bonus to using a service is that you can set a master password timeout after N seconds of inactivity, so that it expires if the user goes idle long enough, without necessarily needing Firefox to be running.
Comment 44 Alex Pakhotin (:alexp) 2011-05-18 16:53:39 PDT
(In reply to comment #43)
> Additional bonus to using a service is that you can set a master password
> timeout after N seconds of inactivity, so that it expires if the user goes
> idle long enough, without necessarily needing Firefox to be running.

Exactly!
I just forgot to mention this. :)
Comment 45 Brad Lassey [:blassey] (use needinfo?) 2011-05-18 20:03:22 PDT
I think the goal here is for the user to never actually have to enter a password. Instead, simply authenticating themselves to their device (in the form of a lock screen) should be sufficient.

What are the permissions of the file that the Account Manager holds the passwords in? I don't think solving this issue for rooted devices is required. However, simply keeping the key in a file with user read only perms isn't sufficient because any one with physical access to the device can read it it with "adb shell run-as org.mozilla.fennec cat <file>" If the Account Manager's db is not readable on a non-rooted device, I think that will be sufficient.
Comment 46 Alex Pakhotin (:alexp) 2011-05-18 23:23:32 PDT
> What are the permissions of the file that the Account Manager holds the
> passwords in?

That's a system file, though Account Manager allows reading data from there to any authenticator, which handles the corresponding account type.

> I don't think solving this issue for rooted devices is required.

OK, in this case the issue should be solvable.
Comment 47 Richard Soderberg [:atoll] 2011-05-19 00:15:20 PDT
(In reply to comment #45)
> If the Account Manager's db is not readable on a non-rooted device, I think that will be sufficient.

"Universal Androot exploits a writeUI back door and allows users to simply press a "Root" button on the screen, and "UnRoot" to restore the device to the original state." -- http://wiki.cyanogenmod.com/index.php?title=Universal_Androot

"The password for email accounts is stored into the SQLite DB which in turn stores it on the phone's file system in plain text." -- http://code.google.com/p/android/issues/detail?id=10809

"the real potential threat is if someone were to create an application to automatically pull these out of your phone and send it back to the malware creator." -- http://www.intomobile.com/2010/09/20/rooted-android-passwords-plain-text/


Storing credentials and/or decryption keys in AccountManager puts our users' data at risk and will eventually shame us in the eyes of the community.  Please reconsider.

If the goal is to protect users from data theft, AccountManager is unacceptable.  When I lose my phone and someone picks it up and roots it, they are fully capable of reading the Fennec decryption key from the AccountManager sqlite database and then decrypting my passwords and  form history.

Either Fennec asks users for their password from time to time, or we accept that all Fennec data on a device can be read from a device that's been taken from its owner, even for just a few minutes.  Which is more important, convenience or protecting users from data theft?


Incidentally, rooting isn't the only threat.  There are devices for non-technical users that simply image the entire filesystem onto a USB stick.  Michigan apparently makes frequent use of them during traffic stops.  Fortunately these sort of devices are only available to law enforcement.  Unfortunately, they're also available to employees at your local carrier store, and presumably on the black market as well.

"It takes no more than a day for an officer to be trained to use the machine" -- http://www.bapcojournal.com/news/fullstory.php/aid/1336/Q_A_with_Cellebrite_about_their_Universal_Forensic_Extraction_Device.html

"The ACLU of Michigan expressed concern about the possible constitutional implications of using these devices to conduct suspicionless searches without consent or a search warrant." -- http://www.aclumich.org/issues/privacy-and-technology/2011-04/1542
Comment 48 Brad Lassey [:blassey] (use needinfo?) 2011-05-19 08:54:34 PDT
(In reply to comment #47)
> (In reply to comment #45)
> > If the Account Manager's db is not readable on a non-rooted device, I think that will be sufficient.
> 
> "Universal Androot exploits a writeUI back door and allows users to simply
> press a "Root" button on the screen, and "UnRoot" to restore the device to
> the original state." --
> http://wiki.cyanogenmod.com/index.php?title=Universal_Androot

That particular exploit was fixed in Froyo. If there is an inherent security hole with the OS, there's not much we can do. I for one do not want to have to enter my password every time I browse the web.

Perhaps we can have a 3 way preference for this ("Don't user master password"/"Use master password stored on my system"/"Use master password I enter every time"). If the user is for some reason stuck on a device with a known security flaw (or they're just hyper paranoid) they can opt for option 3. Let's not loose focus here though and concentrate on enabling option 2 for now.
Comment 49 Alex Pakhotin (:alexp) 2011-05-20 13:51:56 PDT
Created attachment 534094 [details] [diff] [review]
Patch v2

Use AccountManager to store an automatically generated master password.

It is still not very secure, as it is possible to write an application and access the password, which doesn't require any authentication from the user.
Comment 50 Alex Pakhotin (:alexp) 2011-05-20 15:36:22 PDT
(In reply to comment #49)
> It is still not very secure, as it is possible to write an application and
> access the password, which doesn't require any authentication from the user.

This is definitely not good, so we need a better approach.

I was considering the SharedPreferences again, and realized that the "run-as org.mozilla.fennec" way to access private data should not actually work in normal conditions! It does work for Fennec only because we have parameter android:debuggable="true" in the AndroidManifest.xml. If an app is not debuggable, its private data is not be accessible from the outside without root.

I wonder, why do we have android:debuggable unconditionally set to "true"?
Comment 51 Alex Pakhotin (:alexp) 2011-05-20 18:59:23 PDT
Created attachment 534184 [details] [diff] [review]
Patch v3

Use SharedPreferences to store the password.
With debuggable mode disabled (bug 658691) the preferences are not accessible outside of Fennec without rooting the device.
Comment 52 Doug Turner (:dougt) 2011-05-25 10:40:55 PDT
Comment on attachment 534184 [details] [diff] [review]
Patch v3

I should point out two important issues:

1) I don't think that this adds any additional security whatsoever.

The attack that we are worried about is "you lose your phone, don't have a password set up in android, then someone has access to your browser w/ all passwords ect.".

What you have going on here is that you generate a UUID, store it in shared prefs, and use that as the master password.  The user never has to know anything to authenticate.  That means, the user doesn't have to know anything and the attacker doesn't have to know anything.  no uncertain about the password, no information needed to access the data.

I think stuart's idea about saving it in a place that will auto-prompt for a password is awesome.  however, it doesn't look like there are any android APIs for this.

Instead, I think you should take the approach the the early UI did.  Allow the user to enter a password.  (take a look at matt's addon: https://addons.mozilla.org/en-US/mobile/addon/master-password-270907/).

Also, take dholbert's advice and have this master password timeout on idle.  This is super important.

2)

You don't need to add new interface to widget for any of this.  You also don't need to implement this in java.  Instead, i think you could just use JNI directly.  But, since your implementation is going to be completely different, I don't think it matters.
Comment 53 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-25 12:35:41 PDT
Comment 52 is focusing on the opt-in nature of master password. That is all well and good, but a majority of users willnot opt-in (if desktop usage can be used as an indicator) and therefore, we'll have many people exposed to password theft.

Why can't we use the existing approach in the patch as a baseline? If a user never wants to opt-in to a sepcific master password, Fennec will generate one for them and use it without the user needing to interact. The main benefit of this approach is that the passwords will be encrypted all the time. Sure, if the phone is not setup with a homescreen passcode and the phone is lost, anyone can open Fennec and log into websites. The baseline approach does not protect against that.

The baseline approach would protect against the profile being moved to an sd cared then then the card is lost.

The baseline approach should be able to sense a user manually adding a master password, which would then clear the auto-generated password. Even though we might not ship a master password UI in Fx6 or 7 and add-on could.

I just don't think we should have an all or nothing approach to password encryption. Everyone should have encrypted passwords.
Comment 54 Mike Connor [:mconnor] 2011-05-25 12:46:08 PDT
Well, we encrypt with an empty passphrase now, this is just a small increment over that.

Also, for implementation details, there's a couple of other features that would use an OS-level service, so I sort of like the "store it in a service" approach for a few reasons.  Not least of which is controlling duration for manually-entered PWs separate from however Fennec stays open on my memory-constrained Android phone, but it also means that the service is the canonical location for the non-blank default MP.
Comment 55 Brad Lassey [:blassey] (use needinfo?) 2011-05-25 12:52:55 PDT
(In reply to comment #52)
> Comment on attachment 534184 [details] [diff] [review] [review]
> Patch v3
> 
> I should point out two important issues:
> 
> 1) I don't think that this adds any additional security whatsoever.
> 
> The attack that we are worried about is "you lose your phone, don't have a
> password set up in android, then someone has access to your browser w/ all
> passwords ect.".
That has not been the point of this bug up to now. The premise has been if you loose your phone *and* have a password set up in android your data should be safe. Let's not move the goal posts.
Comment 56 Richard Soderberg [:atoll] 2011-05-25 13:00:44 PDT
(In reply to comment #55)
> The premise has been if you loose your phone *and* have a password set up in android
> your data should be safe.

Storing the master password in the Android credentials store violates the above premise, as the system credentials store is cleartext and easily accessible to an attacker who has the mobile device in their possession.  I accept the inconvenience of entering my password frequently in exchange for the certainty that my data cannot be stolen without a brute-force password attack.
Comment 57 Brad Lassey [:blassey] (use needinfo?) 2011-05-25 13:04:30 PDT
(In reply to comment #56)
> (In reply to comment #55)
> > The premise has been if you loose your phone *and* have a password set up in android
> > your data should be safe.
> 
> Storing the master password in the Android credentials store violates the
> above premise, 
Which is why we're not using the system credentials store
> as the system credentials store is cleartext and easily
> accessible to an attacker who has the mobile device in their possession.  I
> accept the inconvenience of entering my password frequently in exchange for
> the certainty that my data cannot be stolen without a brute-force password
> attack.
Yes, and that's a great decision for you. As MFinkle pointed out in comment 53, most users aren't going to make that choice, and we should give them the best security we can given that. Also, as I said in comment 48 and MFinkle hinted at in comment 53 we can provide both options to our users.
Comment 58 Richard Soderberg [:atoll] 2011-05-25 13:10:27 PDT
(In reply to comment #54)
> Also, for implementation details, there's a couple of other features that
> would use an OS-level service, so I sort of like the "store it in a service"
> approach for a few reasons.  Not least of which is controlling duration for
> manually-entered PWs separate from however Fennec stays open on my
> memory-constrained Android phone, but it also means that the service is the
> canonical location for the non-blank default MP.

I agree that using a service for master password caching would improve the user experience.  Storing the master password in a background service would decrease UX issues with the cached password being evicted from memory before the idle timeout and might enable a variety of interesting UX options that have not yet been suggested, both for master password and for other products (eg. Sync).

That said, I do not believe that a background service is required for a minimal implementation of the security goal posts described in comment 55 above.
Comment 59 Alex Pakhotin (:alexp) 2011-06-22 10:40:12 PDT
We should move forward with this bug.

As agreed with Brad the current approach does improve security, at least for the case when the profile is moved to SD card. The auto-generated password is stored on the device and not directly accessible, so the data on SD card is protected, especially for a situation when the card is lost.

Another point is about users who want more security, and don't mind entering a password. For that I think the original Vivien's patch should work. It wasn't ideal, but again, it's an improvement. What exactly were the issues with that approach?

Doug, could you also clarify your point #2 in the comment 52? The Java code is used to access SharedPreferences using native Android API. Did you have an alternative suggestion?
Comment 60 Doug Turner (:dougt) 2011-06-22 10:44:43 PDT
alex: how does this patch work with the addon's that provide a user-provided master password?
Comment 61 Alex Pakhotin (:alexp) 2011-06-22 10:46:10 PDT
(In reply to comment #60)
> alex: how does this patch work with the addon's that provide a user-provided
> master password?

I haven't tried, but I should. Thanks for reminding.
Comment 62 Alex Pakhotin (:alexp) 2011-06-22 10:56:02 PDT
Actually even without trying I'd guess the add-ons just won't work, because my patch on Android replaces a callback function, which normally prompts for a password with the one, which provides the password automatically. As far as I understand, the add-ons work with that regular prompt, and it won't be there. Though this still needs to be confirmed.
Comment 63 Brad Lassey [:blassey] (use needinfo?) 2011-06-22 11:06:59 PDT
If we provide master password functionality (i.e. some version of Vivien's patch) why do we care about breaking addons that do the same?
Comment 64 Alex Pakhotin (:alexp) 2011-06-22 11:20:05 PDT
(In reply to comment #63)
> If we provide master password functionality (i.e. some version of Vivien's
> patch) why do we care about breaking addons that do the same?

You're right - Matt's Master Password is basically that Vivien's patch, and only one of the derivations must be used at a time.
Comment 65 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-22 11:29:41 PDT
IMO:
* We should unbitrot Vivien's patch and get a UX + Dev review
* Alex's patch should not replace the callback. The front-end should decide whether to get the auto-password or not

Both patches use a preference to turn on Master Password, but the intent of Alex's approach was to never offer that preference. Master password would just be used all the time.

If we mix the two approaches, the preference would stay. If true, the dialog approach (Vivien) would be used. If false, the front-end would call nsIAutoPassword.getPassword() and return that without showing a prompt.

In any case, getting an opt-in (dialog) approach seems warranted. I can start to unbitrot.
Comment 66 Alex Pakhotin (:alexp) 2011-06-22 12:51:44 PDT
(In reply to comment #65)
> If we mix the two approaches, the preference would stay. If true, the dialog
> approach (Vivien) would be used. If false, the front-end would call
> nsIAutoPassword.getPassword() and return that without showing a prompt.

Yes, the idea is to mix the two approaches, and it makes sense to have the preference control the prompt.
My patch actually already reuses some of the JS code from Vivien's patch, now I'll need to keep most of it, just add my bits. But I have to look again into that callback function - there was a reason why I had to replace it.
Comment 67 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-22 13:36:22 PDT
Created attachment 541160 [details] [diff] [review]
updated dialog patch

This patch updates Vivien's patch to the new trunk. I can't seem to get the Master Password prompt to ever appear though. I mean:
https://bugzilla.mozilla.org/attachment.cgi?id=476808
Comment 68 Wesley Johnston (:wesj) 2011-06-27 12:42:47 PDT
Comment on attachment 541160 [details] [diff] [review]
updated dialog patch

Review of attachment 541160 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks and works fine for me. A few nits and one bigger question about an "initialized" check. Also there's a dump you need to remove.

::: mobile/chrome/content/MasterPasswordUI.js
@@ +25,5 @@
> +      let slot = this._secModuleDB.findSlotByName(this._tokenName);
> +      if (slot)
> +        this._dialog.setAttribute("status", slot.status);
> +
> +      document.getElementById("masterpassword-newpassword1").focus();

I really hate depending on both dialogs to have elements with the same ID. Can we use querySelector here?

@@ +44,5 @@
> +    let token = this._pk11DB.findTokenByName(this._tokenName);
> +    let newPasswordValue = document.getElementById("masterpassword-newpassword1").value;
> +dump("----- new password: " + newPasswordValue + "\n")
> +    let status = parseInt(this._dialog.getAttribute("status"));
> +    if (status == Ci.nsIPKCS11Slot.SLOT_READY || status != -1)

This looks funny to me. We're looking for the case where we don't need to call initPassword, right? Those are defined here:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/nsIPKCS11Slot.idl#53

and 2 = SLOT_UNINITIALIZED. So my naive reading is that this should be (status > Ci.nsIPKCS11Slot.SLOT_UNINITIALIZED)?

@@ +66,5 @@
> +  checkPassword: function mp_checkPassword() {
> +    let newPasswordValue1 = document.getElementById("masterpassword-newpassword1").value;
> +    let newPasswordValue2 = document.getElementById("masterpassword-newpassword2").value;
> +
> +    let buttonOk = this._dialog.getElementsByAttribute("class", "prompt-buttons")[0].firstChild;

Can we use getElementsByClassName?

@@ +94,5 @@
> +  },
> +
> +  updatePreference: function mp_updatePreference() {
> +    document.getElementById("prefs-master-password").value = this.hasMasterPassword();
> +  }

We could move this little function out of here and save loading MasterPasswordUI.js unless a password was being set. If you wanted to work even harder on things that likely don't matter, you could then load this script with the dialogs by adding it as a src attribute on the dialog itself.
Comment 69 Alex Pakhotin (:alexp) 2011-06-28 18:58:54 PDT
(In reply to comment #68)
> I really hate depending on both dialogs to have elements with the same ID.
> Can we use querySelector here?

Wes, could you please elaborate what you mean? I am not that familiar with our JS code to try optimizing at such level.

> > +    if (status == Ci.nsIPKCS11Slot.SLOT_READY || status != -1)
> 
> This looks funny to me.

Yeah, I was also confused by this. In my version I just check for the Ci.nsIPKCS11Slot.SLOT_UNINITIALIZED, this seems to be a correct way.

> > +    let buttonOk = this._dialog.getElementsByAttribute("class", "prompt-buttons")[0].firstChild;
> 
> Can we use getElementsByClassName?

Do you mean document.getElementsByClassName? This doesn't seem to work with the _dialog.
Comment 70 Alex Pakhotin (:alexp) 2011-06-28 19:53:26 PDT
Created attachment 542708 [details] [diff] [review]
Patch v4

Combined UI and auto-password patches.
Now auto-password is used by default, encrypting the data with an automatically generated password stored in Android SharedPreferences. If the master password is enabled in the preferences, the user is prompted to enter one when the data is accessed.
Comment 71 Alex Pakhotin (:alexp) 2011-06-28 19:55:36 PDT
Comment on attachment 542708 [details] [diff] [review]
Patch v4

Wes, could you have a look at the FE change please? It's based on Vivien/Mark's patch.
Comment 72 Brad Lassey [:blassey] (use needinfo?) 2011-06-29 09:57:06 PDT
Comment on attachment 542708 [details] [diff] [review]
Patch v4

Review of attachment 542708 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the the Android widget code (widget/src/android and embedding/android).

Requesting review from Kai for the security/manager changes and relying on Wes for the mobile/* review
Comment 73 Wesley Johnston (:wesj) 2011-06-29 14:26:46 PDT
Comment on attachment 542708 [details] [diff] [review]
Patch v4

Review of attachment 542708 [details] [diff] [review]:
-----------------------------------------------------------------

This patch won't run on Desktop because Cc["@mozilla.org/security/autopassword;1"].getService(Ci.nsIAutoPassword); isn't defined there. Can you fix that?

I'm happy to let the getElementsById stuff slide for now. We have bugs where we may accidentally import more than one dialog into the browser. When we do, the results of document.getElementById("SOME_ID_IN_THE_DIALOG") are unpredictable. I'll address this in bug 621506 though.

getElementsByClassName is fine too. I'm not sure what best practice is there, but its unlikely to be a significant performance difference.
Comment 74 Matt Brubeck (:mbrubeck) 2011-06-29 15:26:22 PDT
Is it critical to do call MasterPasswordUI.init before the first page load?  If not, then it should be done in the UIReadyDelayed handler instead of BrowserUI.init.
Comment 75 Alex Pakhotin (:alexp) 2011-07-05 17:04:41 PDT
Somehow I missed this comment.

(In reply to comment #74)
> Is it critical to do call MasterPasswordUI.init before the first page load? 
> If not, then it should be done in the UIReadyDelayed handler instead of
> BrowserUI.init.

Frankly speaking there was a question where exactly to put that call. The initialization needs to be called at some time on startup, and actually it is important only for the first run to set the auto-password when no any password was set yet. I think UIReadyDelayed handler is good enough for that. Is there any initialization function, which is called only on the first run?
Comment 76 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-06 07:58:43 PDT
(In reply to comment #73)

> I'm happy to let the getElementsById stuff slide for now. We have bugs where
> we may accidentally import more than one dialog into the browser. When we
> do, the results of document.getElementById("SOME_ID_IN_THE_DIALOG") are
> unpredictable. I'll address this in bug 621506 though.
> 
> getElementsByClassName is fine too. I'm not sure what best practice is
> there, but its unlikely to be a significant performance difference.

When showing the dialog, we should be able to check to make show it is not already visible. It should never be "shown" if it is already visible.
Comment 77 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-06 11:21:35 PDT
(In reply to comment #75)
> Somehow I missed this comment.
> 
> (In reply to comment #74)
> > Is it critical to do call MasterPasswordUI.init before the first page load? 
> > If not, then it should be done in the UIReadyDelayed handler instead of
> > BrowserUI.init.
> 
> Frankly speaking there was a question where exactly to put that call. The
> initialization needs to be called at some time on startup, and actually it
> is important only for the first run to set the auto-password when no any
> password was set yet. I think UIReadyDelayed handler is good enough for
> that. Is there any initialization function, which is called only on the
> first run?

The current patch seems to call init() in UIReadyDelay, which seems fine from looking at the impl of init()
Comment 78 Alex Pakhotin (:alexp) 2011-07-06 11:35:33 PDT
(In reply to comment #77)
> The current patch seems to call init() in UIReadyDelay, which seems fine
> from looking at the impl of init()

No, the current patch adds that call to the end of BrowserUI.init(), outside of the "UIReadyDelayed" sub-function. A WIP approach needed to be clarified, and now it is clear. Thanks!
Comment 79 Alex Pakhotin (:alexp) 2011-07-07 18:49:14 PDT
Created attachment 544677 [details] [diff] [review]
Patch v5

Addressed code review comments:
- Added handling for the case when nsIAutoPassword is not supported;
- Added a check for the dialog visibility;
- Moved MasterPasswordUI initialization to UIReadyDelayed sub-function.

Also updated to the latest code.
Comment 80 Alex Pakhotin (:alexp) 2011-07-07 18:52:28 PDT
Comment on attachment 544677 [details] [diff] [review]
Patch v5

r=blassey for the the Android widget code, which wasn't changed in the latest patch.
Re-requesting review from Kai for the security/manager changes.
Comment 81 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-07 22:01:30 PDT
Comment on attachment 544677 [details] [diff] [review]
Patch v5

The mobile part looks OK to me
Comment 82 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-10 18:26:23 PDT
embedding/android/GeckoAppShell.java:

> password = UUID.randomUUID().toString();

Use PK11_GenerateRandom(x, 128 / PR_BITS_PER_BYTE) to generate the random password. UUID.randomUUID() isn't guaranteed to return a cryptographically secure result[1], and we have no way to know how it is implemented on any particular Android device.

[1] In Java SE, UUID.randomUIID promises to be cryptographically random, but the Android UUID.randomUIID doesn't make any such claim.

Is there a reason we are doing this for Android only, instead of also for all platforms? It seems like it would be just as useful for laptops, netbooks, and Windows- or Linux- based tablets, if not desktops. Accordingly, it makes sense to keep the naming platform-neutral. For example, the function AndroidPK11Password should be renamed to DefaultInternalTokenPassword.
Comment 84 Alex Pakhotin (:alexp) 2011-07-12 16:36:00 PDT
(In reply to comment #82)

> Is there a reason we are doing this for Android only, instead of also for
> all platforms? It seems like it would be just as useful for laptops,
> netbooks, and Windows- or Linux- based tablets, if not desktops.

This whole feature was designed basically for just one scenario - when Fennec stores user data on an external memory card, to make sure the data is encrypted and not accessible by anyone else in case if the card is lost or stolen. It also helps a bit in case if the device itself is lost or stolen, but only until it's rooted.

The original idea was to use Android system secured storage to save the automatically-generated password. It turned out there is no really such thing - the account manager is a bit different entity, so the latest implementation is using SharedPreferences. Essentially that is just an XML file stored in the application's private directory, so theoretically we could just create such a file ourselves. But this raises a question - does this approach really make any sense on other systems if the password will be simply stored as a plain text? What advantage would it add on a laptop/netbook? The security database is currently already created with only user read permissions, it is not accessible without logging into the user's account, right? Are there cases when user data is stored somewhere else, where it may be available for other users?
Comment 85 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-12 16:48:37 PDT
(In reply to comment #84)
> (In reply to comment #82)
> > Is there a reason we are doing this for Android only, instead of also for
> > all platforms? It seems like it would be just as useful for laptops,
> > netbooks, and Windows- or Linux- based tablets, if not desktops.
> 
> This whole feature was designed basically for just one scenario - when
> Fennec stores user data on an external memory card, to make sure the data is
> encrypted and not accessible by anyone else in case if the card is lost or
> stolen. It also helps a bit in case if the device itself is lost or stolen,
> but only until it's rooted.

Is this a P1 feature because we are worried about lost memory cards? Can we get the same level of security by simply forcing the key database to always be on internal storage? Have we measured the performance and battery life impact of storing the profile on a removable memory card vs. internal storage? (When I was doing mobile development, we avoided removable memory cards at all costs, because they have traditionally been incredibly power-hungry and incredibly slow, compared to internal storage. I don't know if this has changed.)

> laptop/netbook? The security database is currently already created with only
> user read permissions, it is not accessible without logging into the user's
> account, right? Are there cases when user data is stored somewhere else,
> where it may be available for other users?

AFAICT, a laptop hard drive is equivalent to a memory card, as far as security is concerned, because both are removable and are then readable without user authentication, unless filesystem encryption (e.g. EFS on Windows) or effective whole-disk encryption (e.g. Bitlocker with TPM) is used. On Windows and Mac OS X (at least), you could store the master password in the system key chain, which would protect the passwords even if filesystem/whole-disk-encryption isn't being used.
Comment 86 Richard Soderberg [:atoll] 2011-07-12 16:52:22 PDT
FWIW, I was worried about my Android device's internal storage, which is not encrypted.  Hadn't considered that it was even possible to store Firefox data on the SD card.
Comment 87 Alex Pakhotin (:alexp) 2011-07-12 17:35:27 PDT
(In reply to comment #85)
> Is this a P1 feature because we are worried about lost memory cards? Can we
> get the same level of security by simply forcing the key database to always
> be on internal storage? Have we measured the performance and battery life
> impact of storing the profile on a removable memory card vs. internal
> storage?

I'm not aware of such performance/battery life tests, I just know the fact that the internal storage memory on many devices is quite limited, and the ability to move application and/or its data to a storage card is definitely a highly demanded feature. If it's possible to store the key database separately from the rest of the user's profile, forcing it to stay in the internal storage, I guess this would solve the issue with an external card.

> AFAICT, a laptop hard drive is equivalent to a memory card, as far as
> security is concerned, because both are removable and are then readable
> without user authentication, unless filesystem encryption (e.g. EFS on
> Windows) or effective whole-disk encryption (e.g. Bitlocker with TPM) is
> used. On Windows and Mac OS X (at least), you could store the master
> password in the system key chain, which would protect the passwords even if
> filesystem/whole-disk-encryption isn't being used.

This makes sense. 
I'll make this change more generic so it would be less Android-specific, and could be implemented for other systems.
Comment 88 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-12 22:07:03 PDT
Some facts:
* Android users love to move apps and data to sdcards. We have to live with that, regardless of power and performance issues.
* The current password security is not good enough, for sdcard or internal storage.
* Master password support is being added to mobile to give additional security, beyond a random or somewhat random auto-generated password.
* This bug is not the end of the work. Impl's for other platforms, additional master password timeouts can be filed as new bugs.

We are not 100% solving the problem in this bug. Let's get the patch right, reviewed, landed and move onto the next bug.
Comment 89 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-12 22:45:19 PDT
To to clarify comment 83: The master password does not securely protect the data from an attacker that can read the key8.db file, unless that master password is extraordinarily strong--much stronger than we can reasonably expect a user to enter on the Android keyboard. For this reason, unless/until NSS is changed, it is important to protect the key8.db file as strongly as we would protect the generated master password, when a user-provided master password is used. But, if we were to do that, then we wouldn't need to generate a strong default master password.

> We are not 100% solving the problem in this bug. Let's get the patch right,
> reviewed, landed and move onto the next bug.

AFAICT, the attack that is prevented by this patch is only this one: The Firefox profile is stored on a removable memory card, that memory card is not in the phone, the phone is not accessible to the attacker, the phone is rooted or rootable, and the user has not chosen his own master password (so the strong generated password is being used). In this specific case (with ALL those conditions met), the data would be protected better with this patch applied than without it. In all other cases, the patch has no security benefit. Is this what is expected?
Comment 90 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-12 23:05:30 PDT
I can't view comment 83. Entering a master password is a feature that exists on desktop Firefox. It's a feature people have asked for on mobile Firefox. This patch gives users that feature. The strength/weakness of a user generated password is the same on mobile and desktop.

I certainly agree that a weak master password helps no one. Perhaps we could look at alternative entry methods to achieve strong manually entered passwords.

As to the attack vector covered by this patch, you are correct. Although, whether the phone is rooted or rootable is not relevant if the attack does not have access to the phone, I assume.
Comment 91 Kai Engert (:kaie) 2011-07-13 08:00:18 PDT
AndroidPK11Password must be threadsafe, it can be called from any thread.

In AndroidPK11Password you access the nsIPrefBranch - people have complained to me in the past that this is not allowed on secondary threads.

I think you must introduce a new bool variable, which you init at PSM init time, and update when we get notified of a pref change, and the variable must be protected with a lock.

This sounds complicated, but you can mostly use existing infrastructure. Search for existing preference "security.enable_ssl3", I propose to add code for your pref accordingly.

You might want to rename your pref, to make it start with "security."; that way we don't need a new observer.

You could add the new boolean to nsNSSComponent.h, e.g. next to "globalConstFlagUsePKIXVerification".

Regarding the lock, well, nsNSSComponent already has a lock to protect its member variables. If you add your code next to the existing code that deals with "security.enable_ssl3", then modifications to the new boolean will be protected by a lock.

In AndroidPK11Password, you could read the bool without having the lock set.
Comment 92 Alex Pakhotin (:alexp) 2011-07-16 00:24:38 PDT
Created attachment 546289 [details] [diff] [review]
Patch v6 [WIP]

Addressed code review comments:
- Generating password using nsIRandomGenerator.GenerateRandomBytes();
- Got rid of Java part - now using a local file in the Fennec directory to store the auto-password;
- Made the NSS callback that uses the auto-password system-independent. If nsIAutoPassword service is implemented, it is used in case there is no master password UI preference set.

The preference-related change suggested by Kai is not finished yet, I would like the security guys to have a look at the current implementation.

I have a concern about the randomly generated password conversion to UTF16 and back to char. Conversion to wide string is required for nsIPK11Token's methods initPassword() and changePassword(). Not sure if it's possible to avoid such conversion. Anybody has a suggestion how to handle this better?
Comment 93 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-16 12:00:26 PDT
(In reply to comment #92)
> - Got rid of Java part - now using a local file in the Fennec directory to
> store the auto-password;

If we can do this, then why not just store the key3.db file in the Fennec directory and avoid generating a password? Then this security mechanism would work equally well for generated passwords and user-chosen master passwords, and we would avoid the complexity of the password generation.

> I have a concern about the randomly generated password conversion to UTF16
> and back to char. Conversion to wide string is required for nsIPK11Token's
> methods initPassword() and changePassword(). Not sure if it's possible to
> avoid such conversion. Anybody has a suggestion how to handle this better?

I think the conversion you implemented seems OK to me. Another way to do the generation would be to do something like this:

    for (i = 0; i < 128 / PR_BITS_PER_BYTE / 2; ++i) {
        do {
           x = two random bytes
        } while (!isValidUTF16(x));
    }

However, again, I recommend trying to store the key3.db file where you are storing the master password file, avoiding this entirely.
Comment 94 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-16 12:28:25 PDT
(In reply to comment #92)
> Created attachment 546289 [details] [diff] [review] [review]
> Patch v6 [WIP]

> - Got rid of Java part - now using a local file in the Fennec directory to
> store the auto-password;

Won't this put the password on the sdcard if the user moves the app to the sdcard?
Comment 95 Alex Pakhotin (:alexp) 2011-07-16 15:52:23 PDT
(In reply to comment #94)
> Won't this put the password on the sdcard if the user moves the app to the
> sdcard?

No - when the app moves to the sdcard, we only move the user profile dir there.
The password file, like some other, stays in the main Fennec directory under /data/data/org.mozilla.*/.

(In reply to comment #93)
> If we can do this, then why not just store the key3.db file in the Fennec
> directory and avoid generating a password? Then this security mechanism
> would work equally well for generated passwords and user-chosen master
> passwords, and we would avoid the complexity of the password generation.

The key*.db is stored in the user profile, which whole directory moves to sdcard. I am not sure if we could keep just one file from there in the internal memory.
Besides, having the AutoPassword service would allow us implementing this feature on other systems, that was one of the new goals, as far as I understood. On those systems, which provide an internal secured storage, the service would save the generated password in that storage, but not in a simple file like this implementation.
Comment 96 Alex Pakhotin (:alexp) 2011-07-19 01:15:14 PDT
Created attachment 546728 [details] [diff] [review]
Patch v7

Made DefaultInternalTokenPassword callback function thread-safe by handling the preference change in an observer as suggested.

FE has not changed (r=mfinkle)
Comment 97 Brad Lassey [:blassey] (use needinfo?) 2011-07-22 18:40:55 PDT
Comment on attachment 546728 [details] [diff] [review]
Patch v7

Review of attachment 546728 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 98 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-22 19:17:38 PDT
Comment on attachment 546728 [details] [diff] [review]
Patch v7


>diff --git a/mobile/app/mobile.js b/mobile/app/mobile.js

> // Name of alternate about: page for certificate errors (when undefined, defaults to about:neterror)
> pref("security.alternate_certificate_error_page", "certerror");
> 
> pref("security.warn_viewing_mixed", false); // Warning is disabled.  See Bug 616712.
>+pref("security.useMasterPasswordUI", false);

I know it's a nit, but maybe you should use the same pref name style as the other security prefs:

pref("security.use_master_password_ui", false);

Otherwise mobile code looks fine
Comment 99 Alex Pakhotin (:alexp) 2011-07-22 20:51:34 PDT
(In reply to comment #98)
> >+pref("security.useMasterPasswordUI", false);
> 
> I know it's a nit, but maybe you should use the same pref name style as the
> other security prefs:
> 
> pref("security.use_master_password_ui", false);

Yeah, I wasn't sure about that myself. Originally followed the naming convention for the other password manager preferences, which use capitalized words without underscore, but as it's now in another group, probably it's better to rename it.
Comment 100 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-24 08:27:15 PDT
1. How will this interact with Android 3.0's master password and encryption mechanism? http://source.android.com/tech/encryption/android_crypto_implementation.html. Will we still need this on Android 3.0 devices?

2. The user removes the removable storage from device A and inserts it into device B. Device B will not have the master password on it. How does the user recover from this situation? Is there a convenient way to reset the key database (and everything that depends on it, including the password database)? This mechanism wouldn't work well for a user that wants to use removable storage to maintain a consistent Firefox profile across devices, which I would think would be a common reason for wanting to store Firefox and the user profile on the removable storage.
Comment 101 Matt Brubeck (:mbrubeck) 2011-07-24 08:31:43 PDT
(In reply to comment #100)
> 2. The user removes the removable storage from device A and inserts it into
> device B. Device B will not have the master password on it. How does the
> user recover from this situation? Is there a convenient way to reset the key
> database (and everything that depends on it, including the password
> database)? This mechanism wouldn't work well for a user that wants to use
> removable storage to maintain a consistent Firefox profile across devices,
> which I would think would be a common reason for wanting to store Firefox
> and the user profile on the removable storage.

This scenario isn't supported at all on Android, because apps on the SD card are themselves encrypted with device-specific keys:

http://android-developers.blogspot.com/2010/07/apps-on-sd-card-details.html
Comment 102 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-26 09:10:20 PDT
I cannot attend the security review meeting tomorrow so I will put my comments here. See also comment 89 to understand the range of attacks that are to be prevented.

Please see bug 524403 / bug 581528. If the attacker has access to the key3.db file, he can probably crack the master password--especially on a mobile device, where it is unreasonably difficult to type a good, long, complex password. Therefore, it is critical to protect the key3.db file in the same was as is proposed to protect the new auto-generated master password file. But, if the key3.db file is protected the same way, then there is no need to use the auto-generated master password file, and so we shouldn't add that complexity. In particular, if the goal is to protect profile data stored on sdcards, then the key3.db file must not be stored on the sdcard (at least when a user-chosen master password is used).

It would be straightforward to change PSM so that the key3.db, cert8.db, and secmod.db files were stored in the same folder that that Alex proposes to store the auto-generated master password file, without requiring changes to NSS. I would much rather do this than add the auto-generated master password file mechanism to PSM.

The goal of this bug is to protect the user's passwords. Note that if an attacker can tamper with the profile data, then he can add new trusted CA certificates and/or certificate overrides to the profile, and cause the user to disclose his password that way. For this reason, it makes sense to protect the cert8.db and cert override file as well as we are protecting the secret decoder ring key.

Some Android devices already have [1], and more will have [2], a master password mechanism with full disk encryption for the internal storage and/or external storage. IMO, when this is the case, we should disable our master password mechanism and redirect the user to the operating system's settings.

[1] https://motorola-enterprise.custhelp.com/app/answers/detail/a_id/57094/~/android---encryption
[2] http://source.android.com/tech/encryption/android_crypto_implementation.html
Comment 103 Brad Lassey [:blassey] (use needinfo?) 2011-07-26 09:15:44 PDT
(In reply to comment #102)
> The goal of this bug is to protect the user's passwords. 
No, the goal of this bug is to protect the entire profile. What your suggesting sounds like a good additional step to take, but let's not derail this bug with it. Please review the patch that's posted.
Comment 104 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-07-26 09:35:54 PDT
(In reply to comment #103)
> (In reply to comment #102)
> > The goal of this bug is to protect the user's passwords. 
> No, the goal of this bug is to protect the entire profile. What your
> suggesting sounds like a good additional step to take, but let's not derail
> this bug with it. Please review the patch that's posted.

The patch only protects the data that is encrypted with the SDR key, which isn't the whole profile. In particular, cert_override.txt (and I think cert8.db) isn't protected. And, for all practical purposes, nothing would be protected when the user chooses his own master password because of the NSS bug I cited in my previous comment + the impracticality of using a strong master password on a mobile device. If we want this to be an effective mechanism then we will need to fix the NSS bug.
Comment 105 Alex Pakhotin (:alexp) 2011-08-03 14:57:37 PDT
Comment on attachment 546728 [details] [diff] [review]
Patch v7

As the bug has passed the security review, let's finish it.
Brian, could you review the security manager part of the patch please?
Comment 106 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-09 18:07:20 PDT
Comment on attachment 546728 [details] [diff] [review]
Patch v7

Review of attachment 546728 [details] [diff] [review]:
-----------------------------------------------------------------

This is not a full review.

There are several problems with the auto password mechanism. Also, we need to avoid the duplication between mobile's MasterPasswordUI.js and and PSM's password.js files. These need to be addressed before I can do a meaningful review.

Supporting the master password mechanism would require a means of resetting the profile when the master password is unknown to the user (e.g. forgotten). Alex said this requires feedback from the UX team.

::: mobile/chrome/content/MasterPasswordUI.js
@@ +4,5 @@
> +
> +  get _secModuleDB() {
> +    delete this._secModuleDB;
> +    return this._secModuleDB = Cc["@mozilla.org/security/pkcs11moduledb;1"].getService(Ci.nsIPKCS11ModuleDB);
> +  },

Keep all code that accesses (non-UI) PSM interfaces together in PSM's browser.js and avoid copy/past/editing code that accesses PSM whenever possible.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +69,5 @@
>  #include "nsIPrompt.h"
>  #include "nsProxyRelease.h"
>  #include "nsIConsoleService.h"
> +#include "nsIPrefService.h"
> +#include "nsIPrefBranch.h"

Remove these two includes.

@@ +856,5 @@
> +    char* password = GetAutoPassword();
> +    if (password != nsnull)
> +      return password;
> +  }
> +

Rename DefaultInternalTokenPassword to PK11AutoPassword inline GetAutoPassword into PK11AutoPassword.

If getting the auto-password failed then the user is not going to be able to type it in so we must not fall back to prompting the user here. Instead, we should call PK11_SetPasswordFunc with either PK11AutoPassword or Pk11PasswordPrompt depending on the value of the pref. Then you don't need to inspect nsNSSComponent::useMasterPasswordUI above and useMasterPasswordUI can be made a private member variable instead of a public static member.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2393,5 @@
>                 || prefName.Equals("security.OCSP.require")) {
>        MutexAutoLock lock(mutex);
>        setValidationOptions(mPrefBranch);
> +    } else if (prefName.Equals("security.useMasterPasswordUI")) {
> +      mPrefBranch->GetBoolPref("security.useMasterPasswordUI", &useMasterPasswordUI);

We must treat the changing of this pref like changing the master password.

For example, if the value was TRUE, then we would need to prompt the user for the old master password (if any), and then generate a new master password file, then set the master password of the internal module to the value in the new master password file.

I stillneed to investigate under what circumstances the internal token would reject a password, which might cause the auto-password file to get out of sync with the token during such transitions.

::: widget/src/android/nsAutoPassword.cpp
@@ +63,5 @@
> +  password[passwordLen] = '\0';
> +  nsresult rv = NS_OK;
> +
> +  if (NS_FAILED(ReadPassword(password, passwordLen))) {
> +    // Couldn't read the password, generating a new one

We cannot just generate a new password when reading the old one fails. Everything would be encrypted with the key derived from the old password. I do not know what we should do instead though.

@@ +85,5 @@
> +}
> +
> +nsresult nsAutoPassword::GetPasswordFileName(nsACString &aFileName)
> +{
> +  char* greHome = getenv("GRE_HOME");

AFAICT, GRE_HOME isn't guaranteed to be in internal memory, and that defeats the whole purpose of the auto-password mechanism. Please advise.
Comment 107 Alex Pakhotin (:alexp) 2011-08-10 13:43:28 PDT
(In reply to Brian Smith (:bsmith) from comment #106)
> > +nsresult nsAutoPassword::GetPasswordFileName(nsACString &aFileName)
> > +{
> > +  char* greHome = getenv("GRE_HOME");
> 
> AFAICT, GRE_HOME isn't guaranteed to be in internal memory, and that defeats
> the whole purpose of the auto-password mechanism. Please advise.

We use ApplicationInfo.dataDir to set GRE_HOME env variable. That is "a directory assigned to the package for its persistent data" (http://developer.android.com/reference/android/content/pm/ApplicationInfo.html#dataDir)
That data directory is used by Android system to store private application data, like SharedPreferences, so for this purpose the directory must not be accessible by other applications.
Comment 108 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-10 18:46:45 PDT
(In reply to Alex Pakhotin (:alexp) from comment #107)
> We use ApplicationInfo.dataDir to set GRE_HOME env variable. That is "a
> directory assigned to the package for its persistent data"
> (http://developer.android.com/reference/android/content/pm/ApplicationInfo.
> html#dataDir)
> That data directory is used by Android system to store private application
> data, like SharedPreferences, so for this purpose the directory must not be
> accessible by other applications.

Right, but isn't the user profile (and thus key3.db) stored under that same path?

The auto-password file must always be stored in internal memory, even when the key3.db file isn't. When both files are stored under the same directory on the same device, the auto-password file isn't helpful.
Comment 109 Alex Pakhotin (:alexp) 2011-08-10 18:58:03 PDT
(In reply to Brian Smith (:bsmith) from comment #108)
> Right, but isn't the user profile (and thus key3.db) stored under that same
> path?

No, the profile is separate. As far as I can tell, the user profile is stored in a directory pointed by the HOME env variable, while the password file is under GRE_HOME, which is always internal memory.
Comment 110 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-10 19:50:04 PDT
(In reply to Alex Pakhotin (:alexp) from comment #109)
> GRE_HOME, which is always internal memory.

This is what I asked yesterday on IRC. Where is it documented that GRE_HOME (dataDir) is always internal memory?
Comment 111 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-10 20:07:00 PDT
The blog post at [1] from 2010 says "(Note that not all of the contents of an “SD-card-resident” APK are actually on the card; the dex files, private data directories, and native shared libraries remain in internal storage.)"
Comment 112 Brad Lassey [:blassey] (use needinfo?) 2011-08-10 20:18:52 PDT
(In reply to Brian Smith (:bsmith) from comment #110)
> (In reply to Alex Pakhotin (:alexp) from comment #109)
> > GRE_HOME, which is always internal memory.
> 
> This is what I asked yesterday on IRC. Where is it documented that GRE_HOME
> (dataDir) is always internal memory?

yes
Comment 113 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-10 20:20:29 PDT
(In reply to Brian Smith (:bsmith) from comment #106)

> This is not a full review.
> 
> There are several problems with the auto password mechanism. Also, we need
> to avoid the duplication between mobile's MasterPasswordUI.js and and PSM's
> password.js files. These need to be addressed before I can do a meaningful
> review.

I'd like more of a explanation for not wanting to review MasterPasswordUI.js - which is JS that uses the PSM interfaces in a UI that is specific to Fennec - not desktop Firefox.

password.js is used in the XUL UI which is not wanted in Fennec. I don't see much duplication beyond some simple setting and getting of data from XUL elements. We have no intentions of using the code in our UI.

> Supporting the master password mechanism would require a means of resetting
> the profile when the master password is unknown to the user (e.g.
> forgotten). Alex said this requires feedback from the UX team.

Currently, resetting the profile is a hard reset and can be done by using the Android "Settings" app. The user can delete the "data", which kills the profile. The next time Fennec starts, a fresh profile is generated.
Comment 114 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-10 20:58:11 PDT
(In reply to Brian Smith (:bsmith) from comment #106)

> ::: mobile/chrome/content/MasterPasswordUI.js
> @@ +4,5 @@
> > +
> > +  get _secModuleDB() {
> > +    delete this._secModuleDB;
> > +    return this._secModuleDB = Cc["@mozilla.org/security/pkcs11moduledb;1"].getService(Ci.nsIPKCS11ModuleDB);
> > +  },
> 
> Keep all code that accesses (non-UI) PSM interfaces together in PSM's
> browser.js and avoid copy/past/editing code that accesses PSM whenever
> possible.

PSM does not have a browser.js and using XPCOM components like this is common and has never been a problem. I see the PSM components access in other non PSM code.
Comment 115 Alex Pakhotin (:alexp) 2011-08-10 23:58:33 PDT
OK, I'll try to summarize what was discussed on IRC and mentioned in the last comments.

So, there are several concerns:

1. Is auto-password really needed, maybe just split up the profile and store the important part (key3.db, etc) in the internal memory unconditionally.
- As far as I understood, the auto-password feature in general was approved during security review, this might be even used on other systems with more proper storage of the generated password (e.g. in the OS keychain);
- Splitting the profile sounds like an additional security measure, which could (and should?) be implemented separately.

2. Some JS code under /mobile in this patch is similar to PSM's password.js file, and should be refactored to avoid duplication and double efforts from security team when implementing changes.
- We don't really see exact duplication, except the similar pattern followed to set the password and check its validity using the public interfaces of nsIPKCS11ModuleDB and nsIPK11TokenDB services.
  If this is considered to be a problem, let's file a follow-up bug to do a required refactoring. It just needs to be clarified, what exactly common code has to be made into a separate component and/or interface.

3. There is a question, what exactly to do if we cannot read the password, which was set before, thus cannot access the profile. 
- There is a workaround involving resetting the profile, but the user probably has to be notified about this problem. Something apparently needs to be done in the current patch.

4. Is the directory where the auto-password is stored, defined by GRE_HOME environment variable, always in the internal memory?
- The answer is yes - this is by the nature of GRE_HOME variable, which is specifically separated from the HOME. If it points somewhere else, that would be a bug and had to be fixed.

5. Some comments on the code itself.
- Just have to be addressed.

Please let me know if I missed or misunderstood something.
Comment 116 Daniel Veditz [:dveditz] 2011-08-11 07:34:18 PDT
(In reply to Daniel Holbert [:dholbert] from comment #24)
> > Remember, the desktop version doesn't have a master password timeout either,
> 
> Right -- (I'm not asking for a timeout) -- but on the desktop, you can quit
> Firefox and be secure in the knowledge that your master-password-protected
> data is safe.  On the phone, there's no such ability right now.

On desktop Firefox you can dig into the advanced|encryption options and log out of the "Software Security Device". Bit of a PITA but possible without any add-ons and without needing to quit Firefox.

(In reply to Mark Finkle (:mfinkle) from comment #113)
> (In reply to Brian Smith (:bsmith) from comment #106)
> > Supporting the master password mechanism would require a means of resetting
> > the profile when the master password is unknown to the user (e.g.
> > forgotten). Alex said this requires feedback from the UX team.
> 
> Currently, resetting the profile is a hard reset and can be done by using
> the Android "Settings" app. The user can delete the "data", which kills the
> profile. The next time Fennec starts, a fresh profile is generated.

If I forget my Master Password my only recourse is to wipe out all my history, bookmarks, cookies, etc. as well? Terrible choice! I guess if I'm using Sync I'd get some of that back fairly easily?
Comment 117 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-11 07:39:02 PDT
(In reply to Daniel Veditz from comment #116)

> (In reply to Mark Finkle (:mfinkle) from comment #113)
> > (In reply to Brian Smith (:bsmith) from comment #106)
> > > Supporting the master password mechanism would require a means of resetting
> > > the profile when the master password is unknown to the user (e.g.
> > > forgotten). Alex said this requires feedback from the UX team.
> > 
> > Currently, resetting the profile is a hard reset and can be done by using
> > the Android "Settings" app. The user can delete the "data", which kills the
> > profile. The next time Fennec starts, a fresh profile is generated.
> 
> If I forget my Master Password my only recourse is to wipe out all my
> history, bookmarks, cookies, etc. as well? Terrible choice! I guess if I'm
> using Sync I'd get some of that back fairly easily?

Yeah, it's pretty bad. Sync would let you get most everything back. Is there another way we should look at doing this? I didn't think desktop Firefox had a way to recover from a forgotten master password either.
Comment 118 Daniel Veditz [:dveditz] 2011-08-11 07:59:40 PDT
(In reply to Mark Finkle (:mfinkle) from comment #117)
> Yeah, it's pretty bad. Sync would let you get most everything back. Is there
> another way we should look at doing this? I didn't think desktop Firefox had
> a way to recover from a forgotten master password either.

Desktop lets you reset just the master password. You lose everything encrypted with it but that's much less than the whole profile (mostly passwords, certs in the rare case you have any).

To add this feature there's got to be some UI to create and change the master password, any room near there for a reset button?
Comment 119 Matt Brubeck (:mbrubeck) 2011-08-11 08:40:26 PDT
(In reply to Daniel Holbert [:dholbert] from comment #22)
> One piece of feedback: it'd be nice to have a means to re-lock your master
> password, at least on Android, since there's no explicit way to quit Firefox
> there.

Some time after this comment was written, we added a "Quit" command to the Firefox menu on Android (bug 659670).
Comment 120 Richard Soderberg [:atoll] 2011-08-11 10:24:30 PDT
(In reply to Daniel Veditz from comment #116)
> If I forget my Master Password my only recourse is to wipe out all my
> history, bookmarks, cookies, etc. as well? Terrible choice! I guess if I'm
> using Sync I'd get some of that back fairly easily?

Only if you wrote down or saved the long "Sync Key" that is used to encrypt/decrypt the data on the server.  Many (most?) users do not, so if their profile is wiped, they either need to use Easy Setup with a working desktop Sync installation to reconnect, or they lose access to the server data altogether.
Comment 121 Brad Lassey [:blassey] (use needinfo?) 2011-08-11 11:58:25 PDT
removing sec-review-needed because we already had sec-review on this bug.

bsmith, what does [sr:bsmith] in the whiteboard mean?
Comment 122 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-11 13:34:07 PDT
It means I am responsible for the security review part.
Comment 123 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-11 13:45:46 PDT
Please factor out the auto-password changes into a separate patch in a new bug. As I explained above, they won't be necessary after we fix bug 678341, because the key4.db file will not have to be on the SD card.
Comment 124 Alex Pakhotin (:alexp) 2011-08-11 17:10:24 PDT
Created attachment 552549 [details] [diff] [review]
UI-only patch

OK, here's basically the original patch for this bug, which only implements the master password preference and simple UI.
Can we go with this?

Mark, could you please have another quick look just to make sure it's fine.

I'll file a separate bug for the auto-password feature.
Comment 125 Alex Pakhotin (:alexp) 2011-08-11 17:14:11 PDT
Comment on attachment 552549 [details] [diff] [review]
UI-only patch

Brian, do you have any comments on this?
If you insist on refactoring to avoid duplication with parts of the code in PSM's password.js, let's have that in a separate bug, but let's be clear about what exactly needs to be done there.
Comment 126 Wesley Johnston (:wesj) 2011-08-15 13:18:46 PDT
Comment on attachment 552549 [details] [diff] [review]
UI-only patch

Review of attachment 552549 [details] [diff] [review]:
-----------------------------------------------------------------

This has bitrotted a bit, but the fixes were fairly easy. Looks good here! Thanks Alex!

::: mobile/chrome/content/MasterPasswordUI.js
@@ +47,5 @@
> +    }
> +    return false;
> +  },
> +
> +  show: function mp_show(aValue) {

Nit: Can we rename aValue to something more informative? Perhaps aType and then a check for aType == "create". Or even just aCreate would be better for me.

@@ +59,5 @@
> +    BrowserUI.pushPopup(this, this._dialog);
> +
> +    if (aValue) {
> +      this.checkPassword();
> +      document.getElementById("masterpassword-newpassword1").focus();

We should focus the textbox for the removepassword dialog as well I think.
Comment 127 Alex Pakhotin (:alexp) 2011-08-15 17:27:26 PDT
Created attachment 553317 [details] [diff] [review]
UI-only patch v2

Review comments addressed. Updated to the latest tree.
r=wesj
Comment 128 Brad Lassey [:blassey] (use needinfo?) 2011-08-15 22:58:54 PDT
http://hg.mozilla.org/mozilla-central/rev/2630af781762
Comment 129 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-18 06:03:19 PDT
is c-n still needed for anything here?
Comment 130 Aaron Train [:aaronmt] 2011-08-18 07:02:46 PDT
Verified Fixed
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110818 Firefox/9.0a1 Fennec/9.0a1
Comment 131 Matt Brubeck (:mbrubeck) 2011-08-18 08:32:21 PDT
(In reply to Marco Bonardo [:mak] from comment #129)
> is c-n still needed for anything here?

No.
Comment 132 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-05-01 08:06:45 PDT
removing secr:bsmith to retriage this bug

Note You need to log in before you can comment on or make changes to this bug.