Closed Bug 1240118 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- affected
firefox48 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → dkeeler
https://reviewboard.mozilla.org/r/40633/#review37179

Hmm, are we OK with having the pref take effect even on opt builds?
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?
Attachment #8731479 - Flags: review?(mgoodwin) → review+
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/
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.
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/
https://reviewboard.mozilla.org/r/40633/#review37443

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

Thanks - good catch.
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/
https://hg.mozilla.org/mozilla-central/rev/078bf91ed20a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.