Last Comment Bug 757646 - Passwords Content Provider silently returns empty strings for username and password if Master Password is enabled
: Passwords Content Provider silently returns empty strings for username and pa...
Status: RESOLVED FIXED
: dataloss
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 critical (vote)
: Firefox 15
Assigned To: Wesley Johnston (:wesj)
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 759022 (view as bug list)
Depends on:
Blocks: 711636
  Show dependency treegraph
 
Reported: 2012-05-22 15:55 PDT by matthew zeier [:mrz]
Modified: 2012-05-29 12:40 PDT (History)
18 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
betaN+


Attachments
sync log 1 (96.64 KB, text/plain)
2012-05-22 15:55 PDT, matthew zeier [:mrz]
no flags Details
sync log 2 (12.01 KB, text/plain)
2012-05-22 15:55 PDT, matthew zeier [:mrz]
no flags Details
Patch (2.39 KB, patch)
2012-05-23 16:42 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2 Parts 1+2 (7.71 KB, patch)
2012-05-23 17:34 PDT, Wesley Johnston (:wesj)
rnewman: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Test Patch (7.86 KB, patch)
2012-05-24 15:32 PDT, Wesley Johnston (:wesj)
rnewman: review+
gbrown: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description matthew zeier [:mrz] 2012-05-22 15:55:32 PDT
Created attachment 626225 [details]
sync log 1

Running Aurora 14.0a2 (2012-05-22).

Started noticing that all my saved usernames & passwords disappear.  I reverted to a known working Profile.

On first startup, I can see all usernames/passwords in Password Manager.  It appears that as soon as Sync is done, I'm left with nothing.

I made the following changes to capture debug:
* signon.debug TRUE
* services.sync.log.appender.file.logOnSuccess TRUE

signon.debug generated this error:
1337726115193	Sync.Engine.Tabs	WARN	DATA LOSS: Both local and remote changes to record: p1a6KkVZcVag


I've attached two logs from the latter.
Comment 1 matthew zeier [:mrz] 2012-05-22 15:55:50 PDT
Created attachment 626226 [details]
sync log 2
Comment 2 matthew zeier [:mrz] 2012-05-22 15:59:57 PDT
I'll add that I forced a Sync reset and still had this issue.
Comment 3 Justin Dolske [:Dolske] 2012-05-22 16:11:29 PDT
To add some context:

I sat down with mrz earlier to look at what was going on.

One interesting bit is that the login entries still exist, but many have a blank username/password. (EG, in the password manager UI, you just see a hostname with the other two columns blank). The signon.debug output didn't show any errors when bringing up the window, so that appears to be the actual data. (CPD hadn't been used on mobile -- so bug 756701 isn't involved here. It should have different symptoms anyway.)

Restoring signons.sqlite locally put things back in a good state. But watching the error console's logging, a sync quickly started and did a _lot_ of pwmgr calls. The console was flooded with pwmgr search/add/modify requests (and probably removes, but I didn't happen to catch any). And after that stopped, again many entries had missing usernames and/or passwords.

That Sync.Engine.Tabs error was logged right afterwards, I've no idea what it means or if it's related.

So, it looks to me like sync is somehow confused and is partially clobbering stuff.
Comment 4 Richard Newman [:rnewman] 2012-05-22 17:43:46 PDT
Well, this might shed some additional light on that Google Play review...
Comment 5 Richard Newman [:rnewman] 2012-05-22 18:37:30 PDT
Questions:

* Are you syncing with Android? If so, what version?
* Do you have Master Password enabled, either on desktop or your phone?
* Please open a new tab, go to about:config, choose Tools > Web Console, and paste the following:

let collection = "passwords";
Components.utils.import("resource://services-sync/main.js");
Components.utils.import("resource://services-sync/record.js");
let recordType = Weave.Engines.get(collection)._recordObj;
let coll = new Collection(Weave.Service.storageURL + collection, recordType);
coll.full = true;
coll.limit = 50;
coll.recordHandler = function(item) {
  item.collection = collection;
  item.decrypt();
  console.log(item.cleartext);
};
coll.get();

This'll print 50 of your password records to the Web Console. Please take a look and see if you see any that are unusual -- for example, with empty or missing username and password fields.

