This needs to be rewritten to something like the following, otherwise it will segfault for non-RSA keys (ECC on yubikey): ``` if (rsa && RSA_get0_key(rsa, &n, &e, NULL) && n && e && ``` I just noticed it with the version I run on Fedora.

RSA_bits? (Also DSA_bits below, but you'll need to add a new compat wrapper.)

Error-handling?

Check that `rsa_n` and `rsa_e` are NULL.

`pkcs11_rsa_finish` doesn't free this.

Nit: These blocks seem over-indented (which makes the diff a little more annoying), or is that the preferred style here? Ditto with the other `case FOO: { ... }` blocks.

Nit: Calling `EVP_MD_CTX_init` seems prudent? (Though it does just memset to zero.) Alternatively, this could just call `EVP_MD_CTX_create` and `EVP_MD_CTX_destroy` which exist in 1.0.x.

(unused)

(unused)

(Personally I would have just made these header-only, but whatever.)

This now leaks `ret->mdctx` if `EVP_DigestInit_ex` hits a malloc failure. You want `EVP_MD_CTX_free(ret->mdctx); free(ret);` or `ssh_digest_free(ret)`.

Asking `EVP_CipherInit` to copy `keylen` bytes worth of `key` before telling it what `keylen` is (`EVP_CIPHER_CTX_set_key_length`) seems kind of questionable, notably for ciphers with variable-length keys. Are you sure this works? It was probably previously in two steps so it'd all happen in the right order.

Nit: I believe you can just omit lines 294 and 295 (after fixing the NULL-initializing above). `BN_hex2bn` just allocates things for you.

This should be NULL-initialized otherwise some of the `goto err` paths below will get confused on malloc error.

This is certainly wrong. In this case, you should not allocate&put&set. It should be get&put. The same for below RSA case.

This is intentionally left out or like todo for future readers?