Closed Bug 303603 Opened 19 years ago Closed 19 years ago

crash during ssl server socket handshake

Categories

(Core :: Security: PSM, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED INVALID

People

(Reporter: bambas, Assigned: KaiE)

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050727 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050727 Firefox/1.0+

There is a crash in function HandshakeCallback in file nsNSSCallbacks.cpp line 242:

  CERTCertificate *peerCert = SSL_PeerCertificate(fd);
> char* caName = CERT_GetOrgName(&peerCert->issuer);

In case the server is setup TO REQUEST but NOT TO REQUIRE the client's
certificate, SSL_PeerCertificate returns NULL when client has no cetificate to
provide. This null pointer is dereferenced.

When client provides its valid certificate connection is established correctly.

This bug was tested on experintal HTTPS server listening on port 61000. I used
FireFox 1.1, Deer Park Alpha 2, IE 6 to connect to this HTTPS server and bug has
appeared using every agent at same condition as described above.

Reproducible: Always

Steps to Reproduce:
I establish the server this way:

I accept the socket using NSPR's PR_Accept. This is a TCP socket. Then I apply
this code to layer ssl connection on this TCP socket:

...
    nsresult rv;
    
    // fd is the accepted TCP socket
    PRFileDesc* sslSocket = fd;
    nsCOMPtr<nsISupports> SSLSecInfo;
    SECStatus secStatus;
	
    PRSocketOptionData socketOption;
    socketOption.option             = PR_SockOpt_Nonblocking;
    socketOption.value.non_blocking = PR_TRUE;
    PR_SetSocketOption(fd, &socketOption);

    // mSSLProvider is the provider getter with thw socket provider service
    rv = mSSLProvider->AddToSocket(addr->raw.family, "localhost", 0, nsnull,
nsnull, 0,
        sslSocket, getter_AddRefs(SSLSecInfo));
    NS_ENSURE_SUCCESS(rv, rv);
    
    secStatus = SSL_OptionSet(sslSocket, SSL_SECURITY, PR_TRUE);
    if (secStatus != SECSuccess) goto error;
    secStatus = SSL_OptionSet(sslSocket, SSL_HANDSHAKE_AS_CLIENT, PR_FALSE);
    if (secStatus != SECSuccess) goto error;
    secStatus = SSL_OptionSet(sslSocket, SSL_HANDSHAKE_AS_SERVER, PR_TRUE);
    if (secStatus != SECSuccess) goto error;
    secStatus = SSL_OptionSet(sslSocket, SSL_REQUIRE_CERTIFICATE, PR_FALSE);
    if (secStatus != SECSuccess) goto error;
    secStatus = SSL_OptionSet(sslSocket, SSL_REQUEST_CERTIFICATE, PR_TRUE);
    if (secStatus != SECSuccess) goto error;

    // Setup server (peer) certificate.
    CERTCertificate  *cert = NULL;
    SECKEYPrivateKey *privKey = NULL;

    // This (my) function retrieves certificate and private key for the server
    rv = AP_GetCertificateAndPrivKey(mUserIdentity, &cert, &privKey);
    NS_ENSURE_SUCCESS(rv, rv);

    SSLKEAType certKEA = NSS_FindCertKEAType(cert);
    secStatus = SSL_ConfigSecureServer(sslSocket, cert, privKey, certKEA);
    if (secStatus != SECSuccess) goto error;

    secStatus = SSL_ResetHandshake(sslSocket, PR_TRUE);
    if (secStatus != SECSuccess) goto error;
...

    sslSocket is now propagated to the higher level as the accepted socket

This approach is almost a copy of SSLsample\server.c example. I do not need the
approach when the ssl listen socket is established and ssl sockets are directly
accepted throught this listen socket (instead of tcp listen socket and ssl
import on accepted tcp sockets).
Attached patch Patch to repairSplinter Review
I have experimented with this patch. In all cases the crash do not occure.
Assignee: wtchang → kaie.bugs
Component: Libraries → Security: PSM
Product: NSS → Core
QA Contact: jason.m.reid
Version: unspecified → Trunk
honza: I am a little bit confused.  Are you using
the mozilla/security/manager code to write your
server, and it is your server that crashed?

mozilla/security/manager is the crypto code in
the Mozilla clients, so it probably assumes that
it is an SSL client in many places.  For an SSL
client, the peer (SSL server) must have a
certificate.  I think this is why the code is not
checking for a null return value from SSL_PeerCertificate.
Just to make sure I understand everything here, Is this a pluggin to mozilla
which is acting as an SSL server?

(it looks like it since you are the COMPtr).

In this case it appears you have a CERTHandler callback meant for handling the
server cert. The patch will require more review to determine if it's applicable
to a client cert. (defaulting the 'caName' to signer may not be appropriate in
that case). It may be you need to create your own CERTHandler callback. I'm
presuming this socket is not attached to a window. It would have some confusing
UI for the user to see a 'user cert' identified as a 'server cert'.

bob
(In reply to comment #2)
> honza: I am a little bit confused.  Are you using
> the mozilla/security/manager code to write your
> server, and it is your server that crashed?
> 

Exactly. I use mozilla/security/manager code to write the server. But also I use
code from nss (SSL_* functions for configuratin and handshaking - see code in
comment #1)

The crash occures in my server (exactly the handshake callback handler attached
to the accepted socket) when external client conencts to my server under
conditions described in comment #1.

> mozilla/security/manager is the crypto code in
> the Mozilla clients, so it probably assumes that
> it is an SSL client in many places.  For an SSL
> client, the peer (SSL server) must have a
> certificate.  I think this is why the code is not
> checking for a null return value from SSL_PeerCertificate.

You want to say the default handshake calback is not designed for server sockets
but for client sockets only. That's why it presumes the peer must have the
certificate.

To note: SSLSample example (my inspiration) uses its own handshake callback.
(In reply to comment #3)
> Just to make sure I understand everything here, Is this a pluggin to mozilla
> which is acting as an SSL server?
> 
> (it looks like it since you are the COMPtr).
> 

Yes. This server is used in a P2P extension for firefox and represents a peer in
the network - certificate of the peer is attached to the server socket.

> In this case it appears you have a CERTHandler callback meant for handling the
> server cert. The patch will require more review to determine if it's applicable
> to a client cert. (defaulting the 'caName' to signer may not be appropriate in
> that case). It may be you need to create your own CERTHandler callback. I'm
> presuming this socket is not attached to a window. It would have some confusing
> UI for the user to see a 'user cert' identified as a 'server cert'.
> 

This socket is running in the backround. There is no need to atatch it to UI at
any time. 

As I noticed in reply to comment #2 I have to write my own handler for the
accepted ssl socket. As I can see in SSLSample example and nss documentation
this callback is fired at the end of successfull handshake and there is no need
for additional actions on the socket to run propertly. So, can be my handler
only an empty funciton?

-hb-
So, in some sense, this bug says that PSM function HandshakeCallback in file
nsNSSCallbacks.cpp isn't written to work in servers.  That's not accidental.
IINM, there was a conscious decision made for mozilla, years ago, to exclude
SSL server functionality from all aspects of the mozilla application suite. 
I don't recall the particular issue that led to that decision.  But 
essentially, by asking mozilla to "fix" this bug, you're asking mozilla to 
reverse that decision.  Maybe that's reasonable, but I think we should try 
to understand the original issue before we reverse that decision.  

Kai, do you recall that decision and the issue that prompted it?
note that my companies gecko based application does use nss for server sockets, 
and i think chatzilla and related creatures want to be able to do at least 
server sockets if not secure server sockets
I believe this callback is to fill in a data structure in PSM so the the
security info dialog that comes up when you click the 'lock' button works correctly.

(It could also be coming up because the certificate isn't 'Trusted' for client
auth by the 'server').

In the former case, I think your extension should supply it's own callback which
stores that cert, and your own overlay can add that information to the security
info when you are doing peer-to-peer appllications.

In the latter case you will need to verify the cert according to trust rules
appropriate to your own situation (for peer-to-peer SSL operations, usually
testing the certs for 'email' privellege is sufficient). You probably already
have a call back to override the Server cert check on the client side of your
peer-to-peer connection (because your user almost certainly does not have a
valid SSL server cert). In the same token, you either need a callback to
override the trust on the SSL client cert (NSS does not trust any SSL client
certs by default, servers have to explicitly select what certs they trust to do
SSL client auth), or explicitly trust the appropriate server certs for client auth.

Anyway you probably don't want to use PSM's default callbacks in either case.

bob
(In reply to comment #8 and all others)

Thanks for all comments. Based on them and on my current experience I decided to
use my own callback, same for server and client case. 

It checks if the issuer of the remote certificate is same as the issuer of my
own identity certificate or some specified in case I have not yet any identity. 

Before this step call to default SSL_AuthCertificate function is made. To check
the server certificate I override url associated with the socket to expected
common name of the remote peer. In that case CN of any cert have not to be a
domain name but virtual name of the peer /checked by SSL_AuthCertificate/ in one
virtual network (domain) under one certificate authority /checked by my extension/.

As you can see we are using central authority for certificate issue. There
cannot by any self-signed certificates.
Status: UNCONFIRMED → RESOLVED
Closed: 19 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: