1400913 - find a way to prefer roots with bug 1364159 or revert bug 1364159

Status: VERIFIED FIXED

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

Description

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

Bug 1364159 introduced an optimization to not search the user's certificate databases (both the built-in roots module and the cached certificate database, as well as whatever other PKCS#11 modules are loaded) when it can build a verified chain out of the list the server sends. This has the side-effect that if a server sends a number of cross-signed CAs, Firefox will traverse all of them and build a long chain rather than quickly finding a trust anchor.

e.g. server sends EE, Intermediate, CA1-signed-by-CA2, CA2-signed-by-CA3 (where CA1, CA2, and CA3 are all trust anchors) -> Firefox will build EE -> Intermediate -> CA1 -> CA2 -> CA3 (as built-in root).

Since CA1 is a trust anchor, we should be able to find EE -> Intermediate -> CA1 (as built-in root).

We could expand bug 1364159 to look in the built-in roots module as well as the list sent by the server, but at that point this optimization may be too complex for the benefit it provides.

[Tracking Requested - why for this release]: potential performance regression in TLS handshakes

Comment 1

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

Attachment: bug 1400913 - back out the functionality changes from bug 1364159 (but keep the test)

Bug 1364159 introduced an optimization that attempted to avoid reading from the
user's cached certificate database as much as possible when building a verified
certificate chain. Unfortunately this had the side-effect of not preferring root
certificates in path building, which can result in unnecessarily long chains
(which rather defeats the purpose, since it means more signature verifications).
This patch reverts the functionality changes from that bug but keeps the test
that was added (the test didn't directly test the functionality changes - it's
more of a check that path building will query the cached certificate db when
necessary).

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

Comment 2

[Mike Taylor [:miketaylr]]

(I think the ship has sailed for 55, possibly 56 as well...)

Comment 3

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

Comment on attachment 8909428 [details]
bug 1400913 - back out the functionality changes from bug 1364159 (but keep the test)

https://reviewboard.mozilla.org/r/180926/#review186208

I've confirmed that this is a partial backout of https://hg.mozilla.org/mozilla-central/rev/600b709c2634 . I don't see anything dangerous in here.

Comment 4

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

Thanks! try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c556fab5e805

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11b46d2109c4
back out the functionality changes from bug 1364159 (but keep the test) r=jcj

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/11b46d2109c4

Comment 7

[Chris H-C :chutten|PTO (back Jan 5)]

This had the desired effect of reversing the data change we saw back in May: https://mzl.la/2kLbF9p

There's but one thing left to do: add an alert_emails and bug_numbers field for CERT_VALIDATION_SUCCESS_BY_CA in Histograms.json so that you can receive (and respond to) future alerts without waiting for me to get around to triaging them.

Would you like me to file a separate bug for this work?

Comment 8

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

Sure - thanks!
(although note we do have bug 1369747 already filed)