If you have any like that, feel free to send them to me.
Comment 6 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-05-22 21:45:17 PDT
FWIW, I had this happen to me this last week too.  I coincidentally ran out of disk space on my laptop with Firefox running, and was assuming that caused it (and sync just propagated it).  Restoring a full profile from backup just let sync re-erase them all.  I let it sync, then restored just signons.sqlite and key3.db from the backup and then forced a sync reset with "overwrite everyone else" from the laptop that just got restored, and the problem hasn't recurred since (It's only been a couple days though).  I had identical symptoms when it happened though, all the sites were still there, just the usernames and passwords disappeared.
Comment 7 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-05-22 21:56:06 PDT
To answer questions from comment 5:

On my Android 2.3.4 HTC Evo 3D phone I have Firefox Beta *v13* (the last XUL version) which I have purposely not upgraded because the password autofill still works on it (I've never been able to get password autofill to work on the NativeUI versions).  I also have Nightly (v15) on this device.  At the point I get password autofill working on Nightly I will remove Beta (because I know two NativeUI versions can't coexist and still sync).

On my Android 3.2 Galaxy Tab 10.1 I have only Nightly (v15).

I also sync with my Mac OS X 10.7.3 laptop (Aurora v14) and two desktop machines (Release v12 on both) that are also Max OS X 10.7.3.  I also sync with Release v12 in an Ubuntu 12.04 VM.

Every synced device I own has a master password set.  They're not all set to the same password (my mobile devices [1 phone, 1 tablet] have one password, the desktops/laptops all have another).

The js snippet into the web console doesn't currently produce any blanks - I haven't had the problem recur since my backup restore/sync reset.
Comment 8 matthew zeier [:mrz] 2012-05-22 22:04:07 PDT
I now have a third state.  Because not having passwords really sucks, I did the following:

* Stop Fx, restore working Profile directory
* Start Fx, don't enter master password
* Download Password Exporter, export all passwords
* Enter master password, allow Sync to sync, notice missing usernames/passwords
* Import passwords
* Delete 800 some empty entries

It's been running for over an hour and I still have my passwords.

I now have three Profile directories:
1. May 15 working Profile with passwords
2. Profile with empty passwords
3. Profile with imported passwords

I can revert to any one of those to help troubleshoot.  Web console doesn't currently show anything.
Comment 9 Richard Newman [:rnewman] 2012-05-22 22:14:01 PDT
(In reply to Dave Miller [:justdave] from comment #7)

Thanks for the thorough answers, Dave.

> Every synced device I own has a master password set.  They're not all set to
> the same password (my mobile devices [1 phone, 1 tablet] have one password,
> the desktops/laptops all have another).

If you have a master password set on Native Fennec, I have No Idea Whatsoever® what will happen with password sync.
Comment 10 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-05-22 23:34:03 PDT
I just managed to reproduce this.

1) removed the master password on my tablet
2) removed the sync account on the tablet
3) re-added the sync account and paired it.

voila, passwords disappeared off the desktops.

