978120 - nsIX509CertDB.addCert and nsIX509CertDB.setCertTrust fail on Android and B2G in xpcshell tests due with SEC_ERROR_TOKEN_NOT_LOGGED_IN (0x805a1f65)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P3)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

The following xpcshell tests in security/manager/ssl/tests/unit fail (at least in part) because nsIX509CertDB.addCert fails with SEC_ERROR_TOKEN_NOT_LOGGED_IN. My guess is that Android and/or B2G have replaced part of the master password management with stuff that doesn't work in xpcshell, and/or the PSM code for doing this is just broken.

If somebody has run into this before and has some tips, I would very much appreciate receiving them.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The tests that fail due to this are:

test_certificate_usages.js [nsIX509CertDB.addCert]
test_cert_signatures.js [nsIX509CertDB.addCert]
test_ev_certs.js [nsIX509CertDB.addCert]
test_intermediate_basic_usage_constraints.js [nsIX509CertDB.addCert]
test_name_constraints.js [nsIX509CertDB.addCert]

test_getchain.js [nsIX509CertDB.setCertTrust]

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This is caused by NSS_DISABLE_DBM=1

In configure.in:

> case "${target}" in
>     *-android*|*-linuxandroid*)
>         if test "$CPU_ARCH" = "arm" ; then
>           USE_ARM_KUSER=1
>         fi
> 
>         NSS_DISABLE_DBM=1
>         MOZ_THEME_FASTSTRIPE=1
>         MOZ_TREE_FREETYPE=1
>         MOZ_MEMORY=1
>         MOZ_RAW=1
>         ;;
> 
> esac

In nsNSSComponent.cpp:

>  if (!profileStr.IsEmpty()) {
>    // First try to initialize the NSS DB in read/write mode.
>    SECStatus init_rv = ::mozilla::psm::InitializeNSS(profileStr.get(), false);
>    // If that fails, attempt read-only mode.
>    if (init_rv != SECSuccess) {
>      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("could not init NSS r/w in %s\n", profileStr.get()));
>      init_rv = ::mozilla::psm::InitializeNSS(profileStr.get(), true);
>    }
>    if (init_rv != SECSuccess) {
>      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("could not init in r/o either\n"));
>    }
>  }

When NSS_DISABLE_DBM=1, the first call to InitializeNSS, to initialize NSS read/write, fails. Consequently, we end up initializing NSS read-only. Then, nsIX509CertDB.addCert and nsIX509CertDB.setCertTrust will fail because the NSS database is read-only.

Still looking into *why* read-write initialization of NSS fails with NSS_DISABLE_DBM=1.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Make nsIX509Cert.setCerttrust, and nsIX509CertDB.addCert, and nsIX509CertDB2.addCertFromBase64 work on Android and B2G

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #2)
> This is caused by NSS_DISABLE_DBM=1

[snip]

> When NSS_DISABLE_DBM=1, the first call to InitializeNSS, to initialize NSS
> read/write, fails. Consequently, we end up initializing NSS read-only. 

This was a red herring due to bug 978519 that happens to result in very similar symptoms. When I actually built Gecko and NSS correctly, I encountered the real problem: The sqlite-based DB (as we use on Android and B2G) requires that the user be authenticated to the token before changing any trust objects (CERT_SetCertTrust). The dbm-based DB (as we use everywhere else) does not require the user to be authenticated to change any trust objects, at least when the user has the default empty password.

Since these nsIX509CertDB methods are only used by tests on Android and B2G, I did the simplest thing that could work: if the user hasn't authenticated yet, then authenticate the user with the empty default password. This makes the tests pass.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Enable PSM tests on Android and B2G that work now that nsIX509CertDB.addCert and nsIX509CertDB.setCertTrust

Comment 5

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

Comment on attachment 8384347 [details] [diff] [review]
Part 1: Make nsIX509Cert.setCerttrust, and nsIX509CertDB.addCert, and nsIX509CertDB2.addCertFromBase64 work on Android and B2G

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

I think this is a bummer, but I don't see a better solution right now.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +71,5 @@
> +  if (!slot) {
> +    return MapSECStatus(SECFailure);
> +  }
> +
> +  if (slot && PK11_NeedUserInit(slot)) {

Doesn't look like we need to check slot for non-nullness here, since we handle that above.

@@ +73,5 @@
> +  }
> +
> +  if (slot && PK11_NeedUserInit(slot)) {
> +    // Ignore the return value. Presumably PK11_InitPin will fail if the user
> +    // has a non-default password. (TODO: double-check!)

Did you find out the answer to this?

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #5)
> @@ +73,5 @@
> > +  }
> > +
> > +  if (slot && PK11_NeedUserInit(slot)) {
> > +    // Ignore the return value. Presumably PK11_InitPin will fail if the user
> > +    // has a non-default password. (TODO: double-check!)
> 
> Did you find out the answer to this?

I didn't check, because these functions are not used in B2G and/or Android yet. I filed bug 978120 about this, and I referenced that bug in the comments.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc843acfdbba
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e38d86e286

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8384347 [details] [diff] [review]
Part 1: Make nsIX509Cert.setCerttrust, and nsIX509CertDB.addCert, and nsIX509CertDB2.addCertFromBase64 work on Android and B2G

[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 IDL/UUID changes made by this patch:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is a long-standing issue.
User impact if declined: Less (practically no) testing of certificate verification on B2G and Android for Firefox 29.
Testing completed (on m-c, etc.): This change is covered by many of the automated tests, in particular the ones enabled by the other patch in this bug.

Risk to taking this patch (and alternatives if risky): For B2G, there is very little risk. For Android, there is a small amount of risk that an addon may be calling these functions and that the addon may not expect the change in behavior. However, there is also a good chance that this change already does exactly the right thing and that no addons will be negatively affected--particularly if the addon is properly written. Also, this may make writing some addons easier since they don't need to solve this problem themselves.

String or IDL/UUID changes made by this patch: none

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc843acfdbba
https://hg.mozilla.org/mozilla-central/rev/33e38d86e286

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Brian, were you going to take care of the uplift on this and bug 978528?

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> Brian, were you going to take care of the uplift on this and bug 978528?

Sure, I will do that in the next day or so.

I'll leave the NEEDINFO set as a reminder.

Comment 12

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

Comment on attachment 8384347 [details] [diff] [review]
Part 1: Make nsIX509Cert.setCerttrust, and nsIX509CertDB.addCert, and nsIX509CertDB2.addCertFromBase64 work on Android and B2G

This didn't get uplifted in time - requesting approval for beta.

[Approval Request Comment]
see comment 8

Comment 13

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

https://hg.mozilla.org/releases/mozilla-beta/rev/e74b6a1da573
https://hg.mozilla.org/releases/mozilla-beta/rev/d3352d36dbdf

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks David!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android xpcshell failures.
https://hg.mozilla.org/releases/mozilla-beta/rev/e5d922ae5641

https://tbpl.mozilla.org/php/getParsedLog.php?id=36678630&tree=Mozilla-Beta

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I suggest we just WONTFIX this for Firefox 29.

Comment 17

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #16)
> I suggest we just WONTFIX this for Firefox 29.

Agreed.