openssh unix dev - Jun 2017 - OpenSSL 1.1.0 support and RSA_set0_key() double frees?

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

-- 

~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer at suse.com - SuSE Security Team

On Wed, 14 Jun 2017, 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.
That does look like a double-free, and those are exploitable under
some circumstances.

The long term solution will be to rework these bits to use the new
OpenSSL API, but I'm loathe to do this for reasons I explained in the
second thread you linked to.

-d

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