Re: Server TLS renegotiation issues with tc-native

[Konstantin Kolinko]

2017-08-11 18:20 GMT+03:00 Mark Thomas :
> On 09/08/17 15:51, Mark Thomas wrote:
>
>> I'll take a look at this. I think the hint about the statistics hack
>> will help me get further than I have so far. Whether it is far enough,
>> we'll see.
>
> As it happened, the statistics weren't the fix I needed but they got me
> looking in the right direction where I found SSL_renegotiate_pending() -
> thanks for the hint.
>
> I have attached my proposed patch for review.

1. Sanity check:  You need to explicitly initialize new local variable
"error = 0".

Current code initializes it only when "if (retVal < 1)" and subsequent
if (error == SSL_ERROR_WANT_READ) reads an uninitialized value.

Local variables need explicit initialization in C (they do not default to 0).
https://stackoverflow.com/questions/14049777/why-are-global-variables-always-initialized-to-0-but-not-local-variables

2. Many new lines have tab characters and some have trailing whitespace.

Best regards,
Konstantin Kolinko

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Server TLS renegotiation issues with tc-native

[Mark Thomas]

On 09/08/17 15:51, Mark Thomas wrote:

> I'll take a look at this. I think the hint about the statistics hack
> will help me get further than I have so far. Whether it is far enough,
> we'll see.

As it happened, the statistics weren't the fix I needed but they got me
looking in the right direction where I found SSL_renegotiate_pending() -
thanks for the hint.

I have attached my proposed patch for review.

It seems to be moving back towards an a previous approach. The commit
history suggests this approach should not be necessary. I have tested
both 7.0.x and 9.0.x and both versions need the additional reads. I'm
not 100% sure what is going on.

If there aren't any objections, I plan to apply this patch roll 1.2.13
in time for it to be included in the September round of Tomcat releases.

