You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2018/10/10 21:49:55 UTC
svn commit: r1843514 - in /tomcat/native/trunk/native: include/ssl_private.h
src/sslnetwork.c src/sslutils.c
Author: markt
Date: Wed Oct 10 21:49:55 2018
New Revision: 1843514
URL: http://svn.apache.org/viewvc?rev=1843514&view=rev
Log:
Implement TLS 1.3 support for CLIENT-CERT when the APR/native connector is not configured with certificateVerification="required" (i.e. the equivalent of server initiated renegotiation to obtain a client cert)
Modified:
tomcat/native/trunk/native/include/ssl_private.h
tomcat/native/trunk/native/src/sslnetwork.c
tomcat/native/trunk/native/src/sslutils.c
Modified: tomcat/native/trunk/native/include/ssl_private.h
URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/include/ssl_private.h?rev=1843514&r1=1843513&r2=1843514&view=diff
==============================================================================
--- tomcat/native/trunk/native/include/ssl_private.h (original)
+++ tomcat/native/trunk/native/include/ssl_private.h Wed Oct 10 21:49:55 2018
@@ -324,7 +324,7 @@ struct tcn_ssl_conf_ctxt_t {
SSL_CONF_CTX *cctx;
};
#endif
-
+
typedef struct {
apr_pool_t *pool;
tcn_ssl_ctxt_t *ctx;
@@ -335,7 +335,7 @@ typedef struct {
* that all client-initiated renegotiations can be rejected, as a
* partial fix for CVE-2009-3555.
*/
- enum {
+ enum {
RENEG_INIT = 0, /* Before initial handshake */
RENEG_REJECT, /* After initial handshake; any client-initiated
* renegotiation should be rejected
@@ -347,6 +347,13 @@ typedef struct {
* connection
*/
} reneg_state;
+#if defined(SSL_OP_NO_TLSv1_3)
+ enum {
+ PHA_NONE = 0, /* Before PHA */
+ PHA_STARTED, /* PHA req sent to client but no response */
+ PHA_COMPLETE /* Client has returned cert */
+ } pha_state;
+#endif
apr_socket_t *sock;
apr_pollset_t *pollset;
} tcn_ssl_conn_t;
@@ -356,7 +363,7 @@ typedef struct {
* Additional Functions
*/
void SSL_init_app_data2_3_idx(void);
-/* The app_data2 is used to store the tcn_ssl_ctxt_t pointer for the SSL instance. */
+/* The app_data2 is used to store the tcn_ssl_ctxt_t pointer for the SSL instance. */
void *SSL_get_app_data2(SSL *);
void SSL_set_app_data2(SSL *, void *);
/* The app_data3 is used to store the handshakeCount pointer for the SSL instance. */
Modified: tomcat/native/trunk/native/src/sslnetwork.c
URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslnetwork.c?rev=1843514&r1=1843513&r2=1843514&view=diff
==============================================================================
--- tomcat/native/trunk/native/src/sslnetwork.c (original)
+++ tomcat/native/trunk/native/src/sslnetwork.c Wed Oct 10 21:49:55 2018
@@ -625,69 +625,130 @@ TCN_IMPLEMENT_CALL(jint, SSLSocket, rene
int error = 0;
char peekbuf[1];
apr_interval_time_t timeout;
+ const SSL_SESSION *session;
UNREFERENCED_STDARGS;
TCN_ASSERT(sock != 0);
con = (tcn_ssl_conn_t *)s->opaque;
+ session = SSL_get_session(con->ssl);
+ apr_socket_timeout_get(con->sock, &timeout);
- /* Toggle the renegotiation state to allow the new
- * handshake to proceed.
- */
- con->reneg_state = RENEG_ALLOW;
-
- // Schedule a renegotiation request
- retVal = SSL_renegotiate(con->ssl);
- if (retVal <= 0)
- return APR_EGENERAL;
-
- /* Need to trigger the renegotiation handshake by reading.
- * Peeking 0 bytes actually works.
- * See: http://marc.info/?t=145493359200002&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.
- */
- retVal = SSL_peek(con->ssl, peekbuf, 0);
- if (retVal < 1) {
- error = SSL_get_error(con->ssl, retVal);
- }
+#if defined(SSL_OP_NO_TLSv1_3)
+ if (SSL_SESSION_get_protocol_version(session) == TLS1_3_VERSION) {
+ // TLS 1.3 renegotiation
+ retVal = SSL_verify_client_post_handshake(con->ssl);
+ if (retVal <= 0) {
+ return APR_EGENERAL;
+ }
+
+ con->pha_state = PHA_STARTED;
+
+ // Need to trigger a write operation to sent the cert request to the
+ // client. As per OpenSSL docs, use SSL_do_handshake() for this.
+ retVal = SSL_do_handshake(con->ssl);
+ if (retVal <= 0) {
+ return APR_EGENERAL;
+ }
+
+ // Trigger reading of the certs from the client
+ retVal = SSL_peek(con->ssl, peekbuf, 0);
+ if (retVal < 1) {
+ error = SSL_get_error(con->ssl, retVal);
+ }
+
+ // If the certs have not been received, then need to wait for I/O
+ while (con->pha_state == PHA_STARTED) {
+ // SSL_ERROR_WANT_READ is expected. Anything else is an error.
+ 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) {
+ con->shutdown_type = SSL_SHUTDOWN_TYPE_UNCLEAN;
+ return retVal;
+ }
+ } else {
+ 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
+ * con->pha_state has not changed. This will trigger an error
+ * to be returned.
+ */
+ error = 0;
+ }
+ }
+ } else {
+#endif
+ // TLS 1.2 and earlier renegotiation
+
+ /* Toggle the renegotiation state to allow the new
+ * handshake to proceed.
+ */
+ con->reneg_state = RENEG_ALLOW;
+
+ // Schedule a renegotiation request
+ retVal = SSL_renegotiate(con->ssl);
+ if (retVal <= 0) {
+ return APR_EGENERAL;
+ }
+
+ /* Need to trigger the renegotiation handshake by reading.
+ * Peeking 0 bytes actually works.
+ * See: http://marc.info/?t=145493359200002&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.
+ */
+ retVal = SSL_peek(con->ssl, peekbuf, 0);
+ if (retVal < 1) {
+ error = SSL_get_error(con->ssl, retVal);
+ }
+
+ // If the renegotiation is still pending, then I/O needs to be triggered
+ while (SSL_renegotiate_pending(con->ssl)) {
+ // SSL_ERROR_WANT_READ is expected. Anything else is an error.
+ 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) {
+ con->shutdown_type = SSL_SHUTDOWN_TYPE_UNCLEAN;
+ return retVal;
+ }
+ } else {
+ 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;
+ }
+ }
- 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)) {
- // SSL_ERROR_WANT_READ is expected. Anything else is an error.
- 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 {
- 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;
+#if defined(SSL_OP_NO_TLSv1_3)
}
-
- con->reneg_state = RENEG_REJECT;
-
+#endif
return APR_SUCCESS;
}
Modified: tomcat/native/trunk/native/src/sslutils.c
URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslutils.c?rev=1843514&r1=1843513&r2=1843514&view=diff
==============================================================================
--- tomcat/native/trunk/native/src/sslutils.c (original)
+++ tomcat/native/trunk/native/src/sslutils.c Wed Oct 10 21:49:55 2018
@@ -305,6 +305,10 @@ int SSL_callback_SSL_verify(int ok, X509
int verify = con->ctx->verify_mode;
int depth = con->ctx->verify_depth;
+#if defined(SSL_OP_NO_TLSv1_3)
+ con->pha_state = PHA_COMPLETE;
+#endif
+
if (verify == SSL_CVERIFY_UNSET ||
verify == SSL_CVERIFY_NONE)
return 1;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1843514 - in /tomcat/native/trunk/native:
include/ssl_private.h src/sslnetwork.c src/sslutils.c
Posted by Rainer Jung <ra...@kippdata.de>.
Am 12.10.2018 um 14:40 schrieb Rainer Jung:
> Am 12.10.2018 um 14:02 schrieb Mark Thomas:
>> On 12/10/18 12:11, Rainer Jung wrote:
>>> Am 10.10.2018 um 23:54 schrieb Mark Thomas:
>>>> On 10/10/18 22:49, markt@apache.org wrote:
>>>>> Author: markt
>>>>> Date: Wed Oct 10 21:49:55 2018
>>>>> New Revision: 1843514
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1843514&view=rev
>>>>> Log:
>>>>> Implement TLS 1.3 support for CLIENT-CERT when the APR/native
>>>>> connector is not configured with certificateVerification="required"
>>>>> (i.e. the equivalent of server initiated renegotiation to obtain a
>>>>> client cert)
>>>>>
>>>>> Modified:
>>>>> tomcat/native/trunk/native/include/ssl_private.h
>>>>> tomcat/native/trunk/native/src/sslnetwork.c
>>>>
>>>> There is a large amount of duplication in this commit for the above
>>>> file. A C programmer with more skill than me can probably find a simple
>>>> way to reduce it.
>>>
>>> I hope I have done it without breaking it in r1843645 and r1843651. It
>>> compiles with OpenSSL 1.0.2, 1.1.0 and 1.1.1 and the refactoring isn't
>>> very complex. Do you have an efficient way of testing whether I broke
>>> reneg or PHA?
>>
>> Thanks for cleaning up after me. Much appreciated.
>>
>> I've tested TLS 1.2 and TLS 1.3 with APR/native and NIO+OpenSSL and
>> reneg and PHA both work as expected.
>>
>> Many thanks,
>
> Thanks for doing the hard part of it :)
Sorry, I forgot: of course big thanks to Chris and our community testers
as well!
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1843514 - in /tomcat/native/trunk/native:
include/ssl_private.h src/sslnetwork.c src/sslutils.c
Posted by Rainer Jung <ra...@kippdata.de>.
Am 12.10.2018 um 14:02 schrieb Mark Thomas:
> On 12/10/18 12:11, Rainer Jung wrote:
>> Am 10.10.2018 um 23:54 schrieb Mark Thomas:
>>> On 10/10/18 22:49, markt@apache.org wrote:
>>>> Author: markt
>>>> Date: Wed Oct 10 21:49:55 2018
>>>> New Revision: 1843514
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1843514&view=rev
>>>> Log:
>>>> Implement TLS 1.3 support for CLIENT-CERT when the APR/native
>>>> connector is not configured with certificateVerification="required"
>>>> (i.e. the equivalent of server initiated renegotiation to obtain a
>>>> client cert)
>>>>
>>>> Modified:
>>>> tomcat/native/trunk/native/include/ssl_private.h
>>>> tomcat/native/trunk/native/src/sslnetwork.c
>>>
>>> There is a large amount of duplication in this commit for the above
>>> file. A C programmer with more skill than me can probably find a simple
>>> way to reduce it.
>>
>> I hope I have done it without breaking it in r1843645 and r1843651. It
>> compiles with OpenSSL 1.0.2, 1.1.0 and 1.1.1 and the refactoring isn't
>> very complex. Do you have an efficient way of testing whether I broke
>> reneg or PHA?
>
> Thanks for cleaning up after me. Much appreciated.
>
> I've tested TLS 1.2 and TLS 1.3 with APR/native and NIO+OpenSSL and
> reneg and PHA both work as expected.
>
> Many thanks,
Thanks for doing the hard part of it :)
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1843514 - in /tomcat/native/trunk/native:
include/ssl_private.h src/sslnetwork.c src/sslutils.c
Posted by Mark Thomas <ma...@apache.org>.
On 12/10/18 12:11, Rainer Jung wrote:
> Am 10.10.2018 um 23:54 schrieb Mark Thomas:
>> On 10/10/18 22:49, markt@apache.org wrote:
>>> Author: markt
>>> Date: Wed Oct 10 21:49:55 2018
>>> New Revision: 1843514
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1843514&view=rev
>>> Log:
>>> Implement TLS 1.3 support for CLIENT-CERT when the APR/native
>>> connector is not configured with certificateVerification="required"
>>> (i.e. the equivalent of server initiated renegotiation to obtain a
>>> client cert)
>>>
>>> Modified:
>>> tomcat/native/trunk/native/include/ssl_private.h
>>> tomcat/native/trunk/native/src/sslnetwork.c
>>
>> There is a large amount of duplication in this commit for the above
>> file. A C programmer with more skill than me can probably find a simple
>> way to reduce it.
>
> I hope I have done it without breaking it in r1843645 and r1843651. It
> compiles with OpenSSL 1.0.2, 1.1.0 and 1.1.1 and the refactoring isn't
> very complex. Do you have an efficient way of testing whether I broke
> reneg or PHA?
Thanks for cleaning up after me. Much appreciated.
I've tested TLS 1.2 and TLS 1.3 with APR/native and NIO+OpenSSL and
reneg and PHA both work as expected.
Many thanks,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1843514 - in /tomcat/native/trunk/native:
include/ssl_private.h src/sslnetwork.c src/sslutils.c
Posted by Rainer Jung <ra...@kippdata.de>.
Am 12.10.2018 um 14:15 schrieb Rémy Maucherat:
> On Fri, Oct 12, 2018 at 1:11 PM Rainer Jung <ra...@kippdata.de> wrote:
>
>> Am 10.10.2018 um 23:54 schrieb Mark Thomas:
>>> On 10/10/18 22:49, markt@apache.org wrote:
>>>> Author: markt
>>>> Date: Wed Oct 10 21:49:55 2018
>>>> New Revision: 1843514
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1843514&view=rev
>>>> Log:
>>>> Implement TLS 1.3 support for CLIENT-CERT when the APR/native connector
>> is not configured with certificateVerification="required" (i.e. the
>> equivalent of server initiated renegotiation to obtain a client cert)
>>>>
>>>> Modified:
>>>> tomcat/native/trunk/native/include/ssl_private.h
>>>> tomcat/native/trunk/native/src/sslnetwork.c
>>>
>>> There is a large amount of duplication in this commit for the above
>>> file. A C programmer with more skill than me can probably find a simple
>>> way to reduce it.
>>
>> I hope I have done it without breaking it in r1843645 and r1843651. It
>> compiles with OpenSSL 1.0.2, 1.1.0 and 1.1.1 and the refactoring isn't
>> very complex. Do you have an efficient way of testing whether I broke
>> reneg or PHA?
>>
>
> Nice, no more warnings for me, and it still builds with my obsolete OpenSSL
> 1.1.0.
Thanks for confirming, much appreciated!
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1843514 - in /tomcat/native/trunk/native:
include/ssl_private.h src/sslnetwork.c src/sslutils.c
Posted by Rémy Maucherat <re...@apache.org>.
On Fri, Oct 12, 2018 at 1:11 PM Rainer Jung <ra...@kippdata.de> wrote:
> Am 10.10.2018 um 23:54 schrieb Mark Thomas:
> > On 10/10/18 22:49, markt@apache.org wrote:
> >> Author: markt
> >> Date: Wed Oct 10 21:49:55 2018
> >> New Revision: 1843514
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1843514&view=rev
> >> Log:
> >> Implement TLS 1.3 support for CLIENT-CERT when the APR/native connector
> is not configured with certificateVerification="required" (i.e. the
> equivalent of server initiated renegotiation to obtain a client cert)
> >>
> >> Modified:
> >> tomcat/native/trunk/native/include/ssl_private.h
> >> tomcat/native/trunk/native/src/sslnetwork.c
> >
> > There is a large amount of duplication in this commit for the above
> > file. A C programmer with more skill than me can probably find a simple
> > way to reduce it.
>
> I hope I have done it without breaking it in r1843645 and r1843651. It
> compiles with OpenSSL 1.0.2, 1.1.0 and 1.1.1 and the refactoring isn't
> very complex. Do you have an efficient way of testing whether I broke
> reneg or PHA?
>
Nice, no more warnings for me, and it still builds with my obsolete OpenSSL
1.1.0.
Rémy
Re: svn commit: r1843514 - in /tomcat/native/trunk/native:
include/ssl_private.h src/sslnetwork.c src/sslutils.c
Posted by Rainer Jung <ra...@kippdata.de>.
Am 10.10.2018 um 23:54 schrieb Mark Thomas:
> On 10/10/18 22:49, markt@apache.org wrote:
>> Author: markt
>> Date: Wed Oct 10 21:49:55 2018
>> New Revision: 1843514
>>
>> URL: http://svn.apache.org/viewvc?rev=1843514&view=rev
>> Log:
>> Implement TLS 1.3 support for CLIENT-CERT when the APR/native connector is not configured with certificateVerification="required" (i.e. the equivalent of server initiated renegotiation to obtain a client cert)
>>
>> Modified:
>> tomcat/native/trunk/native/include/ssl_private.h
>> tomcat/native/trunk/native/src/sslnetwork.c
>
> There is a large amount of duplication in this commit for the above
> file. A C programmer with more skill than me can probably find a simple
> way to reduce it.
I hope I have done it without breaking it in r1843645 and r1843651. It
compiles with OpenSSL 1.0.2, 1.1.0 and 1.1.1 and the refactoring isn't
very complex. Do you have an efficient way of testing whether I broke
reneg or PHA?
Thanks and regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1843514 - in /tomcat/native/trunk/native:
include/ssl_private.h src/sslnetwork.c src/sslutils.c
Posted by Mark Thomas <ma...@apache.org>.
On 10/10/18 22:49, markt@apache.org wrote:
> Author: markt
> Date: Wed Oct 10 21:49:55 2018
> New Revision: 1843514
>
> URL: http://svn.apache.org/viewvc?rev=1843514&view=rev
> Log:
> Implement TLS 1.3 support for CLIENT-CERT when the APR/native connector is not configured with certificateVerification="required" (i.e. the equivalent of server initiated renegotiation to obtain a client cert)
>
> Modified:
> tomcat/native/trunk/native/include/ssl_private.h
> tomcat/native/trunk/native/src/sslnetwork.c
There is a large amount of duplication in this commit for the above
file. A C programmer with more skill than me can probably find a simple
way to reduce it.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org