Closed Bug 489287 Opened 11 years ago Closed 11 years ago
Resolve a few remaining issues with NSS's new revocation flags
I'm testing NSS 3.12. trunk + my own patches with Firefox 3.0.8 on Windows to test Alexei's new CRL DP fetching feature, and I ran into a problem that was due to how NSS is interpreting some of the new revocation flags defined for NSS 3.12 in file certt.h. After discussing it with Kai, I came to (yet another :) new understanding of the meanings of those flags, and wrote a small NSS patch that made NSS work with the few https sites I could find with Comodo EV certs. But I'm not sure I really yet have the interpretation of these flags correct. So, this bug wants to get agreement on the right definition, and a review of a patch to make NSS properly implement it. Attached is the patch that works for me (with very very limited testing). I wonder: Does this patch finally interpret the revocation flags correctly?
Comment on attachment 373797 [details] [diff] [review] Patch v1 for NSS Trunk There were two issues with revocation flags addressed by this patch. 1. The "method dependent" flag CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE was being interpreted as ``report revoked cert if no "source" address for this method can be found, e.g. no AIA OCSP URI for OCSP, or no CrlDP URI for CRLs.'' I'm sure that was wrong. Below I will discuss how the patch changes it. 2. The "method dependent" flag CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO was being honored, even when the "method independent" flag CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE was set. The method dependent flag was and is being understood to mean: This method MUST find fresh info or else report revocation. So, when it is set on any method, the "either one or both" meaning of the method independent flag was lost. For example, if the method dependent flag is set on the OCSP method, and the method independent flag is set, and OCSP fails, then failure is reported, even if a subsequent CRL attempt might succeed. PSM is setting the method dependent flag on both methods and also the method independent flag. In effect, this required the first method to always succeed. The patch addresses these problems as follows (in opposite order to above): a) The "method independent" flag CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE now overrides (unsets) the CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO flag on all methods. When the method independent flag is set, an OCSP failure will not prevent us from going on and trying the other method, too. b) The method dependent CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE flag is the complement to (or inverse of) the CERT_REV_M_SKIP_TEST_ON_MISSING_SOURCE flag. The latter is understood to mean ``skip this test, and do not let it report failure or revocation, if the "source" URL is missing.'' With the patch, the CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE has no meaning unless the CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO is also set. The patch now interprets CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE as follows: If the test found no fresh info, and the CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO flag is set (suggesting that a revocation should be reported) but the source was empty (not found), then report only "no info", rather than "revocation" for that method. I think this is the correct meaning of these flags. Please tell me if you agree.
I went head and looked at the header because I didn't trust my intuitive understanding of the flags (and memory). After reading the header I conclude: Nelson's point 1 is correct. Interpretation b is correct for the CERT_REV_M_REQUIRE_INFO_ON_MISSING_SOURCE. Point 2 isn't clear from the header. I would have been surprised to see PSM set CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO on each algorithm, but only set CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE. In short I would have expected a failure if I couldn't get fresh OCSP info (for instance) even if I could get CRL info. From a practical point of view, it is clear that Setting both CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO and setting CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE is asking for conflicting things. That is if you interpret the former as both the code and myself did for any single one of the algorithms, setting CERT_REV_MI_REQUIRE_SOME_FRESH_INFO_AVAILABLE would be redundant (you are already guaranteeing at least on of the flags are set). If you interpret things as Kai had and the new patch implements, then setting CERT_REV_M_FAIL_IN_SOME_MISSING_FRESH_INFO is a noop. So the net is it doesn't matter which we choose, the setting of both flags yields a redundant situation. I am happy with the new interpretation (as it does not limit the flexibility of the API to express a given policy), but I would also suggest that PSM remove the CERT_REV_M_FAIL_IN_SOME_MISSING_FRESH_INFO flag in it's call just to make things more clear. We should also describe the override effect in certt.h where these bits are defined. bob
Bob, shall I interpret your comment 2 as SR+?
Priority: -- → P1
Attachment #373797 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 373797 [details] [diff] [review] Patch v1 for NSS Trunk sr+ yes, let's get this in. We should also update certt.h with an appropriate comment. bob
pkix_crlchecker.c; new revision: 1.6; previous revision: 1.5 pkix_ocspchecker.c; new revision: 1.15; previous revision: 1.14 pkix_revocationchecker.c; new revision: 1.10; previous revision: 1.9 Checking in certt.h; new revision: 1.50; previous revision: 1.49
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.