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

Fix the misusage of short-circuit evaluation in 'soter_sign_ecdsa.c' #314

Merged
merged 1 commit into from May 10, 2018

Conversation

Projects
None yet
3 participants
@movie-travel-code
Copy link

commented May 10, 2018

I'm henry wong, from Qihoo360 CodeSafe Team. We found a possible null
pointer deference caused by the misusage of short-circuit evaluation. If
pkey is null, the program will continue execute EVP_PKEY_base_id(pkey),
and this will cause a null pointer dereference.

Since I'm unfamiliar with themis, please forgive me if there is anything
wrong with my description.

Henry Wong
Qihoo360 CodeSafe Team

wangliushuai
Fix the misusage of short-circuit evaluation in 'soter_sign_ecdsa.c'
I'm henry wong, from Qihoo360 CodeSafe Team. We found a possible null
pointer deference caused by the misusage of short-circuit evaluation. If
'pkey' is null, the program will continue execute 'EVP_PKEY_base_id(pkey)',
and this will cause a null pointer dereference.

@vixentael vixentael requested review from vixentael, Lagovas and secumod May 10, 2018

@@ -102,7 +102,7 @@ soter_status_t soter_sign_update_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx, const v
soter_status_t soter_sign_final_ecdsa_none_pkcs8(soter_sign_ctx_t* ctx, void* signature, size_t *signature_length)
{
EVP_PKEY *pkey = EVP_PKEY_CTX_get0_pkey(ctx->pkey_ctx);
if (!pkey && EVP_PKEY_base_id(pkey)!=EVP_PKEY_EC){
if (!pkey || EVP_PKEY_base_id(pkey)!=EVP_PKEY_EC){

This comment has been minimized.

Copy link
@secumod

secumod May 10, 2018

Contributor

You are correct - thank you. I would also try to improve it a bit by not relying on potential compiler "optimisations" in evaluating this statement and split it into two consecutive ones, like

if (!pkey) {
  return SOTER_INVALID_PARAMETER;
}
if (EVP_PKEY_base_id(pkey)!=EVP_PKEY_EC) {
  return SOTER_INVALID_PARAMETER;
}

this makes it more explicit and readable

@vixentael

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Thank you! Very good catch.

We found similar line in boringssl wrapper
https://github.com/cossacklabs/themis/blob/master/src/soter/boringssl/soter_sign_ecdsa.c#L93

We plan to fix both places and to add some tests.

Please, change target branch to
https://github.com/cossacklabs/themis/tree/soter_pk_fixes

We will merge your changes into soter_pk_fixes, and add few new checks.

@movie-travel-code movie-travel-code changed the base branch from master to soter_pk_fixes May 10, 2018

vixentael added a commit that referenced this pull request May 10, 2018

@vixentael vixentael merged commit 7e4d45d into cossacklabs:soter_pk_fixes May 10, 2018

7 checks passed

ci/bitrise/b32b4ea8bffedad7/pr Passed - themis
Details
ci/circleci: android Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: php5 Your tests passed on CircleCI!
Details
ci/circleci: php70 Your tests passed on CircleCI!
Details
ci/circleci: php71 Your tests passed on CircleCI!
Details
ci/circleci: x86_64 Your tests passed on CircleCI!
Details

vixentael added a commit that referenced this pull request May 11, 2018

Refactoring if (!pkey) in 'soter_sign_ecdsa.c' for OpenSSL/BoringSSL (#…
…315)

* Fix the misusage of short-circuit evaluation in 'soter_sign_ecdsa.c'

I'm henry wong, from Qihoo360 CodeSafe Team. We found a possible null
pointer deference caused by the misusage of short-circuit evaluation. If
'pkey' is null, the program will continue execute 'EVP_PKEY_base_id(pkey)',
and this will cause a null pointer dereference.

* soter_sign fixes, from PR #314

* test that leads to sigfault

* remove test because we find no way to NULL `pkey` in `EVP_PKEY_CTX`

* fix copy-pasted line

* fixes according to @secumod comment

* ..and remove one more space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.