Closed
Bug 991177
Opened 10 years ago
Closed 10 years ago
SEC_ERROR_CA_CERT_INVALID is not overridable with mozilla::pkix, but the UI makes it look like it is
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: keeler, Assigned: mmc)
References
Details
Attachments
(2 files, 2 obsolete files)
62.07 KB,
image/png
|
Details | |
10.16 KB,
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
See bug 990557. The UI makes it look like the error is overridable, but it is not when using mozilla::pkix (but it appears to be for classic verification). We'll probably have to modify NSSErrorsService::GetErrorClass and related code to take into account which verification library is in use. Or we could just disallow such overrides completely.
Comment 1•10 years ago
|
||
(In reply to David Keeler (:keeler) from comment #0) > Or we could just disallow such overrides completely. I suggest we just disallow the overrides completely, and mark this bug with an indicator that its patch would need to be backed out if we switch the default cert verifier.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mmc
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Tested manually.
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8407036 [details] [diff] [review] Disallow overrides for SEC_ERROR_CA_CERT_INVALID ( Review of attachment 8407036 [details] [diff] [review]: ----------------------------------------------------------------- I thought it was better to be explicit about non-overridable errors instead of falling through to default. Your choice, though.
Attachment #8407036 -
Flags: review?(dkeeler)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8407036 [details] [diff] [review] Disallow overrides for SEC_ERROR_CA_CERT_INVALID ( Review of attachment 8407036 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I forgot to mention a few other things you'll need to change: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#302 https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#566 https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#87 (this was implemented with zero input from PSM folks, so yay!) Also, these will need some modification: https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_overrides.js#100 https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_overrides.js#130 (do something like https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_overrides.js#135 where for classic verification we don't expect an override to be possible) ::: security/manager/ssl/src/NSSErrorsService.cpp @@ +105,5 @@ > case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED: > *aErrorClass = ERROR_CLASS_BAD_CERT; > break; > + // Non-overridable errors. > + case SEC_ERROR_CA_CERT_INVALID: The thing about this is that every other SEC_ERROR_* or SSL_ERROR_* is non-overridable, so I don't think it makes sense to enumerate them (or even this one) here.
Attachment #8407036 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8407036 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8407139 [details] [diff] [review] Disallow overrides for SEC_ERROR_CA_CERT_INVALID ( Review of attachment 8407139 [details] [diff] [review]: ----------------------------------------------------------------- Please take another look. Unittests in security/manager/ssl pass locally. https://tbpl.mozilla.org/?tree=Try&rev=1dfc58de2a47
Attachment #8407139 -
Flags: review?(dkeeler)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8407139 [details] [diff] [review] Disallow overrides for SEC_ERROR_CA_CERT_INVALID ( Review of attachment 8407139 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_cert_overrides.js @@ +52,5 @@ > .getHistogramById("SSL_CERT_ERROR_OVERRIDES") > .snapshot(); > do_check_eq(histogram.counts[ 0], 0); > do_check_eq(histogram.counts[ 2], 8 + 1); // SEC_ERROR_UNKNOWN_ISSUER > + do_check_eq(histogram.counts[ 4], 0 + 5); // SEC_ERROR_UNTRUSTED_ISSUER this is a duplicated line, will remove
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8407139 [details] [diff] [review] Disallow overrides for SEC_ERROR_CA_CERT_INVALID ( Review of attachment 8407139 [details] [diff] [review]: ----------------------------------------------------------------- Awesome - r=me with comments addressed. ::: security/manager/ssl/tests/unit/test_cert_overrides.js @@ +35,5 @@ > } > > +function add_non_overridable_test(aHost, aExpectedError) { > + add_connection_test( > + aHost, getXPCOMStatusFromNSS(aExpectedError), null, nit: so far, the convention has been to convert the expected NSS error to an xpcom status before passing it to any function, which is not what you're doing here (i.e. when this function gets called). However, this makes the calling code a bit easier to read, so I like it. How about filing a follow-up bug to convert the rest of the tests to do the same thing? (i.e. have add_cert_override_test do the conversion from NSS error to xpcom status). @@ +52,5 @@ > .getHistogramById("SSL_CERT_ERROR_OVERRIDES") > .snapshot(); > do_check_eq(histogram.counts[ 0], 0); > do_check_eq(histogram.counts[ 2], 8 + 1); // SEC_ERROR_UNKNOWN_ISSUER > + do_check_eq(histogram.counts[ 4], 0 + 5); // SEC_ERROR_UNTRUSTED_ISSUER This should be: do_check_eq(histogram.counts[ 3], 0); // SEC_ERROR_CA_CERT_INVALID
Attachment #8407139 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8407139 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8407160 [details] [diff] [review] Disallow overrides for SEC_ERROR_CA_CERT_INVALID ( Review of attachment 8407160 [details] [diff] [review]: ----------------------------------------------------------------- Filed https://bugzilla.mozilla.org/show_bug.cgi?id=996872 and carrying over r+ from David. Will wait a bit just in case try turns up anything interesting.
Attachment #8407160 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/eacdbceea76d
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eacdbceea76d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•