370970 - Server SSL code should invalidate Session ID when redoing handshake

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect, P1)

Description

[Steve Parkinson]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 2

[Steve Parkinson]

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.)

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Steve Parkinson]

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

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 6

[Steve Parkinson]

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. 

Comment 7

[Steve Parkinson]

Attachment: new function for setting requireclientauth mode

Comment 8

[Thomas Kwan]

Comment on attachment 256223 [details] [diff] [review]
new function for setting requireclientauth mode

Looks good

Comment 9

[glen beasley]

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.

Comment 10

[Steve Parkinson]

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?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

someone, please set the target fix version to the next JSS version.
Thomas, maybe you should "take" this bug.

Comment 12

[glen beasley]

(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?

Comment 13

[Wan-Teh Chang]

I prefer one method, requireClientAuth(int mode).

Comment 14

[Thomas Kwan]

requireClientAuth(int mode) will give flexibility to developer to support new mode in NSS without changing JSS.

Comment 15

[Steve Parkinson]

Attachment: Add @deprecated, added constants to enum list, add SSLServerSocket impl

I decided to just implement as one method.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 17

[Steve Parkinson]

Attachment: Additional context for diff

Comment 18

[Steve Parkinson]

Attachment: mark pacth as 'patch' in bugzilla

Sorry about the mess.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 20

[Steve Parkinson]

Attachment: more context

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 22

[glen beasley]

(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 23

[Thomas Kwan]

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

Comment 24

[Thomas Kwan]

Plus, see nelson's comment #12

Comment 25

[Steve Parkinson]

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.

Comment 26

[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. 

Comment 27

[Steve Parkinson]

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.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

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.  

Comment 29

[glen beasley]

Attachment: please test

I still need to work on this patch, but I won't have time till next week.

Comment 30

[glen beasley]

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); 

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 32

[Steve Parkinson]

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.

Comment 33

[Wan-Teh Chang]

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.

Comment 34

[glen beasley]

Attachment: requireClientAuth

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)

Comment 35

[Steve Parkinson]

Comment on attachment 258037 [details] [diff] [review]
requireClientAuth

I checked the code, applied the patch. Everything looks fine to me.

Comment 36

[Steve Parkinson]

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);

Comment 37

[glen beasley]

steve why do you want to change code that has been deprecated and 
should be no longer used? 

Comment 38

[Steve Parkinson]

Comment on attachment 258037 [details] [diff] [review]
requireClientAuth

That's okay. I don't feel too strongly about it.

Comment 39

[Thomas Kwan]

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!

Comment 40

[glen beasley]

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

Comment 42

[Slavomir Katuscak]

Glen, the last patch causes JSS tests hangs. Please fix this or return to previous version ASAP. See bug 374756.

Comment 43

[glen beasley]

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