Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Correct EVP_PKEY ownership #535
Correct EVP_PKEY ownership #535
Changes from all commits
cc6b159
9a0c6b1
6b2a901
92fb4d9
f68ad36
878191a
74271f9
64a8349
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok, but for future better to set err type SOTER_INVALID_PARAMETER in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should return SOTER_FAIL in this case. EC_KEY_new_by_curve_name() could fail if the nid argument is invalid, but we’re checking for that here (
nid == 0
):themis/src/soter/boringssl/soter_asym_ka.c
Lines 41 to 45 in 74271f9
That’s why the most likely reason for failure is some internal error in cryptographic backend. SOTER_INVALID_PARAMETER is used to indicate to the user that they broke some contract for expected function parameters and this error can be avoided by passing correct parameter values. It's used for ‘obvious’ errors like using NULL values where they are not expected, while SOTER_FAIL usually indicates deeper issues with arguments, or acts like a generic catch-all error code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this plase we get dereferencing a pointer. It would be good to check ctx before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not in SOTER_PRIVATE_API and i think that it would be good make that check. Same in soter_sign_final_ecdsa_none_pkcs8 (in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions that are not marked SOTER_API are considered to be private to Soter and they can make assumptions about arguments. Usually we check arguments for NULL in public functions and let the private ones assume that the arguments are more or less sane.
In this case soter_sign_init_ecdsa_none_pkcs8() is called by soter_sign_init() which does check that ctx is non-NULL beforehand. It’s also checked a couple of times more up the call stack, so we can be sure that it won’t be NULL in soter_sign_init_ecdsa_none_pkcs8().
SOTER_PRIVATE_API is kind of a hack that intentionally exposes private API for Themis. It’s not really pretty, but some code currently calls such functions and makes necessary precautions to ensure that they are safe to call. At some point we intend to get rid of SOTER_PRIVATE_API to avoid confusion over what’s public and what’s private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your explanation. Now I have knowledge how this visibility works. This kind of bug is not connected with PR. But you have such functions in SOTER_API. Ex. is soter_rsa_key_pair_gen_init(). This is notice to be cerfull with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mb it will be good to make some rule how to work with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, with soter_rsa_key_pair_gen_init() is bad example (becouse it should be not in SOTER_API). But if we look at typtcal usage any public funtion it would be checked twice ore even more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, C does not really provide any amenities for null checks. Theoretically, we could use
__attribute__((nonnull))
or something like that, but that’s not really portable and too much of a bother. Generally, the rules are that public functions (marked with SOTER_API or THEMIS_API) have to doparanoidthorough parameter checking while internal functions may expect the parameters to have reasonable values. And generally, it does not really hurt if internal functions have duplicated checks. Better safe than segfault.