Closed Bug 978120 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
Android
defect

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mobile-testing][xpcshell])

Attachments

(2 files)

+++ 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.
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]
Summary: nsIX509CertDB.addCert fails on Android and B2G in xpcshell tests due with SEC_ERROR_TOKEN_NOT_LOGGED_IN → nsIX509CertDB.addCert and nsIX509CertDB.setCertTrust fail on Android and B2G in xpcshell tests due with SEC_ERROR_TOKEN_NOT_LOGGED_IN (0x805a1f65)
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.
(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.
Attachment #8384347 - Flags: review?(dkeeler)
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?
Attachment #8384347 - Flags: review?(dkeeler) → review+
(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 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
Attachment #8384347 - Flags: approval-mozilla-aurora?
Attachment #8384347 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Brian, were you going to take care of the uplift on this and bug 978528?
Flags: needinfo?(brian)
(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 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
Attachment #8384347 - Flags: approval-mozilla-beta?
Attachment #8384347 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Thanks David!
Flags: needinfo?(brian)
I suggest we just WONTFIX this for Firefox 29.
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #16)
> I suggest we just WONTFIX this for Firefox 29.

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