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 (1.1.1) #12245

Closed

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jun 23, 2020

This is a backport to 1.1.1 of #12180. That PR had this description:

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.

mattcaswell added 4 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
Copy link
Member Author

@mattcaswell mattcaswell commented Jun 23, 2020

@t8m - since you approved the master version of this, perhaps you could look at this backport?

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

@t8m t8m left a comment

I am approving anyway as the sentence is in master as well. But feel free to change it.

For SSL_dup() to work, the connection MUST be in its initial state and
MUST NOT have not yet have started the SSL handshake. For connections
Comment on lines 33 to 34

This comment has been minimized.

@t8m

t8m Jun 23, 2020
Member

MUST NOT have not yet have started? That looks almost incomprehensible to me.

I know the same text is in master.

@openssl-machine
Copy link

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

This pull request is ready to merge

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Jun 25, 2020

I've added an additional commit to fix the bad wording. My plan is to merge all the commits here to 1.1.1, and forward port the last commit only to master (hence I've added the "master" label to this PR).

@t8m - please can you reconfirm your approval?

@t8m
Copy link
Member

@t8m t8m commented Jun 25, 2020

I am OK with this and with adding the last commit to master as well.

@openssl-machine
Copy link

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

This pull request is ready to merge

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

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12245)
openssl-machine pushed a commit that referenced this pull request Jun 30, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12245)
openssl-machine pushed a commit that referenced this pull request Jun 30, 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 #12245)
openssl-machine pushed a commit that referenced this pull request Jun 30, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12245)
openssl-machine pushed a commit that referenced this pull request Jun 30, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12245)
openssl-machine pushed a commit that referenced this pull request Jun 30, 2020
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12245)

(cherry picked from commit 0c3d024)
@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Jun 30, 2020

Pushed to 1.1.1. The final commit has also been pushed to master.

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

3 participants