Skip to content

check the return value of SSL_get_ex_data()#8268

Closed
x2018 wants to merge 3 commits into
curl:masterfrom
x2018:retchk_SSL_get_ex_data
Closed

check the return value of SSL_get_ex_data()#8268
x2018 wants to merge 3 commits into
curl:masterfrom
x2018:retchk_SSL_get_ex_data

Conversation

@x2018

@x2018 x2018 commented Jan 12, 2022

Copy link
Copy Markdown
Contributor

check the return value of SSL_get_ex_data() to prevent potential NULL dereference.
SSL_set_ex_data() called by ossl_associate_connection() may fail, so it is better to check the return value from SSL_get_ex_data(), especially data which will be dereferenced in the following code. And though sockindex seems meaningless in Curl_ssl_getsessionid() or Curl_ssl_addsessionid()?

Comment thread lib/vtls/openssl.c
sockindex_ptr = (curl_socket_t*) SSL_get_ex_data(ssl, sockindex_idx);
if(!conn || !data || !sockindex_ptr)
return 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If no error has been returned before, can this really happen?

And if this can happen, doesn't it signify a major error somewhere to call for everything to get closed down?

@x2018

x2018 commented Jan 14, 2022

Copy link
Copy Markdown
Contributor Author

Yes, it looks like this can happen as

static void ossl_associate_connection(struct Curl_easy *data,
is void type and inside it does not check the return value of SSL_set_ex_data, while SSL_set_ex_data may fail because of some internal errors.
To catch the error in time, I will change the signature of ossl_associate_connection and add several checks these days.

x2018 added 2 commits January 14, 2022 15:40
…ck to catch the internal errors of SSL_set_ex_data or SSL_get_ex_new_index in time
@x2018

x2018 commented Jan 19, 2022

Copy link
Copy Markdown
Contributor Author

I updated the signature of ossl_associate_connection and added several checks. And I retain the usage of Curl_ssl->associate_connection() in vtls.c as before. I am not sure whether these changes make sense. Thank you for taking the time to review this PR.

@bagder bagder closed this in a97eb81 Jan 23, 2022
@bagder

bagder commented Jan 23, 2022

Copy link
Copy Markdown
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants