Closed Bug 370970 Opened 18 years ago Closed 18 years ago

Server SSL code should invalidate Session ID when redoing handshake

Categories

(JSS Graveyard :: Library, defect, P1)

x86
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stevepnscp, Assigned: glenbeasley)

References

Details

Attachments

(1 file, 6 obsolete files)

I have encountered a problem with a recent upgrade to Windows Server 2003 SP1. I am trying to do a client-auth from the above O/S, using Microsoft's WinHTTP API to a NSS-based SSL server: - The client first connects, does an initial handshake, then does it's HTTP POST. - The server, upon seeing that this URL requires client-auth, sends a 'Hello request' message. - The client responds with a client-hello with the session ID set to the 32-byte value it was given in the initial server-hello - The server then sends a server-hello, but does NOT include a 'certificate request' message. So, the client never prompts for a certificate. In contrast, Firefox sends the second client-hello with session ID set to nothing, the server sends a certificate request message, and things work just fine. My guess is that something changed during 2003 SP1, (possibly a side-effect of Microsoft fixing this bug: http://support.microsoft.com/kb/826240/ ) which causes the second client-auth to reuse the old session ID. Bob R suggested it might be an easy fix for us to be more tolerant of this behavior if we were to invalidate the session ID for the socket when we send a Hello Request. The behavior also manifests itself with IE, in strange ways which I can't quite fathom, .... sometimes it works, sometimes not, depending on first visit, shift reload, phase of the moon, etc.
Steve, I think you're saying that the second handshake being done is not a full handshake, but rather is a restart handshake. Yes? What value is your server setting for the SSL_REQUIRE_CERTIFICATE socket option before doing this second handshake?
Yes, it's a restart handshake, in the same TCP session. I set a breakpoint on SSL_OptionSet, and after the URL is processed by the server, it calls: Breakpoint 1, SSL_OptionSet (fd=0x82eab18, which=3, on=1) at sslsock.c:532 So, it's using SSL_REQUEST_CERTIFICATE(3) not, SSL_REQUIRE_CERTIFICATE(=10) (Which is the correct behavior as far as I am concerned, because I don't want a hard failure.)
Steve, both SSL_REQUEST_CERTIFICATE and SSL_REQUIRE_CERTIFICATE are socket variables that have default values unless your app changes them. The value of SSL_REQUEST_CERTIFICATE controls whether or not libSSL even attempts to request client certs when it does a full handshake. It is a boolean. The default value for the SSL_REQUEST_CERTIFICATE option is 0 (false). The value of SSL_REQUIRE_CERTIFICATE controls other aspects of the handshake in which client cert auth is requested. It controls (a) whether the handshake will fail if the client fails to authenticate, and (b) whether a full handshake will be forced, to ensure that the cert request is actually sent to the client, even if the client attempts to restart an old SSL session. The value of SSL_REQUIRE_CERTIFICATE is meaningless unless SSL_REQUEST_CERTIFICATE is set to a true (non-zero) value. The 4 defined values for SSL_REQUIRE_CERTIFICATE are shown in this table: numeric cert full HS name value required forced ------------------------- ----- -------- ------- SSL_REQUIRE_NEVER 0 no no SSL_REQUIRE_ALWAYS 1 yes yes SSL_REQUIRE_FIRST_HANDSHAKE 2 yes* yes* (default) SSL_REQUIRE_NO_ERROR 3 no yes Note *: only on the first handshake on the socket, not on subsequent handshakes on the same socket/connection. The default value for the SSL_REQUIRE_CERTIFICATE option is 2, which is SSL_REQUIRE_FIRST_HANDSHAKE. If you turn on SSL_REQUEST_CERTIFICATE and leave the value of SSL_REQUIRE_CERTIFICATE at its default value, then libSSL will require a certificate in the FIRST handshake done on a socket, but not on a second or subsequent handshakes done on that connection. It will force the first handshake on a socket to be a full handshake, but will not force subsequent handshakes to be full handshakes. Steve, I think you want to set SSL_REQUIRE_HANDSHAKE to SSL_REQUIRE_NO_ERROR.
My server code is java, and I use JSS. It looks like we need to change JSS to support this, as there is no option to pass-in the value '3'. http://lxr.mozilla.org/security/source/security/jss/org/mozilla/jss/ssl/SocketBase.java#303 These options all seem to be conflated to me. Its confusing :-(
So, maybe this bug should change into a JSS RFE? SSL_REQUIRE_CERTIFICATE has always had 3 recognized values, 0, 1 and 2, with 2 being the default. The function to set that value takes a PRBool which is an enumerated type that only uses the values 0 and 1. So, the way it originally worked (when I inherited this code in 1997) was that if you wanted the value 2, you didn't set it, and if you wanted the value 0 or 1 you set it. It became clear a few years ago that none of the options 0, 1 or 2 really provided the right combination of features (actually requiring a client cert, and forcing a full handshake) for the web server, so I added the value 3, and made it so that you could set any of the 4 defined values. I wouldn't have designed this API this way, but I didn't design it. I inherited it, and we've preserved it for backwards compatibility.
Confirmed that SSL_REQUIRE_NO_ERROR works as desired. Changing bug to JSS RFE. I will upload a diff. The diff supplies a new API for JSS so that applications can set the SSL_REQUIRE_CERTIFICATE API to any of the 4 values above. But, this is an unfortunate situation. The new Windows 2003 sp1 will not work properly with existing JSS-based servers, or any NSS server which does not set the value to 3.
Component: Libraries → Library
Product: NSS → JSS
Version: 3.11.4 → 4.2
Attachment #256223 - Flags: superreview?(glen.beasley)
Attachment #256223 - Flags: review?(nkwan)
Comment on attachment 256223 [details] [diff] [review] new function for setting requireclientauth mode Looks good
Attachment #256223 - Flags: review?(nkwan) → review+
Comment on attachment 256223 [details] [diff] [review] new function for setting requireclientauth mode The patch is incomplete, You need to provide requireClientAuth in SSLServerSocket.java http://lxr.mozilla.org/security/source/security/jss/org/mozilla/jss/ssl/SSLServerSocket.java#465 In keeping with the JSS style you should not have + public final static int SSL_REQUIRE_NEVER = 0; + public final static int SSL_REQUIRE_ALWAYS = 1; + public final static int SSL_REQUIRE_FIRST_HANDSHAKE = 2; + public final static int SSL_REQUIRE_NO_ERROR = 3; + take a look at SocketBase.java http://lxr.mozilla.org/security/source/security/jss/org/mozilla/jss/ssl/SocketBase.java#95 and common.c http://lxr.mozilla.org/security/source/security/jss/org/mozilla/jss/ssl/common.c#374 the current requireClientAuth methods need to be marked as * @deprecated As of JSS 4.2.4 since they only provide SSL_REQUIRE_NEVER, SSL_REQUIRE_ALWAYS, SSL_REQUIRE_FIRST_HANDSHAKE.
Attachment #256223 - Flags: superreview?(glen.beasley) → superreview-
Hi, thanks for the comments. I agree with the SSLServerSocket requirement, and the @deprecated comment. For the constants, I'm not sure what the problem is. They are not in the same 'namespace' as the links you pointed to - those are option names, whereas my definitions are for values of the SSL_REQUIRE_CERTIFICATE option. Or perhaps you meant that I should put the definitions in SocketBase?
someone, please set the target fix version to the next JSS version. Thomas, maybe you should "take" this bug.
Priority: -- → P2
(In reply to comment #10) > For the constants, I'm not sure what the problem is. They are not in the same > 'namespace' as the links you pointed to - those are option names, whereas my > definitions are for values of the SSL_REQUIRE_CERTIFICATE option. > > Or perhaps you meant that I should put the definitions in SocketBase? yes, put the definitions in SocketBase. take a look at SocketBase.java http://lxr.mozilla.org/security/source/security/jss/org/mozilla/jss/ssl/SocketBase.java#95 and common.c http://lxr.mozilla.org/security/source/security/jss/org/mozilla/jss/ssl/common.c#374 note ssl.h : #define SSL_REQUIRE_FIRST_HANDSHAKE ((PRBool)2) http://lxr.mozilla.org/security/source/security/nss/lib/ssl/ssl.h#159 I am in debate if you should have four separate methods i.e: requireClientAuth_Never() requireClientAuth_Always() requireClientAuth_FirstHandshake() requireClientAuth_No_Error() or requireClientAuth(int mode) Wan-teh what do you think?
Target Milestone: --- → 4.2.5
I prefer one method, requireClientAuth(int mode).
Assignee: nobody → nkwan
requireClientAuth(int mode) will give flexibility to developer to support new mode in NSS without changing JSS.
I decided to just implement as one method.
Attachment #256223 - Attachment is obsolete: true
Attachment #257579 - Flags: superreview?(glen.beasley)
Attachment #257579 - Flags: review?(nkwan)
Comment on attachment 257579 [details] [diff] [review] Add @deprecated, added constants to enum list, add SSLServerSocket impl Could you please regenerate this patch with more than zero lines of context? cvs diff -pu5 ought to suffice.
Attached patch Additional context for diff (obsolete) — Splinter Review
Attachment #257579 - Attachment is obsolete: true
Attachment #257608 - Flags: superreview?(glen.beasley)
Attachment #257608 - Flags: review?(nkwan)
Attachment #257579 - Flags: superreview?(glen.beasley)
Attachment #257579 - Flags: review?(nkwan)
Sorry about the mess.
Attachment #257608 - Attachment is obsolete: true
Attachment #257609 - Flags: superreview?(glen.beasley)
Attachment #257609 - Flags: review?(nkwan)
Attachment #257608 - Flags: superreview?(glen.beasley)
Attachment #257608 - Flags: review?(nkwan)
Comment on attachment 257609 [details] [diff] [review] mark pacth as 'patch' in bugzilla Sorry, this patch still has zero lines of context. Look at the diff commands in the patch itself. They read like this: diff -u -p -0 That zero causes zero lines of context.
Attachment #257608 - Attachment is patch: true
Attachment #257608 - Attachment mime type: application/octet-stream → text/plain
Attached patch more context (obsolete) — Splinter Review
Attachment #257609 - Attachment is obsolete: true
Attachment #257616 - Flags: superreview?(glen.beasley)
Attachment #257616 - Flags: review?(nkwan)
Attachment #257609 - Flags: superreview?(glen.beasley)
Attachment #257609 - Flags: review?(nkwan)
Comment on attachment 257616 [details] [diff] [review] more context Thanks for the context! Question: given that your code defines these symbols with these values: + static final int SSL_REQUIRE_NEVER = 18; + static final int SSL_REQUIRE_ALWAYS = 19; + static final int SSL_REQUIRE_FIRST_HANDSHAKE = 20; + static final int SSL_REQUIRE_NO_ERROR = 21; while NSS defines them as 0-3 respectively, I would expect to see this patch include some code somewhere that subtracts 18 from the Java number to get the NSS number. But I didn't see it. Is there another part to this patch?
(In reply to comment #21) > (From update of attachment 257616 [details] [diff] [review]) > Thanks for the context! > > Question: given that your code defines these symbols with these values: > > + static final int SSL_REQUIRE_NEVER = 18; > + static final int SSL_REQUIRE_ALWAYS = 19; > + static final int SSL_REQUIRE_FIRST_HANDSHAKE = 20; > + static final int SSL_REQUIRE_NO_ERROR = 21; > > while NSS defines them as 0-3 respectively, I would expect to see > this patch include some code somewhere that subtracts 18 from > the Java number to get the NSS number. But I didn't see it. > Is there another part to this patch? > nelson see comment #12
Comment on attachment 257616 [details] [diff] [review] more context * deprecated note is now added * constants are added to the base class * there is still a minor indentation problem in SSLSocket.java
Attachment #257616 - Flags: review?(nkwan) → review+
Plus, see nelson's comment #12
Comment on attachment 257616 [details] [diff] [review] more context The function setSSLOption will translate its first argument (which is actually an index into a C array of 'real' NSS option values. Unfortunately, it doesn't do that for the second argument. Any ideas on how to fix this Glen? I could make another setSSLoption, which would translate both arguments, or another function to expose the translation back to java and do it there.
Attachment #257616 - Attachment is obsolete: true
Attachment #257616 - Flags: superreview?(glen.beasley)
I plan to apply your patch and test and make minor changes. Can you give me till monday? I am currently working on an escalation.
To be clear, Nelson's comments on the patch were correct.. the parameter passed to the 'mode' argument of the functions will not be translated to the correct NSS values. So, it's pointless to review this as-is.
I noticed another thing about the use of the JSSL_enums array. It is used in 5 functions: - Java_org_mozilla_jss_ssl_SSLSocket_setSSLDefaultOption - Java_org_mozilla_jss_ssl_SSLSocket_getSSLDefaultOption - Java_org_mozilla_jss_ssl_SSLSocket_shutdownNative - Java_org_mozilla_jss_ssl_SocketBase_setSSLOption - Java_org_mozilla_jss_ssl_SocketBase_getSSLOption Each of these 5 functions has an integer (jint) parameter that it uses immediately as an index into the JSSL_enums array. I didn't notice any code that checks to make sure that the value of that jint index is in bounds before using it to index the array. Maybe it's impossible for them to be called with an out-of-range value because they're private double-secret functions, only callable by other methods of the same class, and those methods only use known-good constant values. I hope so, because if any of them is directly callable from other Java code, it would only take one call with a wild index value to crash the code.
Attached patch please test (obsolete) — Splinter Review
I still need to work on this patch, but I won't have time till next week.
I only tested the following serverSock = new SSLServerSocket (port, 5, InetAddress.getByName (fServerHost), null , true); serverSock.requireClientAuth(SSLSocket.SSL_REQUIRE_NO_ERROR); I did a quick review of my patch, and there is an error in common.c Java_org_mozilla_jss_ssl_SocketBase_setSSLOptions (JNIEnv *env, jobject self, jint option, jint mode) needs to have > status = SSL_OptionSet(sock->fd, JSSL_enums[option], JSSL_enums[mode]); not status = SSL_OptionSet(sock->fd, JSSL_enums[option], on);
Glenn, Perhaps your proposed change is appropriate when the second argument is SSL_REQUIRE_CERTIFICATE, but it is not appropriate for other options. Most callers of this function will be passing a boolean value (0, 1) as the third argument. The translation you propose will translate those two values into one of the following: PRInt32 JSSL_enums[] = { SSL_ENABLE_SSL2, (which is 7) SSL_ENABLE_SSL3, (which is 8) Clearly that translation is inappropriate for those other options.
Can I just define the variables in java like in the original patch? It seems there is a lot of obfuscation here for not much gain.
If you add these four constants to the JSSL_enums array, you need to have requireClientAuth call a native method, something like this: In the SocketBase.java: void requireClientAuth(int mode) throws SocketException { if( mode > 0 && !requestingClientAuth ) { requestClientAuth(true); } requireClientAuthNative(mode); } private native void requireClientAuthNative(int mode) throws SocketException; In common.c: JNIEXPORT void JNICALL Java_org_mozilla_jss_ssl_SocketBase_requireClientAuthNative (JNIEnv *env, jobject self, jint mode) { SECStatus status; JSSL_SocketData *sock = NULL; /* get my fd */ if( JSSL_getSockData(env, self, &sock) != PR_SUCCESS ) { goto finish; } /* set the option */ status = SSL_OptionSet(sock->fd, SSL_REQUIRE_CERTIFICATE, JSSL_enums[mode]); if( status != SECSuccess ) { JSSL_throwSSLSocketException(env, "SSL_OptionSet failed"); goto finish; } finish: EXCEPTION_CHECK(env, sock) return; } I agree this is a lot of work for little benefit.
First I totally agree that this is a lot of work for little benefit. I have alway felt this way about the enum table in common.c and SocketBase.java. But the JSS coding style has used the JSSL_enum table for 18 NSS/NSPR defines, and I don't see why we should make an exception for these four. I do not plan to check in test/SSLClientAuth.java as is. It is a test program and I wanted to show I tested all cases. I provided it in this patch so you could easily test if you wanted. keeping with the JSS coding style there are existing functions which enable/disable an SSL option. Java_org_mozilla_jss_ssl_SSLSocket_setSSLDefaultOption; Java_org_mozilla_jss_ssl_SocketBase_setSSLOption; therefore I made the following functions to set the mode for an SSL option on a SSLSocket if that SSL option has more values than just enable/disable. Java_org_mozilla_jss_ssl_SSLSocket_setSSLDefaultOptionMode; Java_org_mozilla_jss_ssl_SocketBase_setSSLOptionMode; I also don't understand why in SSLSocket.java we had public void requireClientAuthDefault(boolean require, boolean onRedo) it actually has two bugs, one it should of been defined as static since its purpose is to set the Default options for all new sockets, and it also should of set SSL_REQUEST_CERTIFICATE. but since we had that function I did create static public void requireClientAuthDefault(int mode)
Assignee: nkwan → glen.beasley
Attachment #257841 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #258037 - Flags: superreview?(nkwan)
Attachment #258037 - Flags: review?(sparkins)
Attachment #258037 - Flags: superreview?(nkwan) → superreview+
Comment on attachment 258037 [details] [diff] [review] requireClientAuth I checked the code, applied the patch. Everything looks fine to me.
Attachment #258037 - Flags: review?(sparkins) → review+
Comment on attachment 258037 [details] [diff] [review] requireClientAuth We should change these lines to use the new constants: setSSLDefaultOption(SocketBase.SSL_REQUIRE_CERTIFICATE, require ? (onRedo ? 1 : 2) : 0); setSSLOption(SSL_REQUIRE_CERTIFICATE, require ? (onRedo ? 1 : 2) : 0);
Attachment #258037 - Flags: review+ → review-
steve why do you want to change code that has been deprecated and should be no longer used?
Comment on attachment 258037 [details] [diff] [review] requireClientAuth That's okay. I don't feel too strongly about it.
Attachment #258037 - Flags: review- → review+
Glen, do you think you can check the fix in JSS 4.2.4 and the trunk. When do you think you have time to check it in soon? thanks for your help again!
Checking in ssl/SSLServerSocket.java; /cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLServerSocket.java,v <-- SSLServerSocket.java new revision: 1.23; previous revision: 1.22 done Checking in ssl/SSLSocket.c; /cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.c,v <-- SSLSocket.c new revision: 1.24; previous revision: 1.23 done Checking in ssl/SSLSocket.java; /cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SSLSocket.java,v <-- SSLSocket.java new revision: 1.26; previous revision: 1.25 done Checking in ssl/SocketBase.java; /cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/SocketBase.java,v <-- SocketBase.java new revision: 1.14; previous revision: 1.13 done Checking in ssl/common.c; /cvsroot/mozilla/security/jss/org/mozilla/jss/ssl/common.c,v <-- common.c new revision: 1.27; previous revision: 1.26 done Checking in tests/JSS_FileUploadServer.java; /cvsroot/mozilla/security/jss/org/mozilla/jss/tests/JSS_FileUploadServer.java,v <-- JSS_FileUploadServer.java new revision: 1.2; previous revision: 1.1 done Checking in tests/JSS_SSLServer.java; /cvsroot/mozilla/security/jss/org/mozilla/jss/tests/JSS_SSLServer.java,v <-- JSS_SSLServer.java new revision: 1.9; previous revision: 1.8 done Checking in tests/JSS_SelfServServer.java; /cvsroot/mozilla/security/jss/org/mozilla/jss/tests/JSS_SelfServServer.java,v <-- JSS_SelfServServer.java new revision: 1.4; previous revision: 1.3 done Checking in tests/SSLClientAuth.java; /cvsroot/mozilla/security/jss/org/mozilla/jss/tests/SSLClientAuth.java,v <-- SSLClientAuth.java new revision: 1.5; previous revision: 1.4 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Glen, the last patch causes JSS tests hangs. Please fix this or return to previous version ASAP. See bug 374756.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: P2 → P1
Sorry I forgot to check in the def file. Checking in jss.def; /cvsroot/mozilla/security/jss/lib/jss.def,v <-- jss.def new revision: 1.38; previous revision: 1.37 done
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: