On Sun, Feb 05, 2023 at 03:59:38PM -0700, Ashlen wrote: > (Can CC to tech@ or elsewhere if needed, I didn't know if it belonged here or > there so I'm starting here)
Please do not send patches to misc. Many of us don't have the time and nerves to dig through all the noise to see if there's anything worth looking at. > These files in the source tree are expecting SSL_OP_NO_RENEGOTIATION when only > SSL_OP_NO_CLIENT_RENEGOTIATION is defined in lib/libssl/ssl.h. > > $ grep -Rl 'SSL_OP_NO_RENEGOTIATION' > usr.sbin/unbound/util/net_help.c > usr.sbin/unbound/smallapp/unbound-control.c > usr.sbin/nsd/server.c > usr.sbin/nsd/nsd-control.c > sbin/unwind/libunbound/util/net_help.c As you noted in your second mail, this is all third-party software. We do not want patches in there that we can't upstream. So in principle I would agree that your first patch is preferrable. > $ grep -Rl 'SSL_OP_NO_CLIENT_RENEGOTIATION' > lib/libssl/ssl_pkt.c > lib/libssl/ssl.h > lib/libssl/d1_pkt.c > lib/libtls/tls_server.c > > Is this intentional? Yes. SSL_OP_NO_CLIENT_RENEGOTIATION was introduced in LibreSSL in Jan '15 and does what it says: it turns off client-side renegotiation. I do not know if it was intentially left undocumented. https://github.com/openbsd/src/commit/0d3c1a5098b4e6a447e95479733e6abd9b485298 [If you look at the code you patch in ssl_pkt.c and d1_pkt.c, it's when the server reads a legacy (TLSv1.2 or earlier) ClientHello, so no change in behavior on the client side.] Of note: at that point renegotiation could still be turned off via the undocumented SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS. This is no longer possible since it needs an access to ssl->s3->flags and ssl is now opaque. > I should note that OpenSSL uses SSL_OP_NO_RENEGOTIATION. At least two ports > I've > seen expect this and fail to disable client renegotiation as a result. This was introduced a few months later in OpenSSL and it turns off both client-initiated and server-initiated renegotiation. The reason for adding this option was precisely that the opaque SSL in OpenSSL 1.1 did no longer allow setting SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS. https://github.com/openssl/openssl/commit/db0f35dda18403accabe98e7780f3dfc516f49de The two options don't do the same thing, so renaming SSL_OP_NO_CLIENT_RENEGOTIATION into SSL_OP_NO_RENEGOTIATION or vice versa isn't correct. > I don't know for sure which direction others would prefer to patch in, but I > get > the feeling it makes more sense to choose the approach that involves less > future > patching (renaming SSL_OP_NO_CLIENT_RENEGOTIATION to > SSL_OP_NO_RENEGOTIATION). If the two options were equivalent, another option would have been to add one compat define to ssl.h: #define SSL_OP_NO_RENEGOTIATION SSL_OP_NO_CLIENT_RENEGOTIATION This way no other patching would be needed. > I'll include both methods of patching, one in this mail and one in my reply to > it. There are a few things to consider. 1. Should we add SSL_OP_NO_RENEGOTIATION? In my opinion your findings suggest that it should be done. It should not be hard if you want to take a stab at it. 2. We can probably also remove SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS (except from ssl3.h) 3. OpenSSL 3 have disabled client-side renegotation by default. Can we do the same? (Also, they now have SSL_OP_ALLOW_CLIENT_RENEGOTIATION let's ignore this for now...) BoringSSL have intricate logic on when they allow renegotiation and when they don't, depending on the ALPN among other things. Basically, they allow it for TLSv1.2 with HTTP/1.1, but disable it once they know they use HTTP/2. Should we do similar instead? > (Also, should lib/libssl/man/SSL_CTX_set_options.3 also get patched? Unsure > what > to write there if so, as it depends on which solution makes more sense) > > Index: lib/libssl/ssl_pkt.c > =================================================================== > RCS file: /cvs/src/lib/libssl/ssl_pkt.c,v > retrieving revision 1.65 > diff -u -p -u -p -r1.65 ssl_pkt.c > --- lib/libssl/ssl_pkt.c 26 Nov 2022 16:08:56 -0000 1.65 > +++ lib/libssl/ssl_pkt.c 5 Feb 2023 22:49:15 -0000 > @@ -958,7 +958,7 @@ ssl3_read_handshake_unexpected(SSL *s) > return -1; > } > > - if ((s->options & SSL_OP_NO_CLIENT_RENEGOTIATION) != 0) { > + if ((s->options & SSL_OP_NO_RENEGOTIATION) != 0) { > ssl3_send_alert(s, SSL3_AL_FATAL, > SSL_AD_NO_RENEGOTIATION); > return -1; > Index: lib/libssl/ssl.h > =================================================================== > RCS file: /cvs/src/lib/libssl/ssl.h,v > retrieving revision 1.230 > diff -u -p -u -p -r1.230 ssl.h > --- lib/libssl/ssl.h 26 Dec 2022 07:31:44 -0000 1.230 > +++ lib/libssl/ssl.h 5 Feb 2023 22:49:16 -0000 > @@ -402,7 +402,7 @@ typedef int (*tls_session_secret_cb_fn)( > /* As server, disallow session resumption on renegotiation */ > #define SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION 0x00010000L > /* Disallow client initiated renegotiation. */ > -#define SSL_OP_NO_CLIENT_RENEGOTIATION 0x00020000L > +#define SSL_OP_NO_RENEGOTIATION 0x00020000L > /* If set, always create a new key when using tmp_dh parameters */ > #define SSL_OP_SINGLE_DH_USE 0x00100000L > /* Set on servers to choose the cipher according to the server's > Index: lib/libssl/d1_pkt.c > =================================================================== > RCS file: /cvs/src/lib/libssl/d1_pkt.c,v > retrieving revision 1.127 > diff -u -p -u -p -r1.127 d1_pkt.c > --- lib/libssl/d1_pkt.c 26 Nov 2022 16:08:55 -0000 1.127 > +++ lib/libssl/d1_pkt.c 5 Feb 2023 22:49:16 -0000 > @@ -644,7 +644,7 @@ dtls1_read_handshake_unexpected(SSL *s) > return -1; > } > > - if ((s->options & SSL_OP_NO_CLIENT_RENEGOTIATION) != 0) { > + if ((s->options & SSL_OP_NO_RENEGOTIATION) != 0) { > ssl3_send_alert(s, SSL3_AL_FATAL, > SSL_AD_NO_RENEGOTIATION); > return -1; > Index: lib/libtls/tls_server.c > =================================================================== > RCS file: /cvs/src/lib/libtls/tls_server.c,v > retrieving revision 1.48 > diff -u -p -u -p -r1.48 tls_server.c > --- lib/libtls/tls_server.c 19 Jan 2022 11:10:55 -0000 1.48 > +++ lib/libtls/tls_server.c 5 Feb 2023 22:49:16 -0000 > @@ -231,7 +231,7 @@ tls_configure_server_ssl(struct tls *ctx > goto err; > } > > - SSL_CTX_set_options(*ssl_ctx, SSL_OP_NO_CLIENT_RENEGOTIATION); > + SSL_CTX_set_options(*ssl_ctx, SSL_OP_NO_RENEGOTIATION); > > if (SSL_CTX_set_tlsext_servername_callback(*ssl_ctx, > tls_servername_cb) != 1) { >
