323557 - Allow non-CA certs' Basic Constraints to have path length constraints

Status: RESOLVED WONTFIX

(NSS :: Libraries, enhancement, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

In bug 259031, [e-mail] 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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

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.

Comment 2

[Kaspar]

> 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.

Comment 3

[Martin v. Löwis]

(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).

Comment 4

[Kaspar]

(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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 6

[Kaspar]

(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?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 8

[Kaspar]

Attachment: Patch implementing suggestions from comment #6

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 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

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.