376748 - Infinite loop in CERT_CertChainFromCert

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

While investigating bug 376329, attempting to reproduce it with cmsutil,
I found another infinite loop.  This is believed to be a DIFFERRENT flaw
from the one reported in bug 376329, which is why I'm filing a separate bug
about it.  

NSS Function CERT_CertChainFromCert invokes NSSCertificate_BuildChain with 
the number zero for the maximum number of certs in the returned chain. 
Zero is interpreted as "no limit".  

I have proposed that bug 376329 be fixed by reverting to the old code 
(the code inside of #ifdef NSS_CHAIN_BUG_FIXED at http://lxr.mozilla.org/security/source/security/manager/ssl/src/nsNSSCertificate.cpp#774 ) but that fix cannot be done until this bug is fixed.  

The simple fix for this is a one line change in CERT_CertChainFromCert, to
invoke NSSCertificate_BuildChain with a small upper bound to the size of 
the chain that NSSCertificate_BuildChain may construct.  The symbol 
CERT_MAX_CERT_CHAIN exists exactly for that purpose.  I have a tiny patch
that does just that.

But I think a somewhat larger fix is needed than the simplest one.  
If & when NSSCertificate_BuildChain returns a maximum length chain, 
CERT_CertChainFromCert should probably treat it as an error, which it 
presently does not.

Comment 1

[Kai Engert (:KaiE:)]

Nelson, could this be done in 2 steps?

Could we take your tiny patch now, land immediately on nss trunk and branch, so that 376329 can be fixed by using the new NSS release?

Your second part related to maximum length chain sounds like an optional improvement.

Taking the tiny patch now and postponing the optional improvement would allow us to get two bugs fixed now,
without requiring you to work on that optional part immediately?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: my tiny patch, v1

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 265598 [details] [diff] [review]
my tiny patch, v1

Seems reasonable.
r=kengert

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 265598 [details] [diff] [review]
my tiny patch, v1

Requesting second review, because we need this on 3.11 branch because this blocks a psm bug.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Kai, Thanks for the reminder.  Reviewed bugs just fall off the radar. :(

Bug 376748 - Infinite loop in CERT_CertChainFromCert , r=kengert,julien

Checking in certhigh.c; new revision: 1.34.2.4; previous revision: 1.34.2.3
Checking in certhigh.c; new revision: 1.39;     previous revision: 1.38