1240118 - mechanism to treat an imported test root certificate as a built-in

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

In bug 1239455 and others (particularly pinning-related bugs, I think) it has become apparent that it would be nice to have a way to treat an imported test root certificate as a built-in. This should probably be limited to debug builds or something. In any case, the security implications need to be considered.

Comment 1

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

Attachment: MozReview Request: bug 1240118 - add functionality to treat a test certificate as a built-in root r?mgoodwin

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

Comment 2

[:Cykesiopka]

https://reviewboard.mozilla.org/r/40633/#review37179

Hmm, are we OK with having the pref take effect even on opt builds?

Comment 3

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8731479 [details]
MozReview Request: bug 1240118 - add functionality to treat a test certificate as a built-in root r?mgoodwin

https://reviewboard.mozilla.org/r/40633/#review37393

Assuming we're happy to accept the risk of this being abused, this looks good to me.

::: security/certverifier/CertVerifier.cpp:85
(Diff revision 1)
>    result = false;
>    nsCOMPtr<nsINSSComponent> component(do_GetService(PSM_COMPONENT_CONTRACTID));
>    if (!component) {
>      return Result::FATAL_ERROR_LIBRARY_FAILURE;
>    }
> +  nsresult rv = component->IsCertTestBuiltInRoot(cert, result);

There are a number of things that behave differently for built-in roots (e.g. setting and observation of PKP state, setting of STS). Because of this, the ability to set a built-in root (for testing) is different (and carries different risks) to the ability to add a local root.

In particular, I'm a little worried about the risk that this could be abused (e.g. by Antiviruinavs, middleboxes, some other addon) causing problems for users (inadvertent DoS, etc).

We currently hide some PSM testing things (actual built-in roots) behind conditional compilation (so they're only present in debug builds). Thinking out loud; would it negate the use of this feature if we did this? Is the risk of the potential problems great enough to even worry about this?

Comment 4

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

Comment on attachment 8731479 [details]
MozReview Request: bug 1240118 - add functionality to treat a test certificate as a built-in root r?mgoodwin

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

Comment 5

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

https://reviewboard.mozilla.org/r/40633/#review37433

::: security/manager/ssl/nsNSSComponent.h:85
(Diff revision 2)
>    NS_IMETHOD ShutdownSmartCardThread(SECMODModule* module) = 0;
>  #endif
>  
>    NS_IMETHOD IsNSSInitialized(bool* initialized) = 0;
>  
> +#ifdef DEBUG

Thanks for the reviews. Upon further thought, I think it is probably best if this is debug-only, so I sprinkled a bunch of #ifdef DEBUGs in there.

Comment 6

[:Cykesiopka]

https://reviewboard.mozilla.org/r/40633/#review37443

Random nit I spotted when skimming the interdiff...

::: security/manager/ssl/tests/unit/test_cert_sha1.js:107
(Diff revision 2)
> +    let root = certFromFile("ca");
> +    Services.prefs.setCharPref("security.test.built_in_root_hash", root.sha256Fingerprint);
> +    checkIntermediate(certFromFile("int-pre"), PRErrorCodeSuccess);
> +    checkEndEntity(certFromFile("ee-pre_int-pre"), PRErrorCodeSuccess);
> +    checkEndEntity(certFromFile("ee-post_int-pre"), SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED);
> +    // This should fail but it doesn't, because for the implementation makes no

Nit: s/because for the/because the/

Comment 7

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

https://reviewboard.mozilla.org/r/40633/#review37443

> Nit: s/because for the/because the/

Thanks - good catch.

Comment 8

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

Comment on attachment 8731479 [details]
MozReview Request: bug 1240118 - add functionality to treat a test certificate as a built-in root r?mgoodwin

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

Comment 9

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

Try looked good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1ce4c02a8c0&filter-tier=1&filter-tier=2&filter-tier=3

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/078bf91ed20a

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/078bf91ed20a