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

Connections not reused when cipher list specified #8160

Closed
sfc-gh-emusser opened this issue Dec 17, 2021 · 1 comment
Closed

Connections not reused when cipher list specified #8160

sfc-gh-emusser opened this issue Dec 17, 2021 · 1 comment

Comments

@sfc-gh-emusser
Copy link

sfc-gh-emusser commented Dec 17, 2021

I did this

Since 7.52.0, with libcurl built with nss, if a cipher list is specified with multiple entries on subsequent requests such as:
curl_easy_setopt(curl, CURLOPT_SSL_CIPHER_LIST, "ecdhe_ecdsa_aes_128_gcm_sha_256,ecdhe_rsa_aes_128_gcm_sha_256,dhe_rsa_aes_128_gcm_sha_256,rsa_aes_128_gcm_sha_25");

we see a new https connection created on each request as opposed to reusing the existing connection. This broke with commit
cb4e2be proxy: Support HTTPS proxy and SOCKS+HTTP(s)

The issue is that the method set_ciphers (in lib/vtls/nss.c) clobbers the cipher_list entry kept on each connection by replacing any commas with null characters. After this method is called the cipher_list on a connection will now only contain the first entry (in the above example: ecdhe_ecdsa_aes_128_gcm_sha_256) and it is not necessarily the cipher negotiated for the connection.

The above is an issue in ConnectionExists and Curl_ssl_config_matches as it expects the cipher_list of a new request to exactly match the cipher_list found on an existing connection in order for that connection to be reused.

A relatively simple fix is to call strdup on the cipher_list before passing to set_ciphers (I can push this for review if desired). But looking at the original commit that broke this it appears that we may be inadvertently sharing cipher_list pointers -- perhaps further investigation is needed.

I expected the following

I expected a subsequent request with the same cipher_list (and other properties) to reuse the existing connection.

curl/libcurl version

libcurl 7.80.0

operating system

Linux xx.xx.xx.xx 4.19.84-33.70.amzn2.x86_64 #1 SMP Sun Nov 17 03:35:39 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

sfc-gh-emusser added a commit to sfc-gh-emusser/curl that referenced this issue Dec 17, 2021
This commit makes a clone of the connection's ssl config
cipher_list before passing it to nss.c's set_ciphers method
as that method clobbers its input by changing commas to
nulls.

Without this commit if a multiple entry cipher list is specified
than reuse of a connection in the connection cache is not possible.

This addresses curl#8160.
bagder added a commit that referenced this issue Dec 17, 2021
The string is set by the user and needs to remain intact for proper
connection reuse etc.

Reported-by: Eric Musser
Fixes #8160
@bagder
Copy link
Member

bagder commented Dec 17, 2021

I prefer to avoid extra memory management. See #8161

@sfc-gh-emusser sfc-gh-emusser changed the title Connections cannot be reused when cipher list specified Connections not reused when cipher list specified Dec 18, 2021
@bagder bagder closed this as completed in 556a42e Dec 20, 2021
sfc-gh-emusser pushed a commit to sfc-gh-emusser/curl that referenced this issue Dec 22, 2021
The string is set by the user and needs to remain intact for proper
connection reuse etc.

Reported-by: Eric Musser
Fixes curl#8160
Closes curl#8161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants