Severity and mitigation

First of all: don’t panic! This is not a vulnerability in openssh, so it doesn’t affect the ssh that we all use every day. libssh2 is a client-side C library, which enables applications to connect to an SSH server. Secondly, this is not a vulnerability in libssh, which is an unrelated C library which provides similar functionality to libssh2.

The vulnerability exists in libssh2 version 1.8.2 and earlier. It is fixed in libssh2 version 1.9.0. I am not aware of any mitigations against this vulnerability, other than upgrading to version 1.9.0.

The vulnerability is an out-of-bounds read, potentially leading to a remote information disclosure. It is triggered when libssh2 is used to connect to a malicious SSH server. The overflow occurs during Diffie Hellman key exchange, which means that the vulnerability can be triggered early in the connection process, before authentication is completed. libssh2 receives a uint32_t from the malicious server and does no bounds checking on it. libssh2 then reads memory from the offset specified by the uint32_t. I have written a proof-of-concept exploit in which a malicious SSH server returns a very large offset value, which causes libssh2 to crash with a segmentation fault. However, I believe that a more carefully chosen offset could lead to an information disclosure as it appears that the memory which is read is subsequently returned to the server. The exploitability will depend on the heap layout. Since libssh2 is only a library, this will vary depending on the application in which it is used.

Finding the vulnerability

On March 18, 2019, Chris Coulson of Canonical Ltd. disclosed nine vulnerabilities in libssh2 (CVE-2019-3855 to CVE-2019-3863). Those vulnerabilities were fixed in libssh2 v1.8.1. At the time, my colleague Pavel Avgustinov noticed that the commit which fixed the vulnerabilities introduced several new alerts on LGTM. Those alerts were due to code like the following:

if((p_len = _libssh2_get_c_string(&buf, &p)) < 0)

The problem is that _libssh2_get_c_string returns -1 as an error code, but p_len is unsigned, so the error condition will be ignored. It turned out that the libssh2 team had already fixed those issues in a later commit, but it prompted us to take a closer look at the code to see if it contained any other obvious bugs. We quickly discovered this badly implemented bounds check function:

int _libssh2_check_length(struct string_buf *buf, size_t len)
{
    return ((int)(buf->dataptr - buf->data) <= (int)(buf->len - len)) ? 1 : 0;
}

The problem with this function is that the casts to int could overflow. The left-hand cast is safe because the fields of buf are trusted values, but the right-hand cast is not safe because the value of len is untrusted. It wasn’t hard to create a proof-of-concept exploit which bypasses this overflow check by making the value of len greater than buf->len + 0x80000000. The PoC is now available on GitHub.

I learned later (see timeline below) that _libssh2_check_length was introduced on the main development branch after the release of version 1.8.2, so this vulnerable bounds check does not exist in version 1.8.2. Unfortunately, version 1.8.2 contains no bounds check whatsoever, so the PoC still works. In version 1.8.2, the source location of the vulnerability is kex.c:1675. The problem is that p_len contains an untrusted value, so the subsequent reads from s could be out-of-bounds. Because _libssh2_check_length does not exist in version 1.8.2, there is no need for the value of p_len to be greater than 0x80000000 to trigger the bug. This means that much smaller values of len can trigger an out-of-bounds read, which means that the bug is much more likely to be exploitable to achieve remote information disclosure.

Querying for negative error codes cast to unsigned

When my colleagues and I were looking at the commit which fixed the vulnerabilities found by Chris Coulson, we noticed a common bug pattern where a negative error return value is cast to unsigned. This bug is an ideal candidate for variant analysis, because it’s an easy mistake to make and (somewhat to our surprise) it isn’t caught by standard compiler warnings. So I wrote this simple CodeQL query to find every instance of the problem:

---
---
import cpp
import semmle.code.cpp.dataflow.DataFlow
import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis

from Function f, FunctionCall call, ReturnStmt ret, DataFlow::Node source, DataFlow::Node sink
where call.getTarget() = f
and ret.getEnclosingFunction() = f
and ret.getExpr().getValue().toInt() < 0
and source.asExpr() = call
and DataFlow::localFlow(source, sink)
and sink.asExpr().getFullyConverted().getType().getUnderlyingType().(IntegralType).isUnsigned()
and lowerBound(sink.asExpr()) < 0
select sink

This query looks for code like this:

r_len = _libssh2_get_c_string(&buf, &r);
if(r_len <= 0)
    return -1;

The query looks for functions which sometimes return a negative integer constant. For example, _libssh2_get_c_string does that on line 773. It then looks for calls to that function which take the return value and cast it to an unsigned type.

The query returned 9 results. I included that link in the email that I sent to the libssh2 team on March 28, 2019. They have since fixed all of those bugs.

Timeline

The libssh2 team posted a fix for the vulnerability on GitHub within a few days of receiving my report, but took almost 3 months to release a new version. This was partly due to a misunderstanding. When I sent them the PoC, I told them that the vulnerability was in revision 38bf7ce. It turns out that this revision was not included in the 1.8.2 release. So the libssh2 team fixed the bug on their development branch and considered it closed until I asked them for an update over a month later. The libssh2 team believed that 1.8.2 was not affected by the vulnerability, but I quickly discovered that my PoC also worked on version 1.8.2. They released the fix in version 1.9.0 without warning me, which is why I posted a hasty security advisory on twitter.