Approximately the first 50 passwords remained intact, everything after that in the file is gone except for the site names.
Comment 11 Gregory Szorc [:gps] 2012-05-23 02:47:11 PDT
(In reply to Dave Miller [:justdave] from comment #10)
> Approximately the first 50 passwords remained intact, everything after that
> in the file is gone except for the site names.

This is most troubling. I would love to see a log for this.

My read of this bug is that Android Sync is likely involved. If you can reproduce this on a Sync account with just desktop Firefox instances, I would love to see steps to reproduce.
Comment 12 Richard Newman [:rnewman] 2012-05-23 08:15:20 PDT
I would still love to see mrz's answers to my questions in Comment 5. But change that '50' to '100' per Comment 10.
Comment 13 Richard Newman [:rnewman] 2012-05-23 08:17:51 PDT
If this does come down to Android, there are two curiosities. Firstly, why are records being reuploaded, and secondly why does some subset end up empty?

I really hope this isn't the shaky NSS bridge/CP combo falling over, but I guess we'll see!
Comment 14 matthew zeier [:mrz] 2012-05-23 08:29:56 PDT
(In reply to Richard Newman [:rnewman] from comment #5)
> Questions:
> 
> * Are you syncing with Android? If so, what version?

Nexus 4.0.4, Nightly 2012-05=18.

> * Do you have Master Password enabled, either on desktop or your phone?

Yes, on both.  Different passwords.

> * Please open a new tab, go to about:config, choose Tools > Web Console, and
> paste the following:
> 

> This'll print 50 of your password records to the Web Console. Please take a
> look and see if you see any that are unusual -- for example, with empty or
> missing username and password fields.

I have a number that show something like:

[08:27:44.631] ({id:"{073a10dd-a8c2-6c41-a5f6-0576def1bbc5}", deleted:true}) @ Web Console:4

This is on the "profile + password import".  Should I repeat with the other profiles?

I'm in SF today if anyone wants my laptop.
Comment 15 Richard Newman [:rnewman] 2012-05-23 08:35:50 PDT
Thanks mrz. So this looks like it might be a Fennec master password issue. I'll poke at that when I'm back in front of a proper keyboard. No need to repeat that test elsewhere; it's looking at the server, not the local storage. But repeating with '100' in place of that 50 limit might be interesting, after Dave's comment.
Comment 16 matthew zeier [:mrz] 2012-05-23 09:49:55 PDT
Sorry, meant to say - I ran that with 100.
Comment 17 Tracy Walker [:tracy] 2012-05-23 12:31:45 PDT
I'm unable to reproduce with Android Fxb1 on mobile connected to nightly on desktop.

Tried comment 10.  Passwords remain intact on desktop.

Tried entering MP, sync devices. Tried not entering MP, sync devices.  Passwords remain intact.


Oh, just saw justdaves comment that he has XUL Fennec not Android.

mrz, wht about you,  XUL or Android?
Comment 18 matthew zeier [:mrz] 2012-05-23 13:37:59 PDT
I'm on native UI.
Comment 19 Justin Dolske [:Dolske] 2012-05-23 14:49:34 PDT
The master password seems likely to be involved, at least that would explain the curiosity of how just the username/password are getting cleared... Those are the only 2 fields in a login entry that are encrypted. Perhaps a bug with assuming that an entry can be decrypted, but it actually fails if the MP hasn't be entered yet?
Comment 20 Richard Newman [:rnewman] 2012-05-23 15:29:14 PDT
(In reply to Justin Dolske [:Dolske] from comment #19)
> The master password seems likely to be involved, at least that would explain
> the curiosity of how just the username/password are getting cleared... Those
> are the only 2 fields in a login entry that are encrypted. Perhaps a bug
> with assuming that an entry can be decrypted, but it actually fails if the
> MP hasn't be entered yet?

Android Sync doesn't know anything about the MP, so if Fennec is responding to a query with failed crypto operations, sadness will result.

I'm going to see if I can get some pseudo-automated test to reproduce badness.
Comment 21 Richard Newman [:rnewman] 2012-05-23 16:27:06 PDT
I just wrote a very trivial test to do the following:

* Grab the Fennec CP. Insert a password.
* Verify that I get the same out from a query.
* Manually launch Fennec, turn on MP, quit.
* Re-run the verification step.

We silently get an empty string for username and password.

So: anyone whose Android Sync client attempts to upload records when master password is enabled will silently erase those username/password pairs, and those changes will propagate to other clients.

I was under the impression that the CP would throw an exception in this case, but apparently not.

So let's think about how we're going to detect this situation. Will almost certainly involve a Fennec change.
Comment 22 Wesley Johnston (:wesj) 2012-05-23 16:33:35 PDT
We should throw if MP is enabled. I'm guessing that we're catching the error instead of propagating it to the sync client..... Let me look....
Comment 23 Richard Newman [:rnewman] 2012-05-23 16:35:49 PDT
Let's morph this for the Fennec part, and if there is additional Sync stuff required, I'll file and fix.
Comment 24 Wesley Johnston (:wesj) 2012-05-23 16:42:00 PDT
Created attachment 626630 [details] [diff] [review]
Patch

I'm gonna have to test this locally some, but wanted to move this fast.
Comment 25 Wesley Johnston (:wesj) 2012-05-23 16:57:04 PDT
Installed Fennec
Synced
Watch logcat for passwords to start (I have a lot of fake passwords in my sync account so this takes a long time)
While syncing, turned on MP
Logcat has some of errors now. No crashing

05-23 16:54:25.328  3915  3921 E NSSBridge: Error encrypting 
05-23 16:54:25.328  3915  3921 E NSSBridge: java.lang.Exception: PK11SDR_Encrypt returned error -8152: The key does not support the requested operation.
05-23 16:54:25.328  3915  3921 E NSSBridge: 
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at org.mozilla.gecko.NSSBridge.nativeEncrypt(Native Method)
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at org.mozilla.gecko.NSSBridge.encrypt(NSSBridge.java:25)
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at org.mozilla.fennec_wesj.db.PasswordsProvider.doCrypto(PasswordsProvider.java:209)
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at org.mozilla.fennec_wesj.db.PasswordsProvider.onPreInsert(PasswordsProvider.java:230)
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at org.mozilla.fennec_wesj.db.GeckoProvider.insert(GeckoProvider.java:265)
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at android.content.ContentProvider$Transport.insert(ContentProvider.java:198)
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at android.content.ContentProviderNative.onTransact(ContentProviderNative.java:167)
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at android.os.Binder.execTransact(Binder.java:336)
05-23 16:54:25.328  3915  3921 E NSSBridge: 	at dalvik.system.NativeStart.run(Native Method)
05-23 16:54:25.336  3915  3921 E JavaBinder: *** Uncaught remote exception!  (Exceptions are not yet supported across processes.)
05-23 16:54:25.336  3915  3921 E JavaBinder: java.lang.RuntimeException: java.lang.Exception: PK11SDR_Encrypt returned error -8152: The key does not support the requested operation.
05-23 16:54:25.336  3915  3921 E JavaBinder: 
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at org.mozilla.gecko.NSSBridge.encrypt(NSSBridge.java:28)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at org.mozilla.fennec_wesj.db.PasswordsProvider.doCrypto(PasswordsProvider.java:209)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at org.mozilla.fennec_wesj.db.PasswordsProvider.onPreInsert(PasswordsProvider.java:230)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at org.mozilla.fennec_wesj.db.GeckoProvider.insert(GeckoProvider.java:265)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at android.content.ContentProvider$Transport.insert(ContentProvider.java:198)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at android.content.ContentProviderNative.onTransact(ContentProviderNative.java:167)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at android.os.Binder.execTransact(Binder.java:336)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at dalvik.system.NativeStart.run(Native Method)
05-23 16:54:25.336  3915  3921 E JavaBinder: Caused by: java.lang.Exception: PK11SDR_Encrypt returned error -8152: The key does not support the requested operation.
05-23 16:54:25.336  3915  3921 E JavaBinder: 
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at org.mozilla.gecko.NSSBridge.nativeEncrypt(Native Method)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	at org.mozilla.gecko.NSSBridge.encrypt(NSSBridge.java:25)
05-23 16:54:25.336  3915  3921 E JavaBinder: 	... 7 more
05-23 16:54:25.351  3616  3925 W SynchronizerSession: First RecordsChannel onFlowStoreFailed. Reporting store error.
05-23 16:54:25.351  3616  3925 W SynchronizerSession: android.os.RemoteException
05-23 16:54:25.351  3616  3925 W SynchronizerSession: 	at org.mozilla.gecko.sync.repositories.android.PasswordsRepositorySession.insert(PasswordsRepositorySession.java:450)
05-23 16:54:25.351  3616  3925 W SynchronizerSession: 	at org.mozilla.gecko.sync.repositories.android.PasswordsRepositorySession$5.run(PasswordsRepositorySession.java:342)
05-23 16:54:25.351  3616  3925 W SynchronizerSession: 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1088)
05-23 16:54:25.351  3616  3925 W SynchronizerSession: 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:581)
05-23 16:54:25.351  3616  3925 W SynchronizerSession: 	at java.lang.Thread.run(Thread.java:1019)
05-23 16:54:25.359  3616  3925 I ServerSyncStage: onSynchronizeAborted.
05-23 16:54:25.359  1325  1481 I ActivityManager: No longer want com.noshufou.android.su (pid 2223): hidden #21
05-23 16:54:25.367  3616  3925 W GlobalSession: Aborting sync: Synchronization was aborted.
05-23 16:54:25.367  3616  3925 I GlobalSession: Not uploading updated meta/global record since there are no engines requesting upload.
05-23 16:54:25.367  3616  3925 I SyncAdapter: GlobalSession indicated error. Flagging auth token as invalid, just in case.
05-23 16:54:25.391  3616  3925 I SyncAdapter: Notifying sync monitor.
05-23 16:54:25.391  3616  3925 W GlobalSession: Aborting sync: Got store error.
05-23 16:54:25.391  3616  3925 W GlobalSession: android.os.RemoteException
05-23 16:54:25.391  3616  3925 W GlobalSession: 	at org.mozilla.gecko.sync.repositories.android.PasswordsRepositorySession.insert(PasswordsRepositorySession.java:450)
05-23 16:54:25.391  3616  3925 W GlobalSession: 	at org.mozilla.gecko.sync.repositories.android.PasswordsRepositorySession$5.run(PasswordsRepositorySession.java:342)
05-23 16:54:25.391  3616  3925 W GlobalSession: 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1088)
05-23 16:54:25.391  3616  3925 W GlobalSession: 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:581)
05-23 16:54:25.391  3616  3925 W GlobalSession: 	at java.lang.Thread.run(Thread.java:1019)
05-23 16:54:25.391  3616  3925 I GlobalSession: Not uploading updated meta/global record since there are no engines requesting upload.
05-23 16:54:25.391  3616  3925 I SyncAdapter: GlobalSession indicated error. Flagging auth token as invalid, just in case.
05-23 16:54:25.398  3616  3925 I SyncAdapter: Notifying sync monitor.
Comment 26 Nick Alexander :nalexander (leave until January 2017) 2012-05-23 16:57:56 PDT
Comment on attachment 626630 [details] [diff] [review]
Patch

Throwing RuntimeException is definitely better than silently eating the Exception, but I would rather we throw the actual exception or explicit checked encrypt/decrypt failure exceptions.
Comment 27 Richard Newman [:rnewman] 2012-05-23 16:58:48 PDT
Comment on attachment 626630 [details] [diff] [review]
Patch

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

::: mobile/android/base/NSSBridge.java
@@ +18,1 @@
>      static public String encrypt(Context context, String aValue) {

I'd rather just propagate the exception here, rather than rethrowing as a RuntimeException. The NSSBridge crypto operations are specified as throwing java.lang.Exception. Then the callers of these can be adjusted to do the right thing -- for example, trying out an encrypt() call prior to instantiating the CP. Much neater if that's not catching a RuntimeException.

@@ +23,5 @@
>          try {
>              String path = GeckoProfile.get(context).getDir().toString();
>              res = nativeEncrypt(path, aValue);
> +        } catch(Exception ex) {
> +            Log.e(LOGTAG, "Error encrypting ", ex);

Before we commit to dumping one error log line, regardless of logging preferences, I'd love to know what's in that exception. If it includes private data I'd rather sanitize it with more specific handling.

(And of course, move this kind of handling into PasswordsProvider. And nit: space before (Exception ex).)
Comment 28 Richard Newman [:rnewman] 2012-05-23 17:01:23 PDT
(In reply to Wesley Johnston (:wesj) from comment #25)

> 05-23 16:54:25.336  3915  3921 E JavaBinder: *** Uncaught remote exception! 
> (Exceptions are not yet supported across processes.)

*sigh*

(In reply to Nick Alexander :nalexander from comment #26)
> ... or explicit
> checked encrypt/decrypt failure exceptions.

We don't get to adjust the CP API, unfortunately.

All I'm hoping for here is that we can get a neat exception thrown within the CP, and then have the CP do the best it can with the API that Android exposes -- failing to create the CP instance if it can, and otherwise throwing/returning what it can across the wire.
Comment 29 Wesley Johnston (:wesj) 2012-05-23 17:34:19 PDT
Created attachment 626652 [details] [diff] [review]
Patch v2 Parts 1+2

This throws Checked Exceptions from the bridge, catches them and logs a generic message in the PasswordProvider, and then "rethrows" them for Sync.

Also added a check in onCreate that return false there if NSS fails. We create the provider on Fennec startup, so I worry this will affect startup.... Maybe better as a follow up.

Also removed other logging that is leftover from long ago.
Comment 30 Wesley Johnston (:wesj) 2012-05-23 19:42:32 PDT
Possible data loss. This should block.
Comment 31 Johnathan Nightingale [:johnath] 2012-05-24 07:20:31 PDT
(In reply to Wesley Johnston (:wesj) from comment #30)
> Possible data loss. This should block.

I agree - and putting it on the betaN list, too.
Comment 32 Tracy Walker [:tracy] 2012-05-24 08:23:50 PDT
Now that I have the correct build, I can confirm that uploads/downloads of passwords, that is, new or changes to them, are not happening with MP enabled on Mobile.  Data synced between devices prior to setting MP on mobile remains intact. 

Also any new or changed passwords that sync touched while MP was on continue to not sync after MP is disabled.
Comment 33 Richard Newman [:rnewman] 2012-05-24 09:29:22 PDT
Comment on attachment 626652 [details] [diff] [review]
Patch v2 Parts 1+2

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

This looks good to me with nits addressed. Thanks, wesj.

I'm going to spend some time this morning verifying behavior in the wider context, and will file a follow-up for Sync if we can be a bit more graceful.

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +100,5 @@
>          setDBVersion(DB_VERSION);
> +        try {
> +            NSSBridge.encrypt(getContext(), "bacon");
> +        } catch (Exception ex) {
> +            Log.e(getLogTag(), "Error starting crypto for PasswordsProvider");

Third argument, ex.

@@ +214,5 @@
> +            if (profilePath != null) result = NSSBridge.encrypt(mContext, profilePath, initialValue);
> +            else result = NSSBridge.encrypt(mContext, initialValue);
> +          } else {
> +            if (profilePath != null) result = NSSBridge.decrypt(mContext, profilePath, initialValue);
> +            else result = NSSBridge.decrypt(mContext, initialValue);            

Would love to fix the style here, if you're breaking blame.

@@ +217,5 @@
> +            if (profilePath != null) result = NSSBridge.decrypt(mContext, profilePath, initialValue);
> +            else result = NSSBridge.decrypt(mContext, initialValue);            
> +          }
> +        } catch (Exception ex) {
> +            Log.e(getLogTag(), "Error in NSSBridge");

Again, exception logging.
Comment 35 Wesley Johnston (:wesj) 2012-05-24 09:54:22 PDT
Comment on attachment 626652 [details] [diff] [review]
Patch v2 Parts 1+2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 718760
User impact if declined: Users with master password enabled can have their passwords wiped.
Testing completed (on m-c, etc.): Landed on mc toady, may 24. Tested locally by me and a two other people in this bug. Will continue to test more locally today.
Risk to taking this patch (and alternatives if risky): Simple change. Makes us throw from the passwords provider when we can't encrypt. The exception is logged and caught by Sync. Should be safe, but there's some risk.
String or UUID changes made by this patch: None.
Comment 36 Richard Newman [:rnewman] 2012-05-24 11:16:26 PDT
Behavior with this patch: Android can't throw exceptions across process boundaries, so it returns null instead. Sync detects that on insert, fakes a RemoteException, and aborts the sync.

I did not see exceptions being raised when the CP client is requested.

The sadness is that in aborting the sync, we don't continue on, so only tabs will be synced until the user deactivates MP. (Bookmarks etc. all follow passwords.)

Unfortunately, even with MP unlocked, the password provider throws an exception. That implies two things:

* We need a neat way to detect this and skip password sync, above and beyond not doing bad things
* We need a way to have the password provider function when MP is unlocked.
Comment 37 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-24 12:13:22 PDT
Comment on attachment 626652 [details] [diff] [review]
Patch v2 Parts 1+2

waiting for an updated patch
Comment 38 Wesley Johnston (:wesj) 2012-05-24 12:17:07 PDT
Backed this out because it causes a failure in the passwordsProvider tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dcce3a259f1
Comment 39 Wesley Johnston (:wesj) 2012-05-24 15:32:02 PDT
Created attachment 626994 [details] [diff] [review]
Test Patch

OK. So I don't think getContentResolver cares if I return false from onCreate in the ContentProvider, leaving me with no way to know that it failed in the tests. I was seeing problems locally because of that I think. I also talked to mark and he agreed that, since the ContentProvider gets created at startup, we want to be careful about doing more in onCreate than is necessary. So, I removed that check.

I also added a test to make sure we return null when this fails. Wanted to check that it threw, but since we can't throw across processes that doesn't exactly work. Had to add some methods to send messages to Gecko for that test, so asking geoff to review them.
Comment 40 Geoff Brown [:gbrown] 2012-05-24 16:24:22 PDT
Comment on attachment 626994 [details] [diff] [review]
Test Patch

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

Looks fine from the Robocop perspective.

::: mobile/android/base/tests/testPasswordEncrypt.java.in
@@ +118,5 @@
> +            jsonPref.put("name", "privacy.masterpassword.enabled");
> +            jsonPref.put("type", "string");
> +            jsonPref.put("value", passwd);
> +            mActions.sendGeckoEvent("Preferences:Set", jsonPref.toString());
> +        } catch (Exception ex) { }

Is this exception expected / normal? If not, we usually report it with something like:

      } catch(Exception ex) {
          mAsserter.ok(false, "Caught exception", ex.toString());
Comment 41 Richard Newman [:rnewman] 2012-05-24 18:28:21 PDT
Comment on attachment 626994 [details] [diff] [review]
Test Patch

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

Looks good (modulo my limited understanding of Robocop!). r=me with nits.

::: build/mobile/robocop/Actions.java.in
@@ +29,5 @@
>          public void blockUntilClear(long millis);
>      }
>  
>      /**
> +     * Sends an event to gecko.

Nit: capital G.

::: mobile/android/base/tests/testPasswordEncrypt.java.in
@@ +84,5 @@
>            mAsserter.is(decryptedP, "password2", "Password was encrypted when updating");
> +
> +          // trying to store a password while master password is enabled should throw
> +          // but because Android can't send exceptions across processes
> +          // it just results in a null uri being returned

Nit: punctuation and capitalization.

@@ +88,5 @@
> +          // it just results in a null uri being returned
> +          toggleMasterPassword("password");
> +          try {
> +              uri = cr.insert(passwordUri, cvs);
> +              mAsserter.is(uri, null, "Storing a password while MP was set should fail");

Can we also assert that cr.query returns a null cursor?

@@ +89,5 @@
> +          toggleMasterPassword("password");
> +          try {
> +              uri = cr.insert(passwordUri, cvs);
> +              mAsserter.is(uri, null, "Storing a password while MP was set should fail");
> +          } catch (Exception ex) {

Log.
Comment 42 Richard Newman [:rnewman] 2012-05-24 18:31:54 PDT
For the record, I filed Bug 758382 to allow the sync to continue after password sync fails.

Right now Sync will essentially be broken for users with MP enabled. (I view that as an acceptable tradeoff when compared to destroying their saved passwords, so I don't suggest waiting for that bug.)
Comment 43 :Ms2ger (⌚ UTC+1/+2) 2012-05-25 08:33:39 PDT
https://hg.mozilla.org/mozilla-central/rev/01f48ef1ee92
Comment 44 Wesley Johnston (:wesj) 2012-05-25 10:05:42 PDT
Was previously backed out. Reopening and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/53aaa1c7823a
Comment 45 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-05-25 23:08:27 PDT
OK, so to get some clarity on this issue for mrz and I, as end users of the product:

Syncing passwords with a master password set apparently works fine on XUL Fennec.  This is the reason I keep the last beta that was still XUL on my phone, because my password autofill actually works there.

Syncing passwords on Native Fennec apparently hasn't ever worked with a Master Password set.  Although I'm wondering why it's only been in the last 2 or 3 weeks that it suddenly started propagating a full set of cleared passwords back to my other clients...

If I understand this correctly, passwords should actually work in Native Fennec if I had Master Password disabled prior to linking it to a Sync account?
Comment 46 Wesley Johnston (:wesj) 2012-05-26 00:14:08 PDT
Passwords won't be synced to Native Fennec if Master Password is enabled. You can disable Master Password, and while its disabled password syncing will work. When you turn it back on, it will no longer work (but any passwords synced during the time MP was disabled will remain). This is just a trade off we had to make for now.
Comment 47 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-05-26 00:27:13 PDT
so you're basically telling me that the least-physically-secure device I have no longer has a way to secure my passwords from someone who physically obtains access to it...
Comment 48 Ryan VanderMeulen [:RyanVM] 2012-05-26 05:14:35 PDT
https://hg.mozilla.org/mozilla-central/rev/53aaa1c7823a
Comment 49 Wesley Johnston (:wesj) 2012-05-26 08:27:36 PDT
(In reply to Dave Miller [:justdave] from comment #47)
> so you're basically telling me that the least-physically-secure device I
> have no longer has a way to secure my passwords from someone who physically
> obtains access to it...

This discussion should be moved somewhere else. Master Password is there if you want it. It just doesn't work happily with Sync. We also store your passwords database on Android's heavily sandboxed internal storage where no one has access to it except Fennec.

My personal opinion: Master passwords can be hacked. Android's sandboxing, combined with putting a password lock on your phone is far far FAR stronger security measure and we use it for every single one of our users without requiring any intervention on their part.
Comment 50 matthew zeier [:mrz] 2012-05-26 10:39:48 PDT
Ignoring your suggestion that discussion move elsewhere...

My kids know how to unlock my phone (you try waiting for a table with an hour wait without giving your kids your phone) and they could launch Fennec.  But they can't get to anything that requires my login because they don't know my Master Password.

You are safe, for instance, from my kids deleting your Mozilla LDAP account.
Comment 51 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-05-26 10:40:20 PDT
(In reply to Wesley Johnston (:wesj) from comment #49)
> (In reply to Dave Miller [:justdave] from comment #47)
> > so you're basically telling me that the least-physically-secure device I
> > have no longer has a way to secure my passwords from someone who physically
> > obtains access to it...
> 
> This discussion should be moved somewhere else.

Already on bug 540975, this situation is just a good reminder of it.
Comment 52 Richard Newman [:rnewman] 2012-05-26 16:06:01 PDT
(In reply to Dave Miller [:justdave] from comment #45)

> Syncing passwords on Native Fennec apparently hasn't ever worked with a
> Master Password set.

Correct; it wasn't identified by product as a beta/1.0 requirement.

> Although I'm wondering why it's only been in the last
> 2 or 3 weeks that it suddenly started propagating a full set of cleared
> passwords back to my other clients...

Password sync only landed enabled in Nightlies in mid-April.

> If I understand this correctly, passwords should actually work in Native
> Fennec if I had Master Password disabled prior to linking it to a Sync
> account?

If Sync doesn't need to read or write passwords from/to Fennec (that is, your password store is stable on each device), then it won't upload blank records. And if you synced passwords before enabling MP, the ones that are already there will continue to exist.

(In reply to Wesley Johnston (:wesj) from comment #46)
> Passwords won't be synced to Native Fennec if Master Password is enabled.
> You can disable Master Password, and while its disabled password syncing
> will work. When you turn it back on, it will no longer work (but any
> passwords synced during the time MP was disabled will remain). This is just
> a trade off we had to make for now.

Furthermore, Sync will abort when passwords fail to sync, so if you have MP enabled and password sync checked then you pretty much only have tab sync on Android. Bug 758382 will address this.
Comment 53 Joe Drew (not getting mail) 2012-05-28 11:56:31 PDT
Can the obsolete patches in this bug be marked as obsolete, and the relevant patches be nommed? This is a betaN blocker, so we'd like to get it in to aurora ASAP.
Comment 54 Wesley Johnston (:wesj) 2012-05-28 17:03:39 PDT
Comment on attachment 626652 [details] [diff] [review]
Patch v2 Parts 1+2

Renoming, but I'm nervous to push this until its baked a bit and QA has had a turn to verify the fix. Also the Aurora approval bits didn't show up...

Fixes possible data loss. Been baking on MC since 5-25 (3 days ago). Fix is fairly safe and has been manually tested by multiple people.
Comment 55 Wesley Johnston (:wesj) 2012-05-28 17:05:03 PDT
Comment on attachment 626994 [details] [diff] [review]
Test Patch

Must be pushed folded with the previous patch. Adds+fixes some tests and removes a change in that patch that will negatively affect startup (and causes tests to fail).

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Comment 56 Aaron Train [:aaronmt] 2012-05-29 07:15:35 PDT
*** Bug 759022 has been marked as a duplicate of this bug. ***
Comment 57 Wesley Johnston (:wesj) 2012-05-29 12:37:33 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/8b7b85eded3f

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