288788 - SSL should not abort handshake just because some issuer certs are undecodable

Status: ASSIGNED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

all.sh passes with this patch.	Julien, please review.

Comment 2

[Julien Pierre]

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 ?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 5

[Robert Relyea]

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

Comment 6

[Julien Pierre]

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.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 8

[Julien Pierre]

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.

Comment 9

[Robert Relyea]

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.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 11

[Robert Relyea]

I'm OK with the compromise as well.

bob

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 13

[Julien Pierre]

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.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.