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)
Tracking
(Not tracked)
RESOLVED
FIXED
4.2.5
People
(Reporter: stevepnscp, Assigned: glenbeasley)
References
Details
Attachments
(1 file, 6 obsolete files)
16.68 KB,
patch
|
stevepnscp
:
review+
nkwan
:
superreview+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
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?
Reporter | ||
Comment 2•18 years ago
|
||
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•18 years ago
|
||
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.
Reporter | ||
Comment 4•18 years ago
|
||
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•18 years ago
|
||
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.
Reporter | ||
Comment 6•18 years ago
|
||
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
Reporter | ||
Comment 7•18 years ago
|
||
Reporter | ||
Updated•18 years ago
|
Attachment #256223 -
Flags: superreview?(glen.beasley)
Attachment #256223 -
Flags: review?(nkwan)
Comment 8•18 years ago
|
||
Comment on attachment 256223 [details] [diff] [review]
new function for setting requireclientauth mode
Looks good
Attachment #256223 -
Flags: review?(nkwan) → review+
Assignee | ||
Comment 9•18 years ago
|
||
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-
Reporter | ||
Comment 10•18 years ago
|
||
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•18 years ago
|
||
someone, please set the target fix version to the next JSS version.
Thomas, maybe you should "take" this bug.
Priority: -- → P2
Assignee | ||
Comment 12•18 years ago
|
||
(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
Comment 13•18 years ago
|
||
I prefer one method, requireClientAuth(int mode).
Updated•18 years ago
|
Assignee: nobody → nkwan
Comment 14•18 years ago
|
||
requireClientAuth(int mode) will give flexibility to developer to support new mode in NSS without changing JSS.
Reporter | ||
Comment 15•18 years ago
|
||
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 16•18 years ago
|
||
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.
Reporter | ||
Comment 17•18 years ago
|
||
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)
Reporter | ||
Comment 18•18 years ago
|
||
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 19•18 years ago
|
||
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.
Updated•18 years ago
|
Attachment #257608 -
Attachment is patch: true
Attachment #257608 -
Attachment mime type: application/octet-stream → text/plain
Reporter | ||
Comment 20•18 years ago
|
||
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 21•18 years ago
|
||
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?
Assignee | ||
Comment 22•18 years ago
|
||
(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•18 years ago
|
||
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+
Comment 24•18 years ago
|
||
Plus, see nelson's comment #12
Reporter | ||
Comment 25•18 years ago
|
||
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)
Assignee | ||
Comment 26•18 years ago
|
||
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.
Reporter | ||
Comment 27•18 years ago
|
||
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•18 years ago
|
||
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.
Assignee | ||
Comment 29•18 years ago
|
||
I still need to work on this patch, but I won't have time till next week.
Assignee | ||
Comment 30•18 years ago
|
||
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•18 years ago
|
||
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.
Reporter | ||
Comment 32•18 years ago
|
||
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•18 years ago
|
||
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.
Assignee | ||
Comment 34•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #258037 -
Flags: superreview?(nkwan) → superreview+
Reporter | ||
Comment 35•18 years ago
|
||
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+
Reporter | ||
Comment 36•18 years ago
|
||
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-
Assignee | ||
Comment 37•18 years ago
|
||
steve why do you want to change code that has been deprecated and
should be no longer used?
Reporter | ||
Comment 38•18 years ago
|
||
Comment on attachment 258037 [details] [diff] [review]
requireClientAuth
That's okay. I don't feel too strongly about it.
Attachment #258037 -
Flags: review- → review+
Comment 39•18 years ago
|
||
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!
Assignee | ||
Comment 40•18 years ago
|
||
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
Comment 42•18 years ago
|
||
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 → ---
Updated•18 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 43•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•