I discovered this issue while analyzing bug 288713. When processing a cert chain received from a peer (e.g. a server's cert chain received by a client, or vice versa), if the peer's "End Entity" (EE, leaf) cert cannot be decoded then there's no way that the peer's public key can be used to encrypt the pre-master secret or validate a server key exchange. So it makes sense to force the handshake to terminate with an error if the EE cert cannot be decoded, and that is what libSSL does today. But if an issuer cert cannot be decoded, that situation should be treated as if the cert was missing. The SSL handshake should not be terminated immediately just for that decode failure. But that is what libSSL does today. Rather, if the EE cert was decoded, libSSL should go on and attempt to verify the cert chain and continue with the handshake. A browser user should see an "unknown issuer" error rather than a "BAD DER" error when the undecodable cert is not the EE cert. This way, if a user chooses to trust the EE cert, the handshake can succeed.
Created attachment 179413 [details] [diff] [review] patch v1 all.sh passes with this patch. Julien, please review.
Comment on attachment 179413 [details] [diff] [review] patch v1 Nelson, I'm not certain that we should change this behavior. If the server sent a chain of certs which it claims are in DER format, IMO, an NSS client should enforce that they really are in that format, regardless of whether the certs are trusted or not. To me, this situation is obviously different from the case where a server sends a truncated chain, of properly DER-encoded certs. NSS has always performed the DER encoding check until now, and we need to consider the security implications of removing it. IMO, a client that trusts a cert which is part of a chain that it can't even decode is suspect, and may not have made a very informed choice of trust. Are there any legit applications that require removing this check ?
Julien, It may be that the browser's cert store already has a copy of the correct cert in it, and that the chain will properly validate without the erroneous cert. Thus it is possible to merely drop the bad cert and still have the chain validate. Recently we saw this very problem a Sun employee's office. The server was sending a root cert that the browser could not parse. If the server had NOT sent the root cert at all, the browser could have succesfully completed the handshake, but because the server sent that root cert, the browser failed utterly to complete the chain.
Julien, Perhaps you misunderstand what this patch does. It doesn't accept the undecodable cert at face value. It discards the undecodable cert, but continues to parse the cert chain, and then relies on cert chain validation logic to decide whether the chain is valid or not. Do you believe that is improper? Do you not trust our cert chain validation logic to come to the right answer? Bob, Wan-Teh, any opinions on this?
I'm with nelson here. The inability to parse or understand certs sent from the server shouldn't automatically fail the SSL operation. We definately shouldn't import certs we can't decode, but we shouldn't fail S/MIME or SSL operations simply because we cannot parse the a cert that was sent to us. bob
Nelson, I understand what the patch does, but I don't like it. The fact is that most people who will "trust" the leaf cert will be browser users who don't know any better, and will do so *after* they receive the cert chain. Except that with this patch, they won't have any idea the chain contained a broken cert. They won't be told about it at all. An incomplete chain will be shown instead. I for one might make different decisions about the trust for the case of missing issuer cert and broken issuer cert. I'd like to be able to look at the broken cert to make an informed decision. But this patch discards it.
Julien, It's impossible to look at the undecodable cert. It's undecodable. Today, the user doesn't get to make any decision. The Handshake fails. period. The user gets no information with which to diagnose the problem, other than error -8182 (or whatever it is). With this patch, the user will either get a) a succesfully completed handshake because the browser was able to supply the missing cert(s) and they chained to a trusted root, or b) an "unknown issuer" dialog, which will at least HELP in diagnosing the problem.
I know the user can't look at undecodable certs, with today's current cert chain building & validation APIs, and PSM code. That's unfortunate. This patch will probably confuse diagnosis in at least as many cases as it helps. We might erroneously think there are misconfigured servers that are missing part of their chain, when in fact they are sending a full chain, except it's a bogus one. I would like a distinction for these 2 cases. Right now, there is a clear distinction, because one case fails the handshake altogether. I think this case is worthy of a new error code and class of pop-ups. The question is how and when to return it to the application.
I totally disagree with Julien here. I is *NOT* unfortunate that the user doesn't get to see or trust the certs. For most users running most applications that is well beyond the call of duty. For those that care, then tools can look at the certs and say the server is sending bad DER data in them. In general, this falls under: "Be liberal in what you accept and strict in what you generate". The exceptions to this rule are 1) if there's a valid security concern, or 2) being "liberal" prevents you from acting properly in an upgrade evironment (example: saying 'sure I do SSL 3.5, instead up negotiating back to SSL 3.0 or 3.1). The default mode of our code should not necessarily be a tester of servers being properly configured. In general ignoring certs malformed (or even well formed certs for that matter) does not affect the security, if it did we would be in trouble already because if the cert in the cert chain *was* properly formated, we would have imported it then ignored it in favor of the certificate we have in our database that we already trust.
Julien and I have reached a compromise. It involves implementing an old idea that I've been thinking about for a long time, namely implementing a new alternative cert chain verification callback API for SSL, one that passes all the raw DER certs to the callback, rather than passing them all to CERT_NewTempCertificate (as the present code does) and then passing the pointer to the leaf's CERTCertificate to the callback. This new API allows the callback API to do things quite differently than the present one does, and probably benefits libPKIX as a side effect (which may be Julien's ulterior motive :) With this alternative available, Julien is willing to allow the old/existing code path to work as I intended in my patch. I'm satisfied with the proposed compromise. But it's too late in the 3.10 release cycle (at Sun) for me to add a new public method to the SSL API (namely the function to register the new alternative callback). So, I'm retargetting this at 3.11
I'm OK with the compromise as well. bob
Comment on attachment 179413 [details] [diff] [review] patch v1 marking patch obsolete to show that this patch is no longer a candidate to be checked in as-is.
Fixing this bug would be really nice for the PKIX work in 3.12 . There should be a new SSL cert checking callback that allows passing the raw DER certs, so that it is possible to write an application that checks the actual certs. This would is needed to properly implement the vfyserv "save certs" feature - see bug 363477 . We also need other enhancements to the callbacks support non-blocking I/O - see bug 324867 . They should be dealt with at the same time in the new callback prototype. The application would get the choice of using either prototype by using either the old or new callback setter function.
Julien, I completely agree with your comment 12. If I ever get a chance to go back to being MisterSSL, then I will work on this. But with our present level of staffing, it's not clear that I will be able to go back to being MisterSSL - certainly not until after 3.12, and maybe not after that. :-(
Unsetting target milestone in unresolved bugs whose targets have passed.