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

check the return value of EVP_MD_CTX_create(EVP_MD_CTX_new) #8133

Closed
wants to merge 8 commits into from

Conversation

@x2018
Copy link
Contributor

@x2018 x2018 commented Dec 10, 2021

check the return value of EVP_MD_CTX_create(i.e. EVP_MD_CTX_new) in lib/sha256.c:87.
If some memory allocation errors occur, EVP_ MD_ CTX_ New() will return null, then EVP_DigestInit_ex(), EVP_DigestUpdate() and EVP_DigestFinal_ex() will make wrong memory access. So it is better to check the return value of it.

@bagder
Copy link
Member

@bagder bagder commented Dec 10, 2021

Shouldn't the init function rather return an error to then have Curl_sha256it() also return an error for this case so that curl can bail out all the way up?

@x2018
Copy link
Contributor Author

@x2018 x2018 commented Dec 11, 2021

Yes, it is best for this case. However, I hesitated to do that yesterday as there are other implementations under different macro conditions and I am not familiar with them. So if we suppose to do that, some definitions and statements need to be also updated.

@x2018
Copy link
Contributor Author

@x2018 x2018 commented Dec 11, 2021

Sorry. I made mistakes that commit to this branch. later I will rollback it.

@@ -288,6 +288,10 @@ CURLcode Curl_output_aws_sigv4(struct Curl_easy *data, bool proxy)
post_data_len = (size_t)data->set.postfieldsize;
Curl_sha256it(sha_hash,
(const unsigned char *) post_data, post_data_len);
if(!(char *) sha_hash) {
goto fail;
}
Copy link
Member

@bagder bagder Dec 11, 2021

Choose a reason for hiding this comment

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

please make Curl_sha256it return error/success instead and check that instead of relying on the output data. Yes, that's a bigger change but is much cleaner and nicer code.

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Dec 11, 2021

Thanks! That was an oversight in 94696e1.

I agree with @bagder , the error should be handled by the caller. That would mean changing the signature of HMAC_hinit_func. Which would be good anyway as other implementations and the corresponding MD5 functions can also fail.

However, I hesitated to do that yesterday as there are other implementations under different macro conditions and I am not familiar with them. So if we suppose to do that, some definitions and statements need to be also updated.

The other implementations could just always return CURLE_OK for now. The CI system should tell us if anything was missed.

@x2018
Copy link
Contributor Author

@x2018 x2018 commented Dec 12, 2021

I changed the return type of Curl_sha256it() and Curl_md5it(). To be cautious, I still just add a check for the return value of EVP_MD_CTX_create(), but the other functions in *_init, *_update and *_final may also fail in some way. As I do not familiar with them and I'm not sure what status they should return when they have an error, so I just retain the original shape(or directly return CURLE_OK).

my_sha256_update(&ctx, input, curlx_uztoui(length));
my_sha256_final(output, &ctx);
return ret;
}


Copy link
Member

@MarcelRaad MarcelRaad Dec 13, 2021

Choose a reason for hiding this comment

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

I can't comment on line 527, but the HMAC_hinit_func signature (and the MD5 init function) would have to be updated too to return CURLcode. Unfortunately the CURLX_FUNCTION_CAST meant for the parameter hides that. Looks good to me otherwise!

Copy link
Contributor Author

@x2018 x2018 Dec 13, 2021

Choose a reason for hiding this comment

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

Sorry, I forgot to update it though you reminded me. I will do it after work...

Copy link
Member

@MarcelRaad MarcelRaad left a comment

Thanks, looks good to me now! Just waiting if @bagder wants to have another look too.

@bagder
Copy link
Member

@bagder bagder commented Dec 13, 2021

Thanks!

@bagder bagder closed this in d6ff35b Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants