Closed Bug 1257362 Opened 4 years ago Closed 2 years ago

remove support from CertVerifier for verifying for certificate usages we don't care about

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox48 --- affected
firefox58 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

Currently CertVerifier::VerifyCert supports the following usages:
certificateUsageSSLClient
certificateUsageSSLServer
certificateUsageSSLCA
certificateUsageEmailSigner
certificateUsageEmailRecipient
certificateUsageObjectSigner
certificateUsageVerifyCA
certificateUsageStatusResponder

The only one that is absolutely essential to Firefox is SSLServer. EmailSigner and EmailRecipient should probably be kept around because Thunderbird uses them. ObjectSigner is currently used as part of nsIDataSignatureVerifier::VerifySignature, but it will stop working when we stop trusting root certificates for code signing (see https://groups.google.com/forum/#!msg/mozilla.dev.security.policy/004uvRRnVyY/UAU7adNMBAAJ - scroll to the bottom if it doesn't automatically). The others are only used either in tests, as part of the certificate viewer UI where it optimistically asks, "what all might this be good for?", or nsIX509Cert.getChain (which is already problematic - see bug 867473 and bug 1004580). (VerifyCA is actually still used in nsNSSCertificateDB::ImportValidCACertsInList, but as I argued in bug 1250258, this is unnecessary.)

These superfluous usages make CertVerifier::VerifyCert much longer and more complicated than necessary, and they should be removed.
Whiteboard: [psm-cleanup]
Depends on: 1257403
Priority: -- → P3
Assignee: nobody → dkeeler
Priority: P3 → P1
Whiteboard: [psm-cleanup] → [psm-assigned]
Comment on attachment 8914554 [details]
bug 1257362 - remove the code-signing usage from certverifier as nothing uses it

https://reviewboard.mozilla.org/r/185874/#review191506

Looks good.

::: security/manager/ssl/nsNSSCertificate.cpp:647
(Diff revision 1)
>    return NS_OK;
>  }
>  
> +static nsresult
> +UniqueCERTCertListToMutableArray(UniqueCERTCertList& nssChain,
> +                                 nsIArray** _rvChain)

Nit: While we're here, it would be nice to rename this parameter to something better (x509CertArray maybe?).
Attachment #8914554 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8914554 [details]
bug 1257362 - remove the code-signing usage from certverifier as nothing uses it

https://reviewboard.mozilla.org/r/185874/#review191506

Thanks!

> Nit: While we're here, it would be nice to rename this parameter to something better (x509CertArray maybe?).

Sounds good.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee6479d783a6
remove the code-signing usage from certverifier as nothing uses it r=Cykesiopka
Backed out for sometimes failing security/manager/ssl/tests/mochitest/browser/browser_certViewer.js, at least on Linux x64 debug:

https://hg.mozilla.org/integration/autoland/rev/1f50f6c0e56ce1568acf6e6f4d954b51a9570be2

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ee6479d783a60ad7ba4aad54a50bc8c1d77a894e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135008120&repo=autoland

[task 2017-10-04T22:16:29.415Z] 22:16:29     INFO - Entering test bound testIssuerExpired
[task 2017-10-04T22:16:29.417Z] 22:16:29     INFO - TEST-PASS | security/manager/ssl/tests/mochitest/browser/browser_certViewer.js | should not have any successful usages in error case - 0 == 0 - 
[task 2017-10-04T22:16:29.418Z] 22:16:29     INFO - Buffered messages finished
[task 2017-10-04T22:16:29.419Z] 22:16:29     INFO - TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/mochitest/browser/browser_certViewer.js | determined error should be the same as expected error - "Could not verify this certificate because the issuer is unknown." == "Could not verify this certificate because the CA certificate is invalid." - JS frame :: chrome://mochitests/content/browser/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js :: checkError :: line 230
[task 2017-10-04T22:16:29.419Z] 22:16:29     INFO - Stack trace:
[task 2017-10-04T22:16:29.420Z] 22:16:29     INFO - chrome://mochitests/content/browser/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:checkError:230
[task 2017-10-04T22:16:29.421Z] 22:16:29     INFO - chrome://mochitests/content/browser/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:testIssuerExpired:56
Flags: needinfo?(dkeeler)
I think I figured it out. The tasks in that test can in theory run in any order. Thus, if the failing task runs before the one that imports the expired issuer CA, the error result will be "unknown CA", not "expired CA". This was a preexisting issue, but these changes must have perturbed execution enough to cause this to happen intermittently.
Flags: needinfo?(dkeeler)
Found this failing also on central.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=66042a70698006d3371c87e092a2c25a80584320&filter-searchStr=Linux%20x64%20asan%20Executed%20by%20TaskCluster%20with%20e10s%20test-linux64-asan%2Fopt-test-verify-e10s%20tc-e10s(TV)

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135122546&repo=mozilla-central&lineNumber=4036

[task 2017-10-05T11:10:26.545Z] 11:10:26     INFO - TEST-PASS | security/manager/ssl/tests/mochitest/browser/browser_certViewer.js | should not have any successful usages in error case - 0 == 0 - 
[task 2017-10-05T11:10:26.545Z] 11:10:26     INFO - Buffered messages finished
[task 2017-10-05T11:10:26.546Z] 11:10:26     INFO - TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/mochitest/browser/browser_certViewer.js | determined error should be the same as expected error - "Could not verify this certificate because the issuer is unknown." == "Could not verify this certificate because the issuer is not trusted." - JS frame :: chrome://mochitests/content/browser/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js :: checkError :: line 230
[task 2017-10-05T11:10:26.549Z] 11:10:26     INFO - Stack trace:
[task 2017-10-05T11:10:26.549Z] 11:10:26     INFO - chrome://mochitests/content/browser/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:checkError:230
[task 2017-10-05T11:10:26.550Z] 11:10:26     INFO - chrome://mochitests/content/browser/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js:testUntrustedIssuer:92
[task 2017-10-05T11:10:26.550Z] 11:10:26     INFO - Leaving test bound testUntrustedIssuer
[task 2017-10-05T11:10:26.551Z] 11:10:26     INFO - Entering test bound testRevoked
So, I finally figured out how the test-verify test works (it appears to only run tests that have changes in them?) and demonstrated that this test fails even without these changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19fed7f6d649ca4429bdfb8737077376e346433e (and unfortunately the changes in comment 8 don't fix it). I filed bug 1409910 to investigate this. In the meantime, I'd like to move forward with this bug.
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69d9ee864c18
Note to sheriffs: this may show up as an intermittent in the test-verify test. Unfortunately that's a preexisting issue. See comment 10, comment 12, and bug 1409910.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/efebd70a62b5
remove the code-signing usage from certverifier as nothing uses it r=Cykesiopka
https://hg.mozilla.org/mozilla-central/rev/efebd70a62b5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.