nsock: infinite loop "reconnecting with SSL_OP_NO_SSLv2"

[cnotin]

In nsock_core.c there's a feature to try reconnect with the SSL_OP_NO_SSLv2 option in some cases:

nmap/nsock/src/nsock_core.c

Lines 462 to 470 in 12f1894

	} else if (!(options & SSL_OP_NO_SSLv2)) {
	int saved_ev;
	/* SSLv3-only and TLSv1-only servers can't be connected to when the
	* SSL_OP_NO_SSLv2 option is not set, which is the case when the pool
	* was initialized with nsock_pool_ssl_init_max_speed. Try reconnecting
	* with SSL_OP_NO_SSLv2. Never downgrade a NO_SSLv2 connection to one
	* that might use SSLv2. */
	nsock_log_info("EID %li reconnecting with SSL_OP_NO_SSLv2", nse->id);

The problem is that this triggers infinite loops every time there's an SSL error, but unrelated.
For example I had this with a server presenting a weak DH key (now refused by recent openssl), cf. #1485

Now, I've observed it when running the ssl-cert script against a SSL server that requires a client certificate (with latest version from source):

# ~/tools/nmap/nmap --script +ssl-cert -dd -vv a.b.c.d -p 8090
Starting Nmap 7.70SVN ( https://nmap.org ) at ....
[...]
NSE: Starting ssl-cert M:560f4a4504e8 against a.b.c.d:8090.
NSOCK INFO [0.3520s] nsock_iod_new2(): nsock_iod_new (IOD #1)
NSOCK INFO [0.4640s] nsock_connect_ssl(): SSL connection requested to a.b.c.d:8090/tcp (IOD #1) EID 9
NSOCK INFO [0.4860s] handle_connect_result(): EID 9 reconnecting with SSL_OP_NO_SSLv2
NSOCK INFO [0.5020s] handle_connect_result(): EID 9 reconnecting with SSL_OP_NO_SSLv2
NSOCK INFO [0.5210s] handle_connect_result(): EID 9 reconnecting with SSL_OP_NO_SSLv2
NSOCK INFO [0.5390s] handle_connect_result(): EID 9 reconnecting with SSL_OP_NO_SSLv2
NSOCK INFO [0.5610s] handle_connect_result(): EID 9 reconnecting with SSL_OP_NO_SSLv2
[...]

Confirmation that the problem comes from a required client cert:

# openssl s_client -connect a.b.c.d:8090 | grep -i client
depth=1 CN = Domain
verify error:num=19:self signed certificate in certificate chain
140146447311936:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../ssl/record/rec_layer_s3.c:1528:SSL alert number 40
Acceptable client certificate CA names
Client Certificate Types: RSA sign, DSA sign, ECDSA sign

[cnotin]

I think the issue here is that the SSL_OP_NO_SSLv2 option was removed from openssl:
https://github.com/openssl/openssl/blob/088dfa133561d7613b9391a56ddbce58f32c934a/include/openssl/ssl.h#L448-L449

/* Removed from OpenSSL 1.1.0. Was 0x01000000L */
# define SSL_OP_NO_SSLv2                                 0x0

Which is what I observed after this modification:

diff --git a/nsock/src/nsock_core.c b/nsock/src/nsock_core.c
index 117e0aa7e..01ad10749 100644
--- a/nsock/src/nsock_core.c
+++ b/nsock/src/nsock_core.c
@@ -484,7 +484,12 @@ void handle_connect_result(struct npool *ms, struct nevent *nse, enum nse_status
         if (!iod->ssl)
           fatal("SSL_new failed: %s", ERR_error_string(ERR_get_error(), NULL));
 
+        printf("options=%x\n",options);
+        printf("options | SSL_OP_NO_SSLv2=%x\n",(options | SSL_OP_NO_SSLv2));
+        printf("SSL_OP_NO_SSLv2=%x\n",SSL_OP_NO_SSLv2);
+        printf("OPTIONS BEFORE: %x\n", SSL_get_options(iod->ssl));
         SSL_set_options(iod->ssl, options | SSL_OP_NO_SSLv2);
+        printf("OPTIONS AFTER: %x\n", SSL_get_options(iod->ssl));
         socket_count_read_inc(nse->iod);
         socket_count_write_inc(nse->iod);
         update_events(iod, ms, nse, EV_READ|EV_WRITE, EV_NONE);

->

options=80120854
options | SSL_OP_NO_SSLv2=80120854
SSL_OP_NO_SSLv2=0
OPTIONS BEFORE: 80120854
OPTIONS AFTER: 80120854
NSOCK INFO [0.6130s] handle_connect_result(): EID 9 reconnecting with SSL_OP_NO_SSLv2

Therefore, this is always true hence the infinite loop:

nmap/nsock/src/nsock_core.c

Line 462 in 12f1894

} else if (!(options & SSL_OP_NO_SSLv2)) {

[dmiller-nmap]

Thanks for catching this! It looks like OpenSSL prefers the SSL_set_min_proto_version function for this in 1.1.0 and later. It would be better to use that when available and avoid a runtime check like the one in #1489.

[cnotin]

Good idea! Something like this?

diff --git a/nsock/src/nsock_core.c b/nsock/src/nsock_core.c
index 117e0aa7e..76983b08b 100644
--- a/nsock/src/nsock_core.c
+++ b/nsock/src/nsock_core.c
@@ -459,15 +459,15 @@ void handle_connect_result(struct npool *ms, struct nevent *nse, enum nse_status
         nse->sslinfo.ssl_desire = sslerr;
         socket_count_write_inc(iod);
         update_events(iod, ms, nse, EV_WRITE, EV_NONE);
-      } else if (!(options & SSL_OP_NO_SSLv2)) {
+      } else if (!(SSL_get_min_proto_version(iod->ssl) >= SSL3_VERSION)) {
         int saved_ev;
 
         /* SSLv3-only and TLSv1-only servers can't be connected to when the
-         * SSL_OP_NO_SSLv2 option is not set, which is the case when the pool
-         * was initialized with nsock_pool_ssl_init_max_speed. Try reconnecting
-         * with SSL_OP_NO_SSLv2. Never downgrade a NO_SSLv2 connection to one
-         * that might use SSLv2. */
-        nsock_log_info("EID %li reconnecting with SSL_OP_NO_SSLv2", nse->id);
+         * SSLv2 version is proposed, which might be case when the pool
+         * was initialized with nsock_pool_ssl_init_max_speed.
+         * Try reconnecting with minimum version SSL3_VERSION (no SSLv2).
+         * Never downgrade a NO_SSLv2 connection to one that might use SSLv2. */
+        nsock_log_info("EID %li reconnecting with minium version: SSL3_VERSION", nse->id);
 
         saved_ev = iod->watched_events;
         nsock_engine_iod_unregister(ms, iod);
@@ -478,13 +478,16 @@ void handle_connect_result(struct npool *ms, struct nevent *nse, enum nse_status
 
         /* Use SSL_free here because SSL_clear keeps session info, which
          * doesn't work when changing SSL versions (as we're clearly trying to
-         * do by adding SSL_OP_NO_SSLv2). */
+         * do by setting a minimum version). */
         SSL_free(iod->ssl);
         iod->ssl = SSL_new(ms->sslctx);
         if (!iod->ssl)
           fatal("SSL_new failed: %s", ERR_error_string(ERR_get_error(), NULL));
 
-        SSL_set_options(iod->ssl, options | SSL_OP_NO_SSLv2);
+        rc = SSL_set_min_proto_version(iod->ssl, SSL3_VERSION);
+        if (rc == 0)
+          nsock_log_error("Uh-oh: SSL_set_min_proto_version() failed - please tell dev@nmap.org");
+
         socket_count_read_inc(nse->iod);
         socket_count_write_inc(nse->iod);
         update_events(iod, ms, nse, EV_READ|EV_WRITE, EV_NONE);

I'm testing against a mock server with the command below, but it seems to fail anyway. Same with #1489. Let me check

socat -v OPENSSL-LISTEN:443,reuseaddr,fork,verify=1,cert=site.pem -

[cnotin]

Nevermind, I'm confusing it with another issue #1490

At least with this patch it doesn't go in a loop:

NSOCK INFO [0.2630s] nsock_iod_new2(): nsock_iod_new (IOD #1)
NSOCK INFO [0.3580s] nsock_connect_ssl(): SSL connection requested to 127.0.0.1:443/tcp (IOD #1) EID 9
NSOCK INFO [0.3720s] handle_connect_result(): EID 9 reconnecting with minium version: SSL3_VERSION
NSOCK INFO [0.3880s] handle_connect_result(): EID 9 error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure
NSOCK INFO [0.3880s] nsock_trace_handler_callback(): Callback: SSL-CONNECT ERROR [Input/output error (5)] for EID 9 [127.0.0.1:443]
NSE: [ssl-cert M:5583a6fa9bd8 127.0.0.1:443] getCertificate error: Failed to connect to server
NSE: Finished ssl-cert M:5583a6fa9bd8 against 127.0.0.1:443.

[dmiller-nmap]

That's closer, but the getter functions aren't available before 1.1.1, and the setter functions aren't available before 1.1.0. We need to maintain compatibility back to 1.0.2 at least.

[cnotin]

I see... Hum hum, how to do that the best 🤔

[cnotin]

If you have any hint or pointer I can try something :)

[nnposter]

I could be missing something but I do not see why a simple conditional compilation is not perfectly sufficient. See #1873