Blanket impl of HasPublic for all T: HasPrivate is not a valid assumption for engine-backed EC keys

[arsing]

For engine-backed keys, the private and public keys may be stored separately. Using ENGINE_load_private_key (returns a *mut EVP_PKEY) + foreign_types::ForeignType::from_ptr I can get an openssl::pkey::PKey<openssl::pkey::Private>. The type system then allows me to write:

let point = key.ec_key().unwrap().public_key();
point.to_bytes(...);

... except that point's inner *mut ffi::EC_POINT is NULL and this segfaults. (It also means you end up with a & that is NULL in safe Rust code.)

I was initially going to report this as a missing NULL-check in EcKeyRef<Public>::public_key for the return value of EC_KEY_get0_public_key, but then I realized the real problem is the assumption that all EcKey<Private>s are also EcKey<Public>s due to the blanket impl of HasPublic for all T: HasPrivate. Such an impl assumes that all private keys have access to public key information, which is not valid for engine-backed EC keys. Only the engine-backed public key's EVP_PKEY (which can be obtained with ENGINE_load_public_key) is guaranteed to have these parameters.

RSA keys don't have this problem (engine-backed or otherwise) because the public key parameters (modulus and exponent) are also available from the private key.

The specific engine that I discovered this issue was libp11, and the relevant code that builds the EC_KEY structure for the EVP_PKEY is here. For private keys, the params (group info) are found, but the point is not.

I want to reiterate that this is not a problem for all engine-backed EC keys, only for engines that store the private and public keys separately. For libp11, it's more complicated by the fact that its behavior depends on the underlying PKCS#11 library.

---

So one way to resolve this is to remove the blanket impl, and copy the methods of Rsa<Public> to Rsa<Private> so that that continues to work. But then if the user has an EcKey<Private> managed by openssl (ie it does have public key parameters), then they would still need a way to call public_key on this. I suppose you could add:

impl EcKey<Private> {
    fn as_public(&self) -> Option<&EcKey<Public>> {
        // If EC_KEY_get0_public_key returns NULL, then return None, otherwise return Some(self)
    }
}

But this is kinda ugly.

Alternatively, changing EcKey<Public>::public_key to check EC_KEY_get0_public_key's result for NULL and return Option<&EcPointRef> would solve the safety problem (no more & that are actually NULL), at the risk of looking silly ("the public key may not have a public key???").

[sfackler]

We could add a new HasPrivateOnly trait with a blanket impl for T: HasPrivate and change all of the private key methods to be based off of that trait.

[arsing]

Yes, I guess that would be the best approach. Then all EcKey<Private> managed by openssl (such as via EcKey::<Private>::generate) can continue having get_public_key, while the engine API can return EcKey<PrivateOnly> which does not.

But it'll be a breaking change, so I assume it has to wait for 0.11 ?

[sfackler]

I don't believe it's a breaking change.