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

SSL: Workaround to remove SNI from ClientHello #9408

Merged
merged 1 commit into from Feb 11, 2021

Conversation

sepalani
Copy link
Contributor

This PR is a dirty workaround to remove the SNI extension from ClientHello. This PR is an alternative to #9406.

It does that by using the only hook that I could find that can be used between handshake steps (see mbedtls_ssl_handshake_step). My approach uses the send callback called in mbedtls_ssl_flush_output at the beginning of each handshake step. I delayed mbedtls_ssl_set_hostname after the client hello as the hostname is needed during the common name verification when connecting to an IP address.

This PR needs testing to be sure there isn't any regression introduced and that there isn't a better way to achieve this goal

Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
@sepalani
Copy link
Contributor Author

@lioncash Done.

Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
@sepalani
Copy link
Contributor Author

sepalani commented Jan 5, 2021

@leoetlino
Done. I chose SSLSendWithoutSNI for the function name.

Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/SSL.cpp Outdated Show resolved Hide resolved
@leoetlino
Copy link
Member

Is it guaranteed that mbedtls will honour the hostname setting and still verify the common name if the hostname is set along the way?

@sepalani
Copy link
Contributor Author

sepalani commented Jan 6, 2021

@leoetlino Done, it's way cleaner that way, indeed.

I don't think this is guaranteed since it's a dirty workaround. As long as the used library code is similar to the one in Externals it should be safe because :

  • mbedtls_ssl_set_hostname only set the hostname in the SSL context
  • the hostname doesn't seem to be inspected/copied/used prior to this (except for SNI)

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks more acceptable now I guess, and the workaround is clearly documented.

@leoetlino
Copy link
Member

@Leseratte10 Could you verify if this PR fixes your issue?

@Leseratte10
Copy link
Contributor

I can confirm that using the version in this PR gets rid of the server_name extensions as planned, yeah. I did not verify whether mbedtls still correctly validates the Common Name with these changes. If that's needed as well I would need to make a certificate with a wrong name and see what happens.

When I compiled the changes from sepalani's 'sni' branch, Dolphin for some reason displayed / used a version number of 4.0-22756 instead of a 5.0-xxxxx version, that doesn't seem like it's related to the changes in this PR - no idea why that happened.

@JosJuice
Copy link
Member

Seems like the 5.0 tag hasn't been pushed to sepalani's repo. It should have no ill effects other than the version number being wrong.

Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed this but the UI code needs to be updated as it currently assumes ctx.p_bio is a mbedtls_next_context. Which is true on master, but this PR breaks the assumption since p_bio is a pointer to a WII_SSL now.

There should probably be a function that takes a WII_SSL and returns the associated mbedtls_net_context.

@sepalani
Copy link
Contributor Author

@leoetlino I opted for a simpler approach. I don't need to grab p_bio from mbedtls_net_context since I already have access to the WII_SSL context and that's what is passed to p_bio.

@leoetlino leoetlino merged commit 3ce72d4 into dolphin-emu:master Feb 11, 2021
9 checks passed
@sepalani sepalani deleted the sni branch February 12, 2021 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants