Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some SSL_dup issues and provide better documentation for it #12180

Closed
wants to merge 5 commits into from

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jun 17, 2020

SSL_dup() is a very strange function and it doesn't behave the way one might expect it to, and not in accordance with the man page. There are two primary code paths:

  1. If the SSL object is in a state where the first handshake has started or completed then we up ref the SSL and return back a reference to the same SSL, i.e. it does not do a dup.

  2. If the SSL object has not yet started the first handshake then

    • We create a new SSL object from the same SSL_CTX
    • We copy some selected settings from the src SSL object to the dest SSL object

Probably when SSL_dup was first written it dup'd all of the setting in the SSL object. But it doesn't do that any more and hasn't done so for a very long time. For example a selection of settings that it doesn't "dup" include:

tlsext_host_name
tlsext_debug_callback
tlsext_status_type
mtu
psk_client_callback
psk_server_callback
psk_find_session_callback
psk_use_session_callback
max_early_data
sigalgs
client_sigalgs
min_proto_version
max_proto_version
groups
max_pipelines
...
(this is just a small selection the list goes on...)

Keeping SSL_dup maintained and current is a hard job. In practice I believe this function to be rarely used. IMO the best way forward is to simply document what it does do and not attempt to fix all the various settings.

Potentially we might consider deprecating it entirely. However I have not taken that step in this PR, as I thought that might be controversial. That could be a follow up PR.

One setting that I have fixed is that for min_proto_version/max_proto_version. This probably really should be "dup'd". Thanks to Rebekah Johnson for reporting this issue.

While looking into this problem I realised that the current BIO handling in SSL_dup is broken to the extent that if you call SSL_dup before a handshake has started, but after you have set BIOs on your SSL object, then it is likely to crash, or at least fail in some unexpected way:

Firstly the SSL_dup code was passing a BIO ** as the destination argument for BIO_dup_state. However BIO_dup_state expects a BIO * for that parameter. Any attempt to use this will either (1) fail silently, (2) crash or fail in some other strange way.

Secondly many BIOs do not implement the BIO_CTRL_DUP ctrl required to make this work.

Thirdly, if rbio == wbio in the original SSL object, then an attempt is made to up-ref the BIO in the new SSL object - even though it hasn't been set yet and is NULL. This results in a crash.

This appears to have been broken for a very long time with at least some of the problems described above coming from SSLeay. The simplest approach is to just remove this BIO dup capability from the function.

Once this is approved, I will create a 1.1.1 backport PR.

@t8m
t8m approved these changes Jun 17, 2020
Copy link
Member

@t8m t8m left a comment

LGTM. I am not sure why there is no Travis run against the latest commit though.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Jun 18, 2020

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

mattcaswell added 5 commits Jun 12, 2020
With thanks to Rebekah Johnson for reporting this issue.
SSL_dup attempted to duplicate the BIO state if the source SSL had BIOs
configured for it. This did not work.

Firstly the SSL_dup code was passing a BIO ** as the destination
argument for BIO_dup_state. However BIO_dup_state expects a BIO * for that
parameter. Any attempt to use this will either (1) fail silently, (2) crash
or fail in some other strange way.

Secondly many BIOs do not implement the BIO_CTRL_DUP ctrl required to make
this work.

Thirdly, if rbio == wbio in the original SSL object, then an attempt is made
to up-ref the BIO in the new SSL object - even though it hasn't been set
yet and is NULL. This results in a crash.

This appears to have been broken for a very long time with at least some of
the problems described above coming from SSLeay. The simplest approach is
to just remove this capability from the function.
@mattcaswell mattcaswell force-pushed the mattcaswell:ssl-dup branch to 1422774 Jun 19, 2020
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Jun 19, 2020

Travis eventually ran and reported a doc-nits issue. I've rebased this to resolve a conflict with master, and pushed a fixup to address the travis failure.

@t8m please reconfirm?

@t8m
t8m approved these changes Jun 19, 2020
Copy link
Member

@t8m t8m left a comment

Still good.

@@ -26,16 +26,9 @@ structure are freed.
SSL_up_ref() increments the reference count for an
existing B<SSL> structure.

SSL_dup() duplicates an existing B<SSL> structure into a new allocated one
or just increments the reference count if the connection is active. All

This comment has been minimized.

@kaduk

kaduk Jun 19, 2020
Contributor

We no longer document the "just increments the reference count" behavior but it is still there?

This comment has been minimized.

@mattcaswell

mattcaswell Jun 19, 2020
Author Member

It is documented. That aspect was duplicated:

"For connections that are not in their initial state SSL_dup() just increments an internal
reference count and returns the I handle."

This comment has been minimized.

@kaduk

kaduk Jun 20, 2020
Contributor

Oops, I guess I missed that. Thanks for the clarification, and sorry for the noise.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Jun 20, 2020

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Jun 23, 2020
With thanks to Rebekah Johnson for reporting this issue.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12180)
openssl-machine pushed a commit that referenced this pull request Jun 23, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12180)
openssl-machine pushed a commit that referenced this pull request Jun 23, 2020
SSL_dup attempted to duplicate the BIO state if the source SSL had BIOs
configured for it. This did not work.

Firstly the SSL_dup code was passing a BIO ** as the destination
argument for BIO_dup_state. However BIO_dup_state expects a BIO * for that
parameter. Any attempt to use this will either (1) fail silently, (2) crash
or fail in some other strange way.

Secondly many BIOs do not implement the BIO_CTRL_DUP ctrl required to make
this work.

Thirdly, if rbio == wbio in the original SSL object, then an attempt is made
to up-ref the BIO in the new SSL object - even though it hasn't been set
yet and is NULL. This results in a crash.

This appears to have been broken for a very long time with at least some of
the problems described above coming from SSLeay. The simplest approach is
to just remove this capability from the function.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12180)
openssl-machine pushed a commit that referenced this pull request Jun 23, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12180)
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Jun 23, 2020

Pushed to master. Backport to 1.1.1 in #12245.

mouse07410 added a commit to mouse07410/openssl that referenced this pull request Jun 23, 2020
With thanks to Rebekah Johnson for reporting this issue.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#12180)
mouse07410 added a commit to mouse07410/openssl that referenced this pull request Jun 23, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#12180)
mouse07410 added a commit to mouse07410/openssl that referenced this pull request Jun 23, 2020
SSL_dup attempted to duplicate the BIO state if the source SSL had BIOs
configured for it. This did not work.

Firstly the SSL_dup code was passing a BIO ** as the destination
argument for BIO_dup_state. However BIO_dup_state expects a BIO * for that
parameter. Any attempt to use this will either (1) fail silently, (2) crash
or fail in some other strange way.

Secondly many BIOs do not implement the BIO_CTRL_DUP ctrl required to make
this work.

Thirdly, if rbio == wbio in the original SSL object, then an attempt is made
to up-ref the BIO in the new SSL object - even though it hasn't been set
yet and is NULL. This results in a crash.

This appears to have been broken for a very long time with at least some of
the problems described above coming from SSLeay. The simplest approach is
to just remove this capability from the function.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#12180)
mouse07410 added a commit to mouse07410/openssl that referenced this pull request Jun 23, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#12180)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants