Closed
Bug 303603
Opened 19 years ago
Closed 19 years ago
crash during ssl server socket handshake
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: bambas, Assigned: KaiE)
Details
Attachments
(1 file)
947 bytes,
patch
|
Details | Diff | Splinter Review |
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).
Reporter | ||
Comment 1•19 years ago
|
||
I have experimented with this patch. In all cases the crash do not occure.
Updated•19 years ago
|
Assignee: wtchang → kaie.bugs
Component: Libraries → Security: PSM
Product: NSS → Core
QA Contact: jason.m.reid
Version: unspecified → Trunk
Comment 2•19 years ago
|
||
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.
Comment 3•19 years ago
|
||
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
Reporter | ||
Comment 4•19 years ago
|
||
(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.
Reporter | ||
Comment 5•19 years ago
|
||
(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-
Comment 6•19 years ago
|
||
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
Comment 8•19 years ago
|
||
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
Reporter | ||
Comment 9•19 years ago
|
||
(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.
Description
•