if PathBuildingStep::Check fails due to a problem with the subject certificate rather than the potential issuer, set keepGoing to false

VERIFIED FIXED in Firefox 59

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla60
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox58 wontfix, firefox59 verified, firefox60 verified)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment)

Consider a PKI hierarchy like so:

A   B
|   |
C   D
 \ /
  E

where A and B are root certificates, C and D are intermediates, and E is an end-entity certificate. Further consider the case where B is marked as "not a trust anchor" by the user and E has been revoked by the CA.

mozilla::pkix will find the path E -> C -> A, which is initially successful until it checks revocation on E, whereupon it discovers that it has been revoked. Ever optimistic, mozilla::pkix will continue attempting to find a path for E, which will obviously always fail because it's revoked (arguably I think this is the bug). In any case, mozilla::pkix finds path E -> D -> B, can't find an issuer for B, and so returns "unknown issuer". This error gets propagated back down the chain, and at some point mozilla::pkix has to choose which of "revoked" and "unknown issuer" to report. That logic is here: https://hg.mozilla.org/mozilla-central/file/35dfa0882568c06f2d81b6749179815cd4f82886/security/pkix/lib/pkixbuild.cpp#l112

In general, it's probably correct to punt on which of two errors to report in situations where two paths differ. However, for errors in PathBuildingStep::Check that are due to the *subject* rather than the *potential issuer*, keepGoing should probably be set to false, because if the error is in the subject, then no issuer will be appropriate. This will help us get a more accurate error in such situations.
Assignee: nobody → dkeeler
Priority: P2 → P1
Whiteboard: [psm-assigned]
Comment on attachment 8950377 [details]
bug 1437214 - if PathBuildingStep::Check fails due to a problem with the subject certificate rather than the potential issuer, set keepGoing to false

https://reviewboard.mozilla.org/r/219594/#review226696

Sorry for the delay. The pathbuilding code is easy, understanding this test is hard.

Let me start with the question below to see if I'm just crazy, but it feels like this trustDomain's stub methods might be worthy of a brief explanation of why they are accomplishing what the test says.

::: security/pkix/test/gtest/pkixbuild_tests.cpp:648
(Diff revision 1)
> +  {
> +    ByteString subjectDER(InputToByteString(encodedIssuerName));
> +    ByteString certDER(subjectDERToCertDER[subjectDER]);
> +    assert(!ENCODING_FAILED(certDER));
> +    bool keepGoing;
> +    Result rv = CheckCert(certDER, checker, keepGoing);

Since this is always starting by checking the end entity, and these tests are that we just _stop_, can't we assert that keepGoing is false here?
Well, a couple thoughts:

Conceptually, the <Whatever>TrustDomain implementations are more or less just helpers that ensure we're exercising the code/cases we're interested in. The properties we're asserting should really be in TEST_<whatever> bodies.

Practically, we can't use the gtest ASSERT_<whatever> macros because they return early with the wrong return type. We could use the c-assert like we already have there, but that's more of a test self-consistency check, and if NDEBUG is defined, it won't fire (although other things will fail there).

Also, checking that we stop at that point is a bit of an implementation detail. We could achieve the "correct" behavior even if we were to explore all paths, as long as we reported the revoked certificate error (provided there were no alternative paths - e.g. if it were an intermediate that was revoked and we found another non-revoked intermediate).

I could certainly add more documentation to the added test/helpers.
Comment on attachment 8950377 [details]
bug 1437214 - if PathBuildingStep::Check fails due to a problem with the subject certificate rather than the potential issuer, set keepGoing to false

https://reviewboard.mozilla.org/r/219594/#review226954

OK, thanks for the explanation!
Attachment #8950377 - Flags: review?(jjones) → review+
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db007b7722fdceb13d5d8b86e97d3954402a4352 so let's yolo land on a friday afternoon just before heading home! (before a 3-day weekend, no less!)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f2669a2b5934
if PathBuildingStep::Check fails due to a problem with the subject certificate rather than the potential issuer, set keepGoing to false r=jcj
https://hg.mozilla.org/mozilla-central/rev/f2669a2b5934
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8950377 [details]
bug 1437214 - if PathBuildingStep::Check fails due to a problem with the subject certificate rather than the potential issuer, set keepGoing to false

Approval Request Comment
[Feature/Bug causing the regression]: long-standing bug
[User impact if declined]: In theory, a malicious server could trick Firefox into displaying a less urgent error (like "unknown issuer") when it should be displaying a more urgent error like "revoked certificate". Note that an unknown issuer error is overridable whereas a revoked certificate error isn't. (So, in theory, an attacker could convince a user to visit a site using a revoked certificate, which is bad.)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes

1. In the certificate manager (about:preferences -> search for "certificates" -> view certificates -> "authorities" tab), select the "VeriSign Class 3 Public Primary Certification Authority - G5" certificate and click "edit trust".
2. Uncheck the "This certificate can identify websites" checkbox.
3. Visit https://revoked-root4.websecurity.symantec.com/
If the bug is fixed, the error will be "SEC_ERROR_REVOKED_CERTIFICATE" and it will not be overridable. If the bug is not fixed, the error will be "SEC_ERROR_UNKNOWN_ISSUER" and it will be overridable.

[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: this is a sensitive area of code, but the change is minimal and we have good test coverage
[String changes made/needed]: none
Attachment #8950377 - Flags: approval-mozilla-beta?
Comment on attachment 8950377 [details]
bug 1437214 - if PathBuildingStep::Check fails due to a problem with the subject certificate rather than the potential issuer, set keepGoing to false

Approved for Beta - let's get this into 59b12. This seems out of scope for ESR inclusion - is this something you'd like to argue for anyway or can it go ride with ESR60 instead?
Flags: needinfo?(dkeeler)
Attachment #8950377 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Yeah, this probably doesn't need to be uplifted to ESR52 (the attack this enables relies on attackers convincing users to click through e.g. "untrusted issuer" warnings, which a) you can't for any HSTS site (so google, etc.) and b) is about as powerful as just sending a self-signed certificate and hoping users will click through the error, so this really doesn't enable anything that isn't already possible).
Flags: needinfo?(dkeeler)
Managed to reproduce this issue on Nightly 60.0a1(2018-02-08) on Windows 10 x64 using the STR from comment 8, but couldn't see it under Mac OS X 10.11 and Ubuntu 16.04 x64.

Verified fixed using the latest Nightly 60.0a1(2018-02-21).
Hmmm - try visiting https://valid-root4.websecurity.symantec.com/ as a step 0? Or maybe step 2b?
I have managed to reproduce this issue using Firefox 60.0a1 (BuildId:20180209220324).

This issue is verified fixed using Firefox 59.0 (BuildId:20180305193205) on Windows 10 64bit, macOS 10.12 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.