In bug 259031, martin@v.loewis.de reports: > [...] a problem in NSS: It is *conforming* to have cA set to FALSE, yet > pathLenConstraint present. 4.2.1.10 just says that pathLenConstraint won't > be meaningful in that case, but does not forbid that (through a MUST NOT > specification). Unfortunately, NSS doesn't provide a > CERT_PATH_CONSTRAINT_ABSENT value, instead, it will report an error in > such a case. The bug is in CERT_DecodeBasicConstraintValue() at (approx) http://lxr.mozilla.org/security/source/security/nss/lib/certdb/xbsconst.c#158 This isn't a problem for any of NSS's own uses of this function, but causes problems for PSM's cert manager cert display. Patch forthcoming.

Before I ask for review, I need a cert with which to test this patch. Martin, please attach to this bug a cert that demonstrates the case you described.

> Before I ask for review, I need a cert with which to test this patch. > Martin, please attach to this bug a cert that demonstrates the case > you described. I hope Martin doesn't mind if I jump in here. This is a (very short) sample certificate with cA set to false and pathLenConstraint set to 5: -----BEGIN CERTIFICATE----- MIIBxDCCAS2gAwIBAgIJAPjtHeYanTLNMA0GCSqGSIb3DQEBBQUAMBsxGTAXBgNV BAMTEHRlc3QuZXhhbXBsZS5jb20wHhcNMDYwMTE2MjAxMDMyWhcNMDYwMjE1MjAx MDMyWjAbMRkwFwYDVQQDExB0ZXN0LmV4YW1wbGUuY29tMIGfMA0GCSqGSIb3DQEB AQUAA4GNADCBiQKBgQC8Jwv9BuzUVoQcPVDOzq9Cscdhnt62jzdJ4ky8/StmxnFE 4Xvm0DJh5eaiWQumYIOdDkc/sLsNDLCMGjN5QTeTm8Lw8t0y7u1lo99kMymSqcnn XsQZVxK/Ebl5zBvJZuj5yro3AWNxjNygYEf5FlYyqdtwY5LPGqcSXIQbcXaMlwID AQABoxAwDjAMBgNVHRMEBTADAgEFMA0GCSqGSIb3DQEBBQUAA4GBAIv/J0nfphX5 C+B8B295Fon8M/7BoWvsNgxncV4yYnAg5SUAJDRZYQh0JefpwnJgUxeku7NX0Zhw EVVnwP/ILcQxUbXpg5GPcmEat8K5XmZrTqjjllwIofdR1Xdoo2rk4v9FSaWU4ceh /jsl5ZHmbw6ncRAUVT6q9nAufSp6kSNe -----END CERTIFICATE----- However, in order to distinguish between a certificate with { cA=false, pathLenConstraint=0 } and a certificate with { cA=false } (i.e. where pathLenConstraint is absent), it would be helpful if you could set value->pathLenConstraint in line 157 to something else than 0 when value->isCA is false (maybe define a constant CERT_PATH_CONSTRAINT_ABSENT, as suggested by Martin)? Otherwise it'll get really tricky in nsNSSCertHelper.cpp to figure out what was in the certificate.

