Re: [patch]: SSL_OP_NO_RENEGOTIATION vs SSL_OP_NO_CLIENT_RENEGOTIATION inconsistency
On 23/02/06 02:24, Theo Buehler wrote: > 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. Hi Theo. Sorry about that (though thank you for making it clear). Would libressl@ have been the right place? > 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 see. Thank you for all of the other information before this as well. Reading through it helped me orient a little. I realize now that what I sent was a very naive patch, and that I really misunderstood what was going on. I underestimated how much I'd need to know to patch this. On that note, I should mention that I didn't know any C until after your mail (and from what I can tell, I still don't know nearly enough). I'm really only competent in Perl and shell. So in hindsight, I had no business offering a patch for this and I honestly feel quite embarrassed about it. Everyone makes mistakes, I guess, but still. > 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. If I felt confident in my ability to write safe, good quality C in a timely manner, I'd readily accept this. But my gut instinct tells me that it'll be a better use of everyone's time for me to properly learn C first and for someone else to take on this problem. Sorry, I really wish I could speak of this situation differently. Even if it turns out to be a trivial fix, I just don't know the fundamentals of C well enough yet to identify what that would look like. While I know that I'm capable of learning them, it'll take me a while to work through the rest of K&R---in large part due to other life events that are really vying for my attention. In any case, I do want to contribute to OpenBSD as it's my favorite OS and I use it pretty much wherever I can. Once I have a better grasp of C, I'll find a different way to help.
Re: [patch]: SSL_OP_NO_RENEGOTIATION vs SSL_OP_NO_CLIENT_RENEGOTIATION inconsistency
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 - 1.65
> +++ lib/libssl/ssl_pkt.c 5 Feb 2023 22:49:15 -
> @@ -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 - 1.230
> +++ lib/libssl/ssl.h 5 Feb 2023 22:49:16 -
> @@ -402,7 +402,7 @@ typedef int (*tls_session_secret_cb_fn)(
> /* As server, disallow session resumption on renegotiation */
> #define SSL_OP_NO_SES
Re: [patch]: SSL_OP_NO_RENEGOTIATION vs SSL_OP_NO_CLIENT_RENEGOTIATION inconsistency
Here's the other way of patching it. I don't like this way as much because it
requires more work in the future (when updating unbound/nsd and ports).
Index: usr.sbin/nsd/nsd-control.c
===
RCS file: /cvs/src/usr.sbin/nsd/nsd-control.c,v
retrieving revision 1.17
diff -u -p -u -p -r1.17 nsd-control.c
--- usr.sbin/nsd/nsd-control.c 30 Jun 2022 10:49:39 - 1.17
+++ usr.sbin/nsd/nsd-control.c 5 Feb 2023 21:55:14 -
@@ -184,11 +184,11 @@ setup_ctx(struct nsd_options* cfg)
if((SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv3) & SSL_OP_NO_SSLv3)
!= SSL_OP_NO_SSLv3)
ssl_err("could not set SSL_OP_NO_SSLv3");
-#if defined(SSL_OP_NO_RENEGOTIATION)
+#if defined(SSL_OP_NO_CLIENT_RENEGOTIATION)
/* disable client renegotiation */
- if((SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION) &
- SSL_OP_NO_RENEGOTIATION) != SSL_OP_NO_RENEGOTIATION)
- ssl_err("could not set SSL_OP_NO_RENEGOTIATION");
+ if((SSL_CTX_set_options(ctx, SSL_OP_NO_CLIENT_RENEGOTIATION) &
+ SSL_OP_NO_CLIENT_RENEGOTIATION) !=
SSL_OP_NO_CLIENT_RENEGOTIATION)
+ ssl_err("could not set SSL_OP_NO_CLIENT_RENEGOTIATION");
#endif
if(!SSL_CTX_use_certificate_file(ctx,c_cert,SSL_FILETYPE_PEM))
ssl_path_err("Error setting up SSL_CTX client cert", c_cert);
Index: usr.sbin/nsd/server.c
===
RCS file: /cvs/src/usr.sbin/nsd/server.c,v
retrieving revision 1.49
diff -u -p -u -p -r1.49 server.c
--- usr.sbin/nsd/server.c 14 Nov 2022 21:09:32 - 1.49
+++ usr.sbin/nsd/server.c 5 Feb 2023 21:55:15 -
@@ -2003,11 +2003,11 @@ server_tls_ctx_setup(char* key, char* pe
return 0;
}
#endif
-#if defined(SSL_OP_NO_RENEGOTIATION)
+#if defined(SSL_OP_NO_CLIENT_RENEGOTIATION)
/* disable client renegotiation */
- if((SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION) &
- SSL_OP_NO_RENEGOTIATION) != SSL_OP_NO_RENEGOTIATION) {
- log_crypto_err("could not set SSL_OP_NO_RENEGOTIATION");
+ if((SSL_CTX_set_options(ctx, SSL_OP_NO_CLIENT_RENEGOTIATION) &
+ SSL_OP_NO_CLIENT_RENEGOTIATION) !=
SSL_OP_NO_CLIENT_RENEGOTIATION) {
+ log_crypto_err("could not set SSL_OP_NO_CLIENT_RENEGOTIATION");
SSL_CTX_free(ctx);
return 0;
}
Index: usr.sbin/unbound/smallapp/unbound-control.c
===
RCS file: /cvs/src/usr.sbin/unbound/smallapp/unbound-control.c,v
retrieving revision 1.25
diff -u -p -u -p -r1.25 unbound-control.c
--- usr.sbin/unbound/smallapp/unbound-control.c 20 Oct 2022 08:26:14 -
1.25
+++ usr.sbin/unbound/smallapp/unbound-control.c 5 Feb 2023 21:55:15 -
@@ -538,11 +538,11 @@ setup_ctx(struct config_file* cfg)
if((SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv3) & SSL_OP_NO_SSLv3)
!= SSL_OP_NO_SSLv3)
ssl_err("could not set SSL_OP_NO_SSLv3");
-#if defined(SSL_OP_NO_RENEGOTIATION)
+#if defined(SSL_OP_NO_CLIENT_RENEGOTIATION)
/* disable client renegotiation */
- if((SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION) &
- SSL_OP_NO_RENEGOTIATION) != SSL_OP_NO_RENEGOTIATION)
- ssl_err("could not set SSL_OP_NO_RENEGOTIATION");
+ if((SSL_CTX_set_options(ctx, SSL_OP_NO_CLIENT_RENEGOTIATION) &
+ SSL_OP_NO_CLIENT_RENEGOTIATION) !=
SSL_OP_NO_CLIENT_RENEGOTIATION)
+ ssl_err("could not set SSL_OP_NO_CLIENT_RENEGOTIATION");
#endif
if(!SSL_CTX_use_certificate_chain_file(ctx,c_cert))
ssl_path_err("Error setting up SSL_CTX client cert", c_cert);
Index: usr.sbin/unbound/util/net_help.c
===
RCS file: /cvs/src/usr.sbin/unbound/util/net_help.c,v
retrieving revision 1.28
diff -u -p -u -p -r1.28 net_help.c
--- usr.sbin/unbound/util/net_help.c20 Oct 2022 08:26:14 - 1.28
+++ usr.sbin/unbound/util/net_help.c5 Feb 2023 21:55:15 -
@@ -989,11 +989,11 @@ listen_sslctx_setup(void* ctxt)
return 0;
}
#endif
-#if defined(SSL_OP_NO_RENEGOTIATION)
+#if defined(SSL_OP_NO_CLIENT_RENEGOTIATION)
/* disable client renegotiation */
- if((SSL_CTX_set_options(ctx, SSL_OP_NO_RENEGOTIATION) &
- SSL_OP_NO_RENEGOTIATION) != SSL_OP_NO_RENEGOTIATION) {
- log_crypto_err("could not set SSL_OP_NO_RENEGOTIATION");
+ if((SSL_CTX_set_options(ctx, SSL_OP_NO_CLIENT_RENEGOTIATION) &
+ SSL_OP_NO_CLIENT_RENEGOTIATION) !=
SSL_OP_NO_CLIENT_RENEGOTIATION) {
+ log_crypto_err("could not set SSL_OP_NO_CLIENT_RENEGOTIATION");
return 0;
}
#endif
@@ -1225,11 +1225,11 @@ void* connect_sslctx_create(char* key, c
SSL_
[patch]: SSL_OP_NO_RENEGOTIATION vs SSL_OP_NO_CLIENT_RENEGOTIATION inconsistency
(Can CC to tech@ or elsewhere if needed, I didn't know if it belonged here or
there so I'm starting here)
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
$ 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?
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.
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).
I'll include both methods of patching, one in this mail and one in my reply to
it.
(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.c26 Nov 2022 16:08:56 - 1.65
+++ lib/libssl/ssl_pkt.c5 Feb 2023 22:49:15 -
@@ -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.h26 Dec 2022 07:31:44 - 1.230
+++ lib/libssl/ssl.h5 Feb 2023 22:49:16 -
@@ -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 0x0001L
/* Disallow client initiated renegotiation. */
-#define SSL_OP_NO_CLIENT_RENEGOTIATION 0x0002L
+#define SSL_OP_NO_RENEGOTIATION0x0002L
/* If set, always create a new key when using tmp_dh parameters */
#define SSL_OP_SINGLE_DH_USE 0x0010L
/* 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 - 1.127
+++ lib/libssl/d1_pkt.c 5 Feb 2023 22:49:16 -
@@ -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 - 1.48
+++ lib/libtls/tls_server.c 5 Feb 2023 22:49:16 -
@@ -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) {
