Security Vulnerability - Action Required: Out-of-bounds Write vulnerability may in your project

[Crispy-fried-chicken]

Hi, we have detected that your project may be vulnerable to Insufficient Information in the function of BF_crypt in the file of crypt_blowfish.c. It shares similarities to a recent CVE disclosure CVE-2020-1916 in the https://github.com/facebook/hhvm. The source vulnerability information is as follows:

Vulnerability Detail: CVE Identifier:CVE-2020-1916 Description: An incorrect size calculation in ldap_escape may lead to an integer overflow when overly long input is passed in, resulting in an out-of-bounds write. This issue affects HHVM prior to 4.56.2, all versions between 4.57.0 and 4.78.0, 4.79.0, 4.80.0, 4.81.0, 4.82.0, 4.83.0. Reference: https://nvd.nist.gov/vuln/detail/CVE-2020-1916 Patch: https://github.com/facebook/hhvm/commit/abe0b29e4d3a610f9bc920b8be4ad8403364c2d4

Would you help to check if this bug is true? If it's true, I'd like to open a PR for that if necessary. Thank you for your effort and patience!

[solardiz]

Hi. As you probably realize, your (mostly automated?) report is confusing. The HHVM commit you link to fixes many unrelated issues against different pieces of their own and upstream code they bundled. This includes changes to code they got from our crypt_blowfish indirectly via upstream PHP, which had also made changes. And then you claim "Out-of-bounds Write" in this issue's title, but also "Insufficient Information" in the issue description.

What helps is your reference to BF_crypt. HHVM is adding the ((unsigned int)(setting[7 + 22 - 1] - 0x20) >= 0x60) check below:

  if (count < min ||
      BF_decode(data.binary.salt, &setting[7], 16) ||
      ((unsigned int)(setting[7 + 22 - 1] - 0x20) >= 0x60)
     ) {
    __set_errno(EINVAL);
    return NULL;
  }

The question is why. Later in the function, we have:

        output[7 + 22 - 1] = BF_itoa64[(int)
                BF_atoi64[(int)setting[7 + 22 - 1] - 0x20] & 0x30];

The purpose of this piece is to reset the unused salt bits, to produce a canonical salt string in the output even if the input had non-zeroes in the unused bits.

We have (int)setting[7 + 22 - 1] - 0x20 in here, which is used to index BF_atoi64 and so could result in an out-of-bounds read. Except that this character is also part of what BF_decode decodes, and we're checking its return value right in this same if above, and it's supposed to indicate error if that character was out of range.

So there should be no issue here, and I'm puzzled as to why HHVM patched it as an issue - maybe just to silence a static analyzer that didn't look inside or figure out BF_decode?

I'll double-check before closing this issue.

[solardiz]

why HHVM patched it as an issue - maybe just to silence a static analyzer that didn't look inside or figure out BF_decode?

This is indeed hard to figure out. We pass 16 as byte size to it, and it could be unclear that 22 characters is still within that size. But it is. And indeed we check for out of range characters in there and return non-zero, which the if above catches:

#define BF_safe_atoi64(dst, src) \
{ \
        tmp = (unsigned char)(src); \
        if ((unsigned int)(tmp -= 0x20) >= 0x60) return -1; \

So HHVM's added check looks redundant to me, at least in our codebase here (I didn't check full context in theirs).

[solardiz]

could result in an out-of-bounds read.

Indeed, https://hhvm.com/blog/2020/11/12/security-update.html says "out of bounds read in crypt()". But it is unclear whether they refer to this specific change - they're making more changes to crypt() in another function. So maybe they actually fixed an issue specific to them, perhaps in those other changes.