(In reply to comment #2) > I hope Martin doesn't mind if I jump in here. Thanks! I'm somewhat busy these days. > However, in order to distinguish between a certificate with { cA=false, > pathLenConstraint=0 } and a certificate with { cA=false } (i.e. where > pathLenConstraint is absent), it would be helpful if you could set > value->pathLenConstraint in line 157 to something else than 0 when value->isCA > is false (maybe define a constant CERT_PATH_CONSTRAINT_ABSENT, as suggested by > Martin)? That was my thinking: - if pathLenConstraint is present, put its value into the struct (if it is in range - else it is a clear protocol violation) - if it is absent, and isCA is true, put UNLIMITED into lenConstraint (for backwards compatibility, and to simplify processing) - if it is absent, and isCA is false, put ABSENT (say, -3) into lenConstraint. This entire issue is hopefully pathological: normally, pathLenConstraint *will* be absent if cA is false (because the RFC tells recipients to ignore the length in that case, anyway).

(In reply to comment #3) > This entire issue is hopefully pathological: Speaking of pathological cases... here's a particularly ugly one: -----BEGIN CERTIFICATE----- MIIBxTCCAS6gAwIBAgIJAOFK5x4MoUeiMA0GCSqGSIb3DQEBBQUAMBsxGTAXBgNV BAMTEHRlc3QuZXhhbXBsZS5jb20wHhcNMDYwMTE4MTc1NTMwWhcNMDYwMjE3MTc1 NTMwWjAbMRkwFwYDVQQDExB0ZXN0LmV4YW1wbGUuY29tMIGfMA0GCSqGSIb3DQEB AQUAA4GNADCBiQKBgQD3ehC7qpr51DDQDZTIupOEIPthEdSP7qtb72maGJtUM/T+ eCb/kClMbjTidUNtEyZdfD/0CqvhzXPiTtWSCOHBkPf48FGf0w1LWuSLbEQ0BVfX EcfVmuzbnJvHFHCuewfJAgOK0m7ma6QlaEf/KCjNWYZC2AYJCco6LyOQcHbDzQID AQABoxEwDzANBgNVHRMEBjAEAgL+AzANBgkqhkiG9w0BAQUFAAOBgQBNOnkBoRr2 wqONtB6xWjRYY0HWxkDGDIr8WD1k94gw+5Pfs6us0QQ10DC7RvUpufV0suTRyWNj Of47oz+VhvOO3QpCVTJt7wzy3U9A4m8EDR62XbObF7w2ModddL71FTQdGosSJp1+ ZoSfYgZg50dZ7LzaEwjYm9mjP2/RxKSCvg== -----END CERTIFICATE----- OpenSSL will happily display this as CA:FALSE, pathlen:-509 ... (as will Win XP) Nelson, I think it would be a good idea to catch this in CERT_DecodeBasicConstraintValue() as well (i.e., return SEC_ERROR_EXTENSION_VALUE_INVALID or something like that). With the current version of the proposed patch, CERT_DecodeBasicConstraintValue() will return negative values for value->pathLenconstraint if isCA is false.

Kaspar, if isCA is false, do we care what is the actual value of the path length constraint? I agree that NSS should not treat the presence of the constraint as an error in that case, but simply ignore it. Any error returned will make the whole extension appear to be invalid, which is not what we want. The choices are: a) return the encoded value, even if it's invalid (when isCA is false), or b) return some fixed valid value, which replaces (and hides) the encoded value. What's your preference?

(In reply to comment #5) > I agree that NSS should not treat the presence of the constraint as an error > in that case, but simply ignore it. I was specifically thinking of the case where pathLenConstraint is < 0, something which clearly violates the spec: BasicConstraints ::= SEQUENCE { cA BOOLEAN DEFAULT FALSE, pathLenConstraint INTEGER (0..MAX) OPTIONAL } > Any error returned will make the whole > extension appear to be invalid, which is not what we want. If pathLenConstraint is < 0, then NSS should return SEC_ERROR_EXTENSION_VALUE_INVALID, since (at least a part of) the extension value is invalid - or am I missing something here? > The choices are: > a) return the encoded value, even if it's invalid (when isCA is false), or > b) return some fixed valid value, which replaces (and hides) the encoded > value. > What's your preference? a) in the case where pathLenConstraint is present and is >= 0 and b) in the case where pathLenConstraint is absent. I.e. we would distinguish between the following cases: 1) pathLenConstraint < 0, isCA either true or false: return EXTENSION_VALUE_INVALID 2) pathLenConstraint absent, isCA true: return CERT_UNLIMITED_PATH_CONSTRAINT (-2) for pathLenConstraint 3) pathLenConstraint absent, isCA false: return a fixed value (-3 would be a good candidate, you're probably better at finding a suitable name for it) 4) pathLenConstraint present, >= 0 and isCA either true or false: return the "real" pathLenConstraint value Does this make sense to you?

Kaspar, may I ask you to write a patch with your proposal from comment 6? My only concern about that proposal is that I suspect there is code in NSS, or in programs that use NSS, that test for pathLenConstraint < 0 when what they really should test for is pathLenConstraint == CERT_UNLIMITED_PATH_CONSTRAINT Any use of other negative numbers, such as the proposal to use -3, to mean "constraint was absent" are likely to be interpreted as meaning CERT_UNLIMITED_PATH_CONSTRAINT by such code. Indeed, you can see evidence that negative values are tested this way. http://mxr.mozilla.org/security/search?string=pathLenConstraint.*%3D.*0&regexp=on&tree=security It also appears that, when encoding, a negative value means "do not encode any basic constraint option". CERT_DecodeBasicConstraintValue is a public exported function, otherwise I would suggest that we redefine CERT_UNLIMITED_PATH_CONSTRAINT to be MAX_INT. But we can't do that now. It would break binary compatibility.

What Martin wrote in bug 259031 comment 70 (quoted above in comment #0) isn't completely true, actually. RFC 3280 also says: CAs MUST NOT include the pathLenConstraint field unless the cA boolean is asserted and the key usage extension asserts the keyCertSign bit. But anyway, I've tried to clean up CERT_DecodeBasicConstraintValue() with the attached patch somewhat. It basically implements my suggestions from comment #6, except that I chose to use -1 for CERT_PATH_CONSTRAINT_NOT_INCLUDED and that I will set SEC_ERROR_EXTENSION_VALUE_INVALID when pathLenConstraint is set but the cA boolean is set to false (or not present). The PSM code (in nsNSSCertHelper.cpp) doesn't necessarily have to be adapted, and neither do certcgi.c and certext.c. CERT_EncodeBasicConstraintValue() does the right thing already, IMO - it just omits pathLenConstraint if it's < 0. The only other place where a change should be made is secu_PrintBasicConstraints() in secutil.c, I'd say... the current code doesn't actually care about CERT_DecodeBasicConstraintValue() failures when isCA is false, so I'm including a change to secutil.c in my patch, too. The two certificates posted above (comment #2 and comment #4) can be used to test the new behavior, e.g. when printing with pp. Let me know if I should attach additional test certificates to this bug.

Comment on attachment 279239 [details] [diff] [review] Patch implementing suggestions from comment #6 Thanks, Kaspar. I'm giving this r+, although I have some concerns about binary compatibility. Let's try it and see how it goes with other apps. Hopefully they'll test 3.12 Betas and report this if it is a concern.

As Kaspar noted in comment 8, the RFC says: CAs MUST NOT include the pathLenConstraint field unless the cA boolean is asserted and the key usage extension asserts the keyCertSign bit. So, this "bug" is really asking NSS to relax the requirements, and not report an error even when the cert does violate the RFC. The reason being offered to do that is so that the cert will at least appear to have decoded, and will be visible in cert manager's details view, even though it is not valid.

In light of the RFC's "MUST NOT", I'm inclined to resolve this RFE with WONTFIX. Clearly, I've changed my position since I wrote comment 5. There's a bunch of risk to making the change suggested in the patch, risk that callers (including ones on in the mozilla repository) will interpret any negative value for pathLenConstraint as meaning UNLIMITED rather than as INVALID. If even one NSS-based product experiences that, it will be perceived as a failure to maintain backwards binary compatibility. That's a headache we just don't need. IMO, The risk is too high, the reward too low. Sorry. With the minimal NSS staff we have left, we need to focus on standards compliance, not on being forgiving of badly-made certs.