Asymmetric RSA key code

[wintersteiger]

The code to initialize an oe_public_key_t in oe_public_key_init has different behavior on the host and in the enclave. The mbedTLS code sometimes copies keys: https://github.com/openenclave/openenclave/blob/5eb82c855a3874817578b17a5e46671c1a77d077/enclave/crypto/key.c#L77 while the OpenSSL code never does so: https://github.com/openenclave/openenclave/blob/5eb82c855a3874817578b17a5e46671c1a77d077/host/crypto/openssl/key.c#L30 For me, the result was a double free that took me quite a while to trace. It would have helped me if the oe_public_key_free functions had set the corresponding key fields (public_key/impl->pkey) to NULL, because my code would have failed much earlier.

Relates to #1003, #3007.

[mikbras]

Thank you Christoph for filing the issue.

I was wondering whether you're calling oe_public_key_init() directly or indirectly. If indirectly, which of these functions are you calling?

oe_rsa_public_key_init()
oe_cert_get_rsa_public_key()
oe_rsa_get_public_key_from_private()

Hopefully this information will help me pinpoint the bug.

Thanks!

[wintersteiger]

Hi Mike, thanks for taking a look at this!

I use oe_rsa_public_key_init and oe_rsa_public_key_free (here). The mbedTLS version of oe_rsa_public_key_init creates an internal copy, while the OpenSSL version doesn't, so I need an additional mbedtls_pk_free. I may well be using the API in an unusual way, so let me know if there's a better/cleaner way to do this.

[mikbras]

Hi Mike, thanks for taking a look at this!

I use oe_rsa_public_key_init and oe_rsa_public_key_free (here). The mbedTLS version of oe_rsa_public_key_init creates an internal copy, while the OpenSSL version doesn't, so I need an additional mbedtls_pk_free. I may well be using the API in an unusual way, so let me know if there's a better/cleaner way to do this.

Oh, I see. I guess oe_rsa_public_key_init is an internal function since it is not exposed under the include directory. There seem to be three solutions.

1. Pass the key to oe_ras_public_key_init and never free it (the function takes over the key and decrements the reference count eventually).
2. Up-ref the key (EVP_PKEY_up_ref) before calling oe_ras_public_key_init and then free it later (EVP_PKEY_free).
3. Convert the key to PEM format, and then call the public (well more public) function called oe_rsa_private_key_read_pem.

If you tried #1 or #2 and they don't work, then OE might have a bug and I'll look into it.

The APIs are dissimilar because mbedtls uses copying whereas openssl uses reference counting. Maybe the openssl RSA functions should have increased the reference count (but then the caller would have to free the key). Since the init functions are internal, I guess little attention was paid to the diferences.

[wintersteiger]

I see, makes sense now! It's been a while, but if I remember correctly, not freeing the mbedTLS key reports a memory leak, that why I dug deeper. You're right, maybe I should just up-ref the OpenSSL key to make it symmetric again. I would expect oe_rsa_public_key_init to do that for me though.

[mikbras]

I would expect oe_rsa_public_key_init to do that for me though.

I agree and if this ever becomes a public API, I would suggest that the function should make a deep copy of the key since changing the key after the call affects the behavior in one case (OpenSSL) but not the other (mbedtls).

[manojrupireddy]

I have few questions here.

• Deep copy on mbedtls side is a conditional one. Should similar conditions should be used on OpenSSL part? If deep copy is enabled in both cases, who should own freeing up the keys? Should it be the caller of oe_rsa_public_key_init? (In this case eeid.c). And if it is so will deep copy approach on OpenSSL side cause any regressions?
• I have an other idea. Not sure if it is correct one. Should freeing up the key should be owned by copy_key function on mbedtls side. As copy_key is the function allocating the additional memory, it should own up freeing the resource. Is there any requirement that there need to be two copies of keys? Can't we free one key after doing deep copy on mbedtls side?

@CodeMonkeyLeet - Adding Simon for context.

[CodeMonkeyLeet]

@manojrupireddy

• I think you're getting confused by the fact that the mbedTLS and OpenSSL implementations are structurally asymmetric; the conditional deep copy on the mbedtls you're referring to exists in the internal implementation function oe_public_key_init(), not oe_rsa_public_key_init. The internal function has the conditional you're referring to, to handle its two use cases:

  • in the oe_rsa_public_key_init codepath, where it clones a key. For all intents and purposes, Mike's analysis is accurate as it relates to this scenario, there is no conditional handling here.
  • as a helper function to zero initialize the oe_public_key_t struct before populating it with key data as part of oe_public_key_read_pem.
    • arguably, these two usages should be separated out; oe_public_key_init sets the magic number before the key is actually valid, which at other points in the code is treated as the arbiter of key validity. Given that you were confused by the implementation here, it's also probably good reason to clean up the internal implementation as a maintainability fix.

• No, in general that breaks existing lifetime management semantics because you would destroy an existing reference from the caller, and it is also not consistent with the OpenSSL semantics.

  • I agree with @wintersteiger: for an internal implementation, it should at least be consistent in where init/frees are needed for /common code scenarios. This would require updating existing internal implementations which are internally consistent today, but remains a problem spot for future maintainers to understand. It's worth looking at the incremental cost of doing this to address his issue.
  • @mikbras is also correct in that for the semantics between the two implementations, they really need to be either both refcounted or both cloned as they do represent different lifetime management semantics. I would actually prefer making mbedtls refcounted, which makes it consistent with both the OpenSSL and Windows BCrypt implementations (Bcrypt uses key handles), but that introduces a different set of overhead on the enclave side for thread safety on refcounting. I would suggest tabling this and filing a different issue to track this irregularity in the internal interface.

[manojrupireddy]

From the above discussion comments, action items were summarized below.

• Clean up the internal implementation of oe_public_key_init of mbedtls code part.

• Bring consistency between OpenSSL and mbedtls by using EVP_KEY_up_ref on OpenSSL functions.

• Created new issue to track the irregularity in the life management semantics. OpenSSL uses refcount and mbedtls uses deep copy. It should be consistent.# 3296