1311077 - TestCertDB fails to actually load any root certificates on android

Status: RESOLVED FIXED

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

Description

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

In debugging the test failure in bug 1227638, I discovered that on android, TestCertDB doesn't actually successfully load the built-in root certificates module:

Gecko   : [Main Thread]: D/pipnss nsNSSComponent::InitializeNSS
Gecko   : [Main Thread]: D/pipnss NSS Initialization beginning
Gecko   : [Main Thread]: D/pipnss NSS profile at '/data/local/tests/cppunittests/h/cpp-unit-profd'
Gecko   : [Main Thread]: D/pipnss inSafeMode: 0
Gecko   : [Main Thread]: D/pipnss LoadLoadableRoots
Gecko   : [Main Thread]: D/pipnss LoadLoadableRoots: GetPIPNSSBundleString(RootCertModuleName) failed

The above is some added debugging output. The code in question is:

void
nsNSSComponent::LoadLoadableRoots()
{
...
  nsresult rv;
  nsAutoString modName;
  rv = GetPIPNSSBundleString("RootCertModuleName", modName);
  if (NS_FAILED(rv)) return;

... which means if GetPIPNSSBundleString fails, this function silently returns and PSM continues initialization without having loaded the built-in root modules.

Comment 1

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

Attachment: bug 1311077 - fix builtin root module loading so it works in Android Cpp unit tests

Also updates the relevant test (TestCertDB) to ensure that we actually are
loading builtin root certificates.

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

Comment 2

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

Comment on attachment 8802733 [details]
bug 1311077 - fix builtin root module loading so it works in Android Cpp unit tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87040/diff/1-2/

Comment 3

[:Cykesiopka]

Comment on attachment 8802733 [details]
bug 1311077 - fix builtin root module loading so it works in Android Cpp unit tests

https://reviewboard.mozilla.org/r/87040/#review86284

Looks good, but see my question below.

::: security/manager/ssl/nsNSSComponent.cpp:1086
(Diff revision 2)
>    MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("imported %u roots", numImported));
>  }
>  #endif // XP_WIN
>  
>  void
>  nsNSSComponent::LoadLoadableRoots()

Is there a reason why this method doesn't return an error code?
Not being able to load roots seems like something we should at least propagate to the caller of this method.

Comment 4

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

Comment on attachment 8802733 [details]
bug 1311077 - fix builtin root module loading so it works in Android Cpp unit tests

https://reviewboard.mozilla.org/r/87040/#review86284

Thanks!

> Is there a reason why this method doesn't return an error code?
> Not being able to load roots seems like something we should at least propagate to the caller of this method.

Well, either it was an oversight in the original implementation or it was because of things like this, where depending on the environment, operations like getting a string bundle might fail. In the latter case, I think there's a bit of an open question of if PSM initialization should fail completely in that case or just try as hard as it can, even if that might result in starting Firefox with no trust anchors. Going forward, what might be best now is to audit PSM initialization for things like this where we may fail silently, resulting in an unusable session (basically, I'm thinking we need to divide all potential failures into two categories: those where we can fall back to a sensible default and those where it's truly a show-stopping error). I'll file a new bug on this.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f33c02b5d54f
fix builtin root module loading so it works in Android Cpp unit tests r=Cykesiopka

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f33c02b5d54f