489287 - Resolve a few remaining issues with NSS's new revocation flags

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1 for NSS Trunk

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 1

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 2

[Robert Relyea]

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

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Bob, shall I interpret your comment 2 as SR+?

Comment 4

[Robert Relyea]

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

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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