Mark
Index: native/src/sslnetwork.c
===
--- native/src/sslnetwork.c	(revision 180)
+++ native/src/sslnetwork.c	(working copy)
@@ -365,7 +365,7 @@
 * Check for failed client authentication
 */
 if (con->ctx->verify_mode != SSL_VERIFY_NONE &&
-	(vr = SSL_get_verify_result(con->ssl)) != X509_V_OK) {
+(vr = SSL_get_verify_result(con->ssl)) != X509_V_OK) {
 
 if (SSL_VERIFY_ERROR_IS_OPTIONAL(vr) &&
 con->ctx->verify_mode == SSL_CVERIFY_OPTIONAL_NO_CA) {
@@ -622,8 +622,9 @@
 {
 tcn_socket_t *s   = J2P(sock, tcn_socket_t *);
 tcn_ssl_conn_t *con;
-int retVal;
+int retVal, error;
 char peekbuf[1];
+apr_interval_time_t timeout;
 
 UNREFERENCED_STDARGS;
 TCN_ASSERT(sock != 0);
@@ -633,28 +634,59 @@
  * handshake to proceed.
  */
 con->reneg_state = RENEG_ALLOW;
+
+// Schedule a renegotiation request
 retVal = SSL_renegotiate(con->ssl);
 if (retVal <= 0)
 return APR_EGENERAL;
 
-retVal = SSL_do_handshake(con->ssl);
-if (retVal <= 0)
-return APR_EGENERAL;
-if (!SSL_is_init_finished(con->ssl)) {
-return APR_EGENERAL;
-}
-
-/* Need to trigger renegotiation handshake by reading.
+/* Need to trigger the renegotiation handshake by reading.
  * Peeking 0 bytes actually works.
  * See: http://marc.info/?t=14549335922&r=1&w=2
+ *
+ * This will normally return SSL_ERROR_WANT_READ whether the renegotiation
+ * has been completed or not. Afterwards, need to determine if I/O needs to
+ * be triggered or not.
  */
-SSL_peek(con->ssl, peekbuf, 0);
+retVal = SSL_peek(con->ssl, peekbuf, 0);
+if (retVal < 1) {
+error = SSL_get_error(con->ssl, retVal);
+}
 
-con->reneg_state = RENEG_REJECT;
+apr_socket_timeout_get(con->sock, &timeout);
+// If the renegotiation is still pending, then I/O needs to be triggered
+while (SSL_renegotiate_pending(con->ssl)) {
+		if (error == SSL_ERROR_WANT_READ) {
+			retVal = wait_for_io_or_timeout(con, error, timeout);
+			/*
+			 * Since this is blocking I/O, anything other than APR_SUCCESS is an
+			 * error.
+			 */
+			if (retVal != APR_SUCCESS) {
+printf("ERROR\n");
+con->shutdown_type = SSL_SHUTDOWN_TYPE_UNCLEAN;
+return retVal;
+			}
+		} else {
+			// SSL_ERROR_WANT_READ is expected. Anything else is an error.
+			return APR_EGENERAL;
+		}
 
-if (!SSL_is_init_finished(con->ssl)) {
-return APR_EGENERAL;
+		// Re-try SSL_peek after I/O
+		retVal = SSL_peek(con->ssl, peekbuf, 0);
+	if (retVal < 1) {
+	error = SSL_get_error(con->ssl, retVal);
+	} else {
+		/*
+		 * Reset error to handle case where SSL_Peek returns 0 but
+		 * SSL_renegotiate_pending returns true. This will trigger an error
+		 * to be returned.
+		 */
+		error = 0;
+	}
 }
+
+con->reneg_state = RENEG_REJECT;
 
 return APR_SUCCESS;
 }


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Server TLS renegotiation issues with tc-native

[Mark Thomas]

On 09/08/17 08:43, Rémy Maucherat wrote:
> On Tue, Aug 8, 2017 at 10:07 PM, Mark Thomas  wrote:
> 
>> Hi,
>>
>> The good news is I have managed to unpick the various TLS issues I've
>> been struggling with.
>>
>> The Chrome not selecting the user cert issue looks to be related to how
>> many of the fields were complete in the DN. That has been resolved by
>> recreating the test keys and certs I have been using.
>>
>> I was also seeing some ugly stack traces in the console during
>> renegotiation. The root cause is the final issue in this mail but the
>> stack traces were a sign of some edge cases in the error handling. These
>> have been fixed in r1804463.
>>
>> NIO and NIO2 with JSSE work as expected.
>>
>> NIO and NIO2 with OpenSSL mostly work as expected. The only glitch is
>> that because we don't specify a set of trusted certs, OpenSSL doesn't
>> send any CA names to the user agent which in turn opts to display all of
>> the available client certs. This is more a usability issue for users
>> with lots of certs than a bug. I'll create a Bugzilla entry for this one.
>>
>> The final issue is the one I have tried, and failed, to solve. The
>> renegotiation sequence with APR/native doesn't work as expected. I've
>> been debugging may way through the call to SSL.renegotiate(long) and
>> this is what happens:
>>
>> SSL_renegotiate() returns 1 (success)
>> SSL_do_handshake() returns 1 (success)
>> SSL_is_init_finished() returns 1 (success)
>> SSL_peek() returns -1 (failure)
>>   At this point the client prompts the user to select a certificate
>>   The return code is not currently tested so the code continues
>>   (If SSL_get_error() is called here, SSL_ERROR_WANT_READ is returned)
>> SSL_is_init_finished() returns 1 (success)
>>
>> Execution returns to the Java code where no certs are found on the
>> connection so a 401 response is sent back to the client.
>>
>> Shortly afterwards the user selects a certificate, the handshake
>> continues and then the connection fails as soon as the handshake completes.
>>
>> I think the behaviour we are seeing is because the connection is in
>> non-blocking mode. I have tried various ways to handle this without
>> success.
>>
>> Help with a solution very much appreciated.
>>
> I think it would need the exact same algorithm as in OpenSSLEngine then
> (which is a bit complex ...). renegociate actually does nothing, in
> particular the SSL state remains as if the handshake was complete (as you
> can see with the SSL isininit call). Actually, it needs to wrap/unwrap a
> few times before it's actually done. To detect that the rehandshake is
> done, you can either use the callback or hack using a statistics
> (OpenSSLEngine calls  SSL.getHandshakeCount to see if a handshake was done
> after IO operations). So that would probably be a significant amount of
> native code changes.

I'll take a look at this. I think the hint about the statistics hack
will help me get further than I have so far. Whether it is far enough,
we'll see.

> IMO it's not really worth adding to APR, shouldn't we tell people to use
> JSSE/OpenSSL instead now ?

That is certainly an option. I'm not quite ready to give up on fixing
this yet but if we aren't able to fix this then I'd be happy with that
work-around.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Server TLS renegotiation issues with tc-native

[Rémy Maucherat]

On Tue, Aug 8, 2017 at 10:07 PM, Mark Thomas  wrote:

> Hi,
>
> The good news is I have managed to unpick the various TLS issues I've
> been struggling with.
>
> The Chrome not selecting the user cert issue looks to be related to how
> many of the fields were complete in the DN. That has been resolved by
> recreating the test keys and certs I have been using.
>
> I was also seeing some ugly stack traces in the console during
> renegotiation. The root cause is the final issue in this mail but the
> stack traces were a sign of some edge cases in the error handling. These
> have been fixed in r1804463.
>
> NIO and NIO2 with JSSE work as expected.
>
> NIO and NIO2 with OpenSSL mostly work as expected. The only glitch is
> that because we don't specify a set of trusted certs, OpenSSL doesn't
> send any CA names to the user agent which in turn opts to display all of
> the available client certs. This is more a usability issue for users
> with lots of certs than a bug. I'll create a Bugzilla entry for this one.
>
> The final issue is the one I have tried, and failed, to solve. The
> renegotiation sequence with APR/native doesn't work as expected. I've
> been debugging may way through the call to SSL.renegotiate(long) and
> this is what happens:
>
> SSL_renegotiate() returns 1 (success)
> SSL_do_handshake() returns 1 (success)
> SSL_is_init_finished() returns 1 (success)
> SSL_peek() returns -1 (failure)
>   At this point the client prompts the user to select a certificate
>   The return code is not currently tested so the code continues
>   (If SSL_get_error() is called here, SSL_ERROR_WANT_READ is returned)
> SSL_is_init_finished() returns 1 (success)
>
> Execution returns to the Java code where no certs are found on the
> connection so a 401 response is sent back to the client.
>
> Shortly afterwards the user selects a certificate, the handshake
> continues and then the connection fails as soon as the handshake completes.
>
> I think the behaviour we are seeing is because the connection is in
> non-blocking mode. I have tried various ways to handle this without
> success.
>
> Help with a solution very much appreciated.
>
> I think it would need the exact same algorithm as in OpenSSLEngine then
(which is a bit complex ...). renegociate actually does nothing, in
particular the SSL state remains as if the handshake was complete (as you
can see with the SSL isininit call). Actually, it needs to wrap/unwrap a
few times before it's actually done. To detect that the rehandshake is
done, you can either use the callback or hack using a statistics
(OpenSSLEngine calls  SSL.getHandshakeCount to see if a handshake was done
after IO operations). So that would probably be a significant amount of
native code changes.

IMO it's not really worth adding to APR, shouldn't we tell people to use
JSSE/OpenSSL instead now ?

Rémy

Server TLS renegotiation issues with tc-native

[Mark Thomas]

Hi,

The good news is I have managed to unpick the various TLS issues I've
been struggling with.

The Chrome not selecting the user cert issue looks to be related to how
many of the fields were complete in the DN. That has been resolved by
recreating the test keys and certs I have been using.

I was also seeing some ugly stack traces in the console during
renegotiation. The root cause is the final issue in this mail but the
stack traces were a sign of some edge cases in the error handling. These
have been fixed in r1804463.

NIO and NIO2 with JSSE work as expected.

NIO and NIO2 with OpenSSL mostly work as expected. The only glitch is
that because we don't specify a set of trusted certs, OpenSSL doesn't
send any CA names to the user agent which in turn opts to display all of
the available client certs. This is more a usability issue for users
with lots of certs than a bug. I'll create a Bugzilla entry for this one.

The final issue is the one I have tried, and failed, to solve. The
renegotiation sequence with APR/native doesn't work as expected. I've
been debugging may way through the call to SSL.renegotiate(long) and
this is what happens:

SSL_renegotiate() returns 1 (success)
SSL_do_handshake() returns 1 (success)
SSL_is_init_finished() returns 1 (success)
SSL_peek() returns -1 (failure)
  At this point the client prompts the user to select a certificate
  The return code is not currently tested so the code continues
  (If SSL_get_error() is called here, SSL_ERROR_WANT_READ is returned)
SSL_is_init_finished() returns 1 (success)

Execution returns to the Java code where no certs are found on the
connection so a 401 response is sent back to the client.

Shortly afterwards the user selects a certificate, the handshake
continues and then the connection fails as soon as the handshake completes.

I think the behaviour we are seeing is because the connection is in
non-blocking mode. I have tried various ways to handle this without success.

Help with a solution very much appreciated.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org