OpenSSL 1.1.0 support and RSA_set0_key() double frees?

[Jakub Jelen]

On 06/14/2017 03:50 PM, Sebastian Krahmer wrote:
> Hi
> 
> Our openssh maintainer pointed me to these patches:
> 
> http://lists.mindrot.org/pipermail/openssh-unix-dev/2016-September/035378.html
> http://lists.mindrot.org/pipermail/openssh-unix-dev/2016-November/035454.html
> 
> aiming to solve the openssl 1.1. API-change problems.
> 
> In particular
> 
> https://pkgs.fedoraproject.org/cgit/rpms/openssh.git/tree/openssh-7.3p1-openssl-1.1.0.patch
> 
> which seems to be used by fedora, contains code like this:
> 
> 
>   +         (r = sshbuf_get_u32(ids, &bits)) != 0 ||
>   +         (r = sshbuf_get_bignum1(ids, e)) != 0 ||
>   +         (r = sshbuf_get_bignum1(ids, n)) != 0 ||
>   +         (RSA_set0_key(key->rsa, n, e, NULL) == 0) ||
>   +         (r = sshbuf_get_cstring(ids, &comment, NULL)) != 0) {
>   +             BN_free(n);
>   +             BN_free(e);
>                 goto out;
>   ...
>   out:
>           sshkey_free(key);
> 
> Note, that the manpage says about RSA_set0_key():
> 
> "Calling this function
> transfers the memory management of the values to the RSA object, and
> therefore the values that have been passed in should not be freed by
> the caller after this function has been called."
> 
> So, unless theres some tricky ref-counting inside BN_free() and
> RSA_free(), that gets called by sshkey_free(), this is a double-free
> condition. Since RSA_set0_key() may succeed and sshbuf_get_cstring()
> may fail inside the if.
> 
> There are various places like this, so the get0/set0 pattern that is
> used has to be reviewed.
> I am not sure whether the manpage also forbids calling BN_free()
> in case of RSA_set0_key() errors.
> 
> Please take me in Cc, I am not subscribed.
> 
> Sebastian
> 

Hello Sebastian,
thank you for a message. You are right that this could cause some 
problems. Reading through the code, most of the occurrences are in the 
SSH1 code which is already gone from upstream.

I tried to fix in Fedora repository. [1] Let me know if it is as you 
would expected or you found some other issues. If so, patches are welcomed.

[1] https://src.fedoraproject.org/cgit/rpms/openssh.git/commit/?id=f07a0866

Regards,

-- 
Jakub Jelen
Software Engineer
Security Technologies
Red Hat