1437754 - Add a pref to disable the Symantec distrust algorithm

Status: RESOLVED FIXED

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

Description

[J.C. Jones [:jcj] (he/him)]

The Symantec distrust being implemented in Firefox 60 in Bug 1434300 carries some significant compatibility risk. Let's add an unlisted boolean pref, "security.ssl.distrust_symantec_roots", that if set false will resume trusting the affected Symantec roots.

Comment 1

[J.C. Jones [:jcj] (he/him)]

Attachment: Bug 1437754 - Add a pref and disable the Symantec distrust algorithm

This adds the unregistered pref "security.pki.distrust_ca_policy" which, if set
to 0, will re-enable the Symantec roots; it defaults to 1, which enforces
the graduated distrust from Bug 1409257.

This pref is intended to outlast the Symantec distrust, and instead be able to
extend to enable/disable future root policy actions. It would need its own
tests for that, in the future.

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

Comment 2

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8954445 [details]
Bug 1437754 - Add a pref and disable the Symantec distrust algorithm

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

Comment 3

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

Comment on attachment 8954445 [details]
Bug 1437754 - Add a pref and disable the Symantec distrust algorithm

https://reviewboard.mozilla.org/r/223524/#review229656

Looks good - just some nits. J.C.'s out for the day (and maybe more), and we want to land this asap, so I would fix this up and land, but the trees are closed anyway, so we'll have to wait.

::: commit-message-6d72e:4
(Diff revision 2)
> +Bug 1437754 - Add a pref and disable the Symantec distrust algorithm r?keeler r?fkiefer
> +
> +This adds the pref "security.pki.distrust_ca_policy" which, if set to 1,
> +enforces the graduated distrust from Bug 1409257, and set to 0 (as it is in this

"and if set to 0"?

::: security/certverifier/NSSCertDBTrustDomain.cpp:21
(Diff revision 2)
>  #include "cert.h"
>  #include "certdb.h"
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Casting.h"
>  #include "mozilla/Move.h"
> +#include "mozilla/Preferences.h"

Is this needed here?

::: security/certverifier/NSSCertDBTrustDomain.cpp:67
(Diff revision 2)
>                                             unsigned int minRSABits,
>                                             ValidityCheckingMode validityCheckingMode,
>                                             CertVerifier::SHA1Mode sha1Mode,
>                                             NetscapeStepUpPolicy netscapeStepUpPolicy,
>                                             const OriginAttributes& originAttributes,
> +                                           DistrustedCAPolicy distrustedCAPolicy,

nit: I would say maybe put this before OriginAttributes, actually.

::: security/manager/ssl/nsNSSComponent.cpp:1697
(Diff revision 2)
> +  DistrustedCAPolicy defaultCAPolicyMode =
> +    DistrustedCAPolicy::DistrustSymantecRoots;
> +  DistrustedCAPolicy distrustedCAPolicy =
> +    static_cast<DistrustedCAPolicy>
> +      (Preferences::GetUint("security.pki.distrust_ca_policy",
> +                            static_cast<uint32_t>(defaultCAPolicyMode)));

Usually we have a switch statement for these so we catch the case where the user put in something we don't even have a policy for.

::: security/manager/ssl/security-prefs.js:136
(Diff revision 2)
>  pref("security.cert_pinning.max_max_age_seconds", 5184000);
> +
> +// security.pki.distrust_ca_policy controls what root program distrust policies
> +// are enforced at this time:
> +// 0: No distrust policies enforced
> +// 1: Symantec root distrust policy enforced

Maybe link to wiki or something here?

Comment 4

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8954445 [details]
Bug 1437754 - Add a pref and disable the Symantec distrust algorithm

https://reviewboard.mozilla.org/r/223524/#review229656

Thanks. Updating.

> Is this needed here?

Oops. Thanks.

Comment 5

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8954445 [details]
Bug 1437754 - Add a pref and disable the Symantec distrust algorithm

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

Comment 6

[J.C. Jones [:jcj] (he/him)]

Try run is pretty clean. https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa39d640f33c3f1e0c7cbe5bd8759d3ac13a5a50&selectedJob=164674050

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53f2c8abb352
Add a pref and disable the Symantec distrust algorithm r=keeler

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8954445 [details]
Bug 1437754 - Add a pref and disable the Symantec distrust algorithm

https://reviewboard.mozilla.org/r/223524/#review229780

I'm fine with the patch as is. But I think it needs work or a little more explenation for how to handle the pref to be future proof.

::: security/manager/ssl/security-prefs.js:138
(Diff revision 3)
> +// security.pki.distrust_ca_policy controls what root program distrust policies
> +// are enforced at this time:
> +// 0: No distrust policies enforced
> +// 1: Symantec root distrust policy enforced
> +// See https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions for more details.
> +pref("security.pki.distrust_ca_policy", 0);

I wonder how the next distrust looks like. Is it `2: Let's Encrypt root distrust policy enforced`? If yes, how is `1` still enforced then? Or is it supposed to be a bit mask that's set to `3`? (That wouldn't work with the code we currently have.)

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/53f2c8abb352

Comment 10

[J.C. Jones [:jcj] (he/him)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #8)
> Comment on attachment 8954445 [details]
> Bug 1437754 - Add a pref and disable the Symantec distrust algorithm
> 
> https://reviewboard.mozilla.org/r/223524/#review229780
> 
> I'm fine with the patch as is. But I think it needs work or a little more
> explenation for how to handle the pref to be future proof.

Yeah, I've left some technical debt here. That's haste for you. :(

> ::: security/manager/ssl/security-prefs.js:138
> (Diff revision 3)
> > +// security.pki.distrust_ca_policy controls what root program distrust policies
> > +// are enforced at this time:
> > +// 0: No distrust policies enforced
> > +// 1: Symantec root distrust policy enforced
> > +// See https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions for more details.
> > +pref("security.pki.distrust_ca_policy", 0);
> 
> I wonder how the next distrust looks like. Is it `2: Let's Encrypt root
> distrust policy enforced`? If yes, how is `1` still enforced then? Or is it
> supposed to be a bit mask that's set to `3`? (That wouldn't work with the
> code we currently have.)

So I actually wrote this to work like a bitmask, and then had linter issues and -- for the sake of haste and illness -- made the choice to just do equality checking.

I'm going to open a follow-on to change this to a bitmask.