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

Correct EVP_PKEY ownership #535

Merged
merged 8 commits into from
Sep 23, 2019
Merged

Correct EVP_PKEY ownership #535

merged 8 commits into from
Sep 23, 2019

Commits on Sep 20, 2019

  1. Correct EVP_PKEY ownership: soter_asym_cipher

    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 then we should return
    appropriate error codes.
    ilammy committed Sep 20, 2019
    Configuration menu
    Copy the full SHA
    cc6b159 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    9a0c6b1 View commit details
    Browse the repository at this point in the history
  3. Correct EVP_PKEY ownership: soter_rsa_key_pair_gen

    OpenSSL and BoringSSL implementations are slightly different. Currently
    we don't have time to dive deep into the reasons behind this so we just
    fix the ownership issues here and move on.
    
    In this case it makes sense to introduce a couple of utility functions
    to hide some complexity of dealing with BIGNUMs.
    ilammy committed Sep 20, 2019
    Configuration menu
    Copy the full SHA
    6b2a901 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    92fb4d9 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    f68ad36 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    878191a View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    74271f9 View commit details
    Browse the repository at this point in the history

Commits on Sep 23, 2019

  1. Use EVP_PKEY_assign_EC_KEY

    We don't really need to retain the EC_KEY instances in these
    cases so instead of calling EVP_PKEY_set1_EC_KEY() we can call
    EVP_PKEY_assign_EC_KEY() which avoids unnecessary refcounting
    in the happy path.
    
    Investigating uses of EVP_PKEY_set1_EC_KEY() has uncovered a possible
    memory leak in soter_ec_gen_key() when an error occurs.
    ilammy committed Sep 23, 2019
    Configuration menu
    Copy the full SHA
    64a8349 View commit details
    Browse the repository at this point in the history