Correct EVP_PKEY ownership

[ilammy]

Memory management is hard, especially in C. Recent code reviews by @outspace have uncovered a bunch of issues with OpenSSL usage, in particular with EVP_PKEY and EVP_PKEY_CTX objects. Here we attempt to fix them. This PR should resolve issues #532, #533, #534.

I have manually reviewed EVP_PKEY and EVP_PKEY_CTX usage in our code and it seems to be okay now. @ignatk, could you please vet these changes?

The key point in most of the cases is that EVP_PKEY_CTX_new() does not transfer ownership over EVP_PKEY – it increments the refcount on it and we still have to free the original reference to avoid memory leaks. Sometimes we did an additional free in the destructor and this balanced all out, but it’s hardly an elegant solution.

---

We've got a bunch of functions which are currently doing tricky things with EVP_PKEY and EVP_PKEY_CTX objects, sometimes incorrectly. Let’s make sure that the code is readable, that we don't leak objects in the failure path, and that we don't free objects twice.

The general rules we follow are:

1. The function allocating the object is the one that frees it.

2. Unless the ownership is transferred to some other object, in which case we must not free the transferred object: the new owner is responsible for that. This includes constructors.

3. Set the field to NULL after freeing the object stored there. (It's okay to leave local variables as is.)

They all seem obvious in the hindsight, but the current code contains a number of violations in these rules. Partially, due to magic macros which make it seem like the error handling and resource management are correct, while in fact they are not. Therefore we get rid of the macros and replace them with plain old conditions.

We use "goto" pattern in order to make destruction of objects more structured. Yeah, "goto" and "structured" in the same sentence, but this is one of the few cases where using goto makes sense in C. (We'd use proper destructors in modern language, but we aren't.)

Since we use goto, this may have weird effects on variable scoping and initialization. Move all variable declarations to the top of the function and make sure that all local variables are initialized.

Some of these changes make Soter return more accurate error codes: for example, SOTER_INVALID_PARAMETER instead of SOTER_FAIL when the issue is actually in the parameter. Update the test suite to expect the new error codes. Strictly speaking, this does break backwards compatibility (or at least, may break someone's weird use case), but I feel that it's acceptable to make this change. Usually, applications don't check concrete error values, and if they do they expect to catch parameter errors that way and we should return appropriate error codes.

[Lagovas]

looks nicer

[vixentael]

@outspace, your comments are welcome :)

[outspace]

If we talk just about PR it looks ok, exempted one place in soter_asym_ka_init (boringssl) where it can be rewrited simpler. I decided to describe all small bugs that i noticed during rewiew.

[outspace]

It's ok, but for future better to set err type SOTER_INVALID_PARAMETER in this case.

[ilammy]

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

	int nid = soter_alg_to_curve_nid(alg);
	if ((!asym_ka_ctx) || (0 == nid)) {
	return SOTER_INVALID_PARAMETER;
	}

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.

[outspace]

Okay. I agree

[outspace]

I don't know about your coding style guide, but it would be better to swap this parameters for better reading.

[outspace]

I think that it can be rewrited with less code using EVP_PKEY_assign_EC_KEY(). I don't see any reason to use EVP_PKEY_set1_EC_KEY() in this plase.

[ilammy]

Regarding the Yoda-style comparisons, they are not prescribed by the coding style and we don’t really like them that much (exactly for readability reasons). But it’s tedious to walk through all instances of such legacy code at once, so we incrementally modernize it during refactoring. Thank you for noting 👍 This code style, much readable it is not.

[ilammy]

You’re absolutely right about EVP_PKEY_assign_EC_KEY(), It avoid unnecessary reference counting. I’ll update this part. Thank you.

[Lagovas]

As I remember, this type of comparisons was used to avoid errors in C code like if (a = someFunc()). Using constant values (rvalue) as left operand of comparison you will see error on compilation because can't assign value if miss one "=" sign. It's less readable but saves from such errors

[ilammy]

Another merit of this style is that it actually makes comparisons with long function calls more readable if they are made inline. Compare

if (EVP_PKEY_decrypt(asym_cipher->pkey_ctx, NULL, &output_length, (const unsigned char*)cipher_data, cipher_data_length) != -1) {
	// ...
}

with

if (-1 != EVP_PKEY_decrypt(asym_cipher->pkey_ctx, NULL, &output_length, (const unsigned char*)cipher_data, cipher_data_length)) {
	// ...
}

where it's really easy to miss whether it’s == or != and if it’s -1, or 0, or whatever.

Though, it’s less of an issue with our current code style that places a reasonable limit on the line length:

if (EVP_PKEY_decrypt(asym_cipher->pkey_ctx,
                     NULL,
                     &output_length,
                     (const unsigned char*)cipher_data,
                     cipher_data_length) != -1) {
    // ...
}

or even better:

err = EVP_PKEY_decrypt(asym_cipher->pkey_ctx,
                       NULL,
                       &output_length,
                       (const unsigned char*)cipher_data,
                       cipher_data_length);
if (err != -1) {
    // ...
}

As for accidentally using = instead of ==, I personally favor the -Wparentheses compiler warnings over an unnatural code style. So I’m all in for making the dog wag the tail, just without having a dedicated 8000-line-change that does only that.

[Lagovas]

As for accidentally using = instead of ==, I personally favor the -Wparentheses compiler warnings over an unnatural code style.

how this flag will save from assigning in "if" statements which allowed by language syntax?

[ilammy]

@Lagovas, a typical mistake looks like this:

pkey = EVP_PKEY_CTX_get0_pkey(pkey_ctx);
if (pkey = NULL) {
    return SOTER_INVALID_PARAMETER;
}

in which case the compiler will give a friendly warning:

src/soter/boringssl/soter_ecdsa_common.c:33:14: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
    if (pkey = NULL) {
        ~~~~~^~~~~~
src/soter/boringssl/soter_ecdsa_common.c:33:14: note: place parentheses around the assignment to silence this warning
    if (pkey = NULL) {
             ^
        (          )
src/soter/boringssl/soter_ecdsa_common.c:33:14: note: use '==' to turn this assignment into an equality comparison
    if (pkey = NULL) {
             ^
             ==
1 warning generated.

This has to be addressed, given that we treat warnings as errors in pull requests.

Obviously, if you do intent to make an assignment in conditional then you may explicitly code it like this (note the double parentheses):

if ((pkey = EVP_PKEY_CTX_get0_pkey(pkey_ctx))) {
    // pkey != NULL here
}

(It works for boolean function arguments as well.)

[outspace]

In this plase we get dereferencing a pointer. It would be good to check ctx before.

[outspace]

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)

[ilammy]

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.

[outspace]

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.

[outspace]

Mb it will be good to make some rule how to work with this.

[outspace]

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.

[ilammy]

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 do paranoid thorough 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.