-
Notifications
You must be signed in to change notification settings - Fork 871
Parse more of the ClientHello when using SSL3 in the record layer #2821
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
Comments
I'm assuming this is related to dotnet/runtime#113729, dotnet/aspnetcore#60805? |
I suppose we can do more. There really was no reason to do it and testing is going to be pain IMHO. |
Surprisingly no, but it probably would be helpful for those as well. This came from a team who wants to reject connections earlier if the TLS version is too low. |
it it comes as SSL3 frame it it probably already too old :) I never seen that with TLS 1.2+ |
This scenario popped up for a number of clients using IoT devices. I can share with you more data over Teams if you wish. |
I think getting samples of the Hello should be sufficient. We will need to synthesize tests from that. I'm not sure who will work on that. I'm on loan to other assignments for near future. |
I already gave Brennan one - his testing against that led to this issue :) |
We experienced this issue with our Azure Function App, but it has now been resolved on our end. Our IoT devices send an HTTPS GET request to our API to retrieve data before proceeding with the application logic. All our devices have SSL and TLS enabled(This is due to our GSM module and keeping the module flexible), though they currently do not support TLS 1.3. We plan to update some of these devices to only use TLS 1.2 Could you please provide more information on the issue and how we can prevent it in the future? Or would I be right in understanding that it has already been resolved? Also, could you clarify where the fix was applied? I’d like to avoid reintroducing the issue by republishing our Function App. Can this fix be incorporated into my local copy of the Function App or its dependencies? Another concern I have is regarding IoT Hub and Azure Blob Storage, especially if they are being updated to require a minimum of TLS 1.2. We’d like to avoid any service interruptions there as well. I’d be happy to connect over Teams if that’s easier to discuss this further. Looking forward to your response. |
@aardrasystems To reproduce this issue you might use the attached code. It is limited to send a ClientHello and not useful for other purposes. |
@godly We are running our API via Azure Function app consumption plan using C# so I don't think its the yarp version. Our Iot devices all have internal setting that enables SSL, TLS1, TLS1.1 and TLS 1.2. I believe this has been resolved for our resources, but is this change editable by our actual Visual Studio project? |
@aardrasystems I added some comments to the Python code which might help you to understand the issue we experienced. |
@godly OK, so am i correct in understanding our IoT device that have enabled all (SSL,TLS1,1.1 and 1.2), should be OK for the future? Can we get more information on yarp and function app or is this not available? |
Can you two please move this conversation elsewhere? It looks like it has nothing to do with this issue, which is just about surfacing more of the parsed ClientHello if you happen to use the TlsFrameHelper in YARP. |
Describe the bug
Using the
TlsFrameHelper
to parse ClientHello doesn't populate very much information if the record layer version is SSL3.0. Specifically, theTlsFrameInfo.SupportedVersions
isn't populated with the ClientHello version (which is likely TLS1.2 or higher). This means application level logic can't easily do early SSL/TLS version checks to reject requests.This is due to the following check
yarp/src/ReverseProxy/Utilities/TlsFrameHelper.cs
Line 325 in 68f81c4
SSL3.0 is less than TLS1.0 so the rest of the ClientHello parsing is skipped.
From the TLS1.2 Protocol RFC
https://www.rfc-editor.org/rfc/rfc5246#appendix-E
It seems like the check could easily be changed to
>= SslProtocols.Ssl3
. I did a quick test with this and it looks like it correctly parsed the data and showedSsl3 | Tls12
inSupportedVersions
.To Reproduce
Send a TLS request with SSL3 as the version in the record layer and TLS1.2 as the ClientHello version.
Further technical details
N/A
The text was updated successfully, but these errors were encountered: