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

sectransp: Remove large cipher table #13823

Closed
wants to merge 1 commit into from

Conversation

jan2000
Copy link
Contributor

@jan2000 jan2000 commented May 29, 2024

Previously a large table of ciphers was used to determine the default ciphers and to lookup manually selected ciphers names.

With the lookup of the manually selected cipher names moved to Curl_cipher_suite_walk_str() the large table is no longer needed for that purpose.

The list of manually selected cipher can now be intersected with the ciphers supported by Secure Transport (SSLGetSupportedCiphers()), instead of using the fixed table for that.

The other use of the table was to filter the list of all supported ciphers offered by Secure Transport to create a list of ciphers to use by default, excluding ciphers in the table marked as weak.

Instead of using a complement based approach (exclude weak), switch to using an intersection with a smaller list of ciphers deemed appropriate.

Previously a large table of ciphers was used to determine the default
ciphers and to lookup manually selected ciphers names.

With the lookup of the manually selected cipher names moved to
Curl_cipher_suite_walk_str() the large table is no longer needed for
that purpose.

The list of manually selected cipher can now be intersected with the
ciphers supported by Secure Transport (SSLGetSupportedCiphers()),
instead of using the fixed table for that.

The other use of the table was to filter the list of all supported
ciphers offered by Secure Transport to create a list of ciphers to
use by default, excluding ciphers in the table marked as weak.

Instead of using a complement based approach (exclude weak), switch
to using an intersection with a smaller list of ciphers deemed
appropriate.
@github-actions github-actions bot added TLS appleOS specific to an Apple operating system labels May 29, 2024
CIPHER_STRONG_ENOUGH),
#endif /* CURL_BUILD_MAC_10_13 || CURL_BUILD_IOS_11 */
static const uint16_t default_ciphers[] = {
TLS_RSA_WITH_3DES_EDE_CBC_SHA, /* 0x000A */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I am not to sure about the inclusion of TLS_RSA_WITH_3DES_EDE_CBC_SHA here. This is a change from previous method.

It is included here (based on Mozilla's cipher list https://wiki.mozilla.org/Security/Server_Side_TLS#Old_backward_compatibility) because it is the one cipher that provides way way back compatibility. It is at the bottom of the list of the ciphers offered by Secure Transport, so will be used as last choice (if the client's cipher order is used).

This cipher is known to be weak when using it for multi GB transfers (sweet32). Some other SSL implementations have dropped support for it. However, it is the only cipher left from the original TLSv1.0 specification (rfc2246).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow what the change is. Was this not supported before this PR, but it is now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR the cipher TLS_RSA_WITH_3DES_EDE_CBC_SHA was marked as weak. The default ciphers used were based on the list of ciphers returned by SSLGetSupportedCiphers() excluding the ciphers marked in the code as weak. So TLS_RSA_WITH_3DES_EDE_CBC_SHA would not be on the default cipher list.

With this PR the list of default ciphers will be based on an intersection with a smaller list of ciphers (deafult_ciphers[]). This results in a simular, but more restricted set than previous. With TLS_RSA_WITH_3DES_EDE_CBC_SHA being the only exception, in that is was excluded before and is included now. So it may be on the default cipher list if offered by sectranp.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I'll proceed and merge this now and keep an eye out if any user reports something...

@bagder bagder closed this in 4e2c451 Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appleOS specific to an Apple operating system TLS
Development

Successfully merging this pull request may close these issues.

None yet

2 participants