1171687 - test_logins_decrypt_failure.js fails on Android

Status: NEW

(Toolkit :: Password Manager, defect, P5)

Description

[Matthew N. [:MattN]]

Attachment: Test log

+++ This bug was initially created as a clone of Bug #1170772 +++

Bug 1170772 is enabling the xpcshell test directory on Android but test_storage_mozStorage.js and test_logins_decrypt_failure.js fail and need to be fixed before enabling.

(Quoting Geoff Brown [:gbrown] from bug 1170772 comment #3)
> test_storage_mozStorage.js and test_logins_decrypt_failure.js fail on
> Android:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d1affc45dad
> 
> http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-
> 4d1affc45dad/try-android-api-11/try_ubuntu64_vm_armv7_mobile_test-xpcshell-3-
> bm122-tests1-linux64-build99.txt.gz

Comment 1

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c6a9ad508b2

Comment 2

[Nick Alexander :nalexander [he/him]]

dkeeler: I started digging into these tickets, motivated by replacing password storage on Android.  (My thinking: we should make the existing tests work for the existing storage, and then make the existing tests work for the replacement storage.  Sadly, I didn't realize that the "storage" tests are really about upgrading and downgrading storage-specific SQLite DBs, so not germane to replacement storage.  Sigh.)  In any case, I think the try pushes will address test_storage_mozStorage.js, and I'll flag you for review when they're green.

However, I can't make progress on test_logins_decrypt_failure.js.  What is happening is that `token.reset()` followed by `token.changePassword("", "")` fails around http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/toolkit/components/passwordmgr/test/unit/test_logins_decrypt_failure.js#22.  I have tried `token.initPassword("")` as well.

There are similar tests that are disabled on Android due to something about smartcards which I don't understand; specifically http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/security/manager/ssl/tests/unit/test_pkcs11_token.js#80.

It's not clear to me that this flow that invokes `token.reset()` _should_ work on Android, and we definitely don't invoke it in Fennec's Master Password support.  Can you provide some light on this test, whether it should work on Android, and whether the usage of the internal token API is even correct?  Thanks!

Comment 3

[Nick Alexander :nalexander [he/him]]

I have a green X8 job at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bed995b6c23c07254885ebb0a58c8f779f5a11f&selectedJob=108295051 with a log containing

[task 2017-06-19T19:18:34.368082Z] 19:18:34     INFO -  TEST-PASS | toolkit/components/passwordmgr/test/unit/test_storage_mozStorage.js | took 13364ms

and it looks like the Android x86 job is passing too.  \o/

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

As far as I can tell, the issue is that token.reset() deletes the table "nssPrivate" in key4.db: https://dxr.mozilla.org/mozilla-central/rev/95543bdc59bd038a3d5d084b85a4fec493c349ee/security/nss/lib/softoken/sdb.c#1625
This table is only ever created when the user cert/key DB is loaded, which for Firefox basically means NSS initialization: https://dxr.mozilla.org/mozilla-central/rev/95543bdc59bd038a3d5d084b85a4fec493c349ee/security/nss/lib/softoken/sdb.c#2068

I did figure out a way to get NSS to reload the cert/key DB module, and by changing the changePassword to initPassword, the test does pass (with a ton of the following:  0:06.92 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_FAILURE: Couldn't decrypt string" {file: "file:///home/keeler/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/bin/components/crypto-SDR.js" line: 136}]" (which makes sense, because the backend can't decrypt the encrypted login entries).

However, this is a bit of a hack, and in any case we don't actually expose the ability to reset the key DB in this way (there *is* a resetpassword.xul, but you have to manually type it in to the location bar - no UI links to it). I'm tempted to say this test isn't relevant unless and until we want to actually expose this feature (and when we do, we'll have to actually make it work). Maybe for right now the best thing to do would be to create new, empty key DBs (one for android, one for the other platforms) and test that if you start xpcshell with them they can't decrypt the saved passwords - that would achieve most of what this test is aiming to test (without the portion that we don't support).

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1171687 - Part 1: Fix test_storage_mozStorage.

It appears that this test was failing only because it requires an
updated key database.  I did the following to produce such a key
database, following the instructions at
https://blogs.oracle.com/meena/whats-new-in-nss-312-new-shared-dbs:

mkdir nss
cp toolkit/components/passwordmgr/test/unit/data/key3.db nss/
~/.mozbuild/host-utils.37.0a2.en-US.mac/certutil -K -d sql:nss -X
cp nss/key4.db toolkit/components/passwordmgr/test/unit/data/
hg add toolkit/components/passwordmgr/test/unit/data/key4.db

With these tweaks and the examplar from
http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/security/manager/ssl/tests/unit/test_sdr_preexisting.js#176,
the test passes.

Review commit: https://reviewboard.mozilla.org/r/150612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150612/

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1171687 - Part 2: Fix running xpcshell tests that initialize NSS locally.

Review commit: https://reviewboard.mozilla.org/r/150614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150614/

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8879325 [details]
Bug 1171687 - Part 1: Fix test_storage_mozStorage.

https://reviewboard.mozilla.org/r/150612/#review155368

(dkeeler from the commit message didn't match :keeler, not sure if you still want his review too)

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8879326 [details]
Bug 1171687 - Part 2: Fix running xpcshell tests that initialize NSS locally.

https://reviewboard.mozilla.org/r/150614/#review155544

You'll probably want some test harness peer to look at this for the implementation details side of things.

Comment 9

[Geoff Brown [:gbrown]]

Comment on attachment 8879326 [details]
Bug 1171687 - Part 2: Fix running xpcshell tests that initialize NSS locally.

https://reviewboard.mozilla.org/r/150614/#review155666

Comment 10

[Matthew N. [:MattN]]

I'm landing part 1 in bug 1556117 since I want the test coverage for bug 1148205.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/807a86140aba
Fix test_storage_mozStorage. r=MattN

Comment 12

[Matthew N. [:MattN]]

(In reply to Pulsebot from comment #11)

https://hg.mozilla.org/integration/mozilla-inbound/rev/807a86140aba
Fix test_storage_mozStorage. r=MattN

and I forgot to update the bug number after filing…

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/807a86140aba