Possible out-of-bounds read in `ValidateSalt` for a short `$`-prefixed salt/hash

[OvOhao]

Possible out-of-bounds read in ValidateSalt for a short $-prefixed salt/hash

I found a possible out-of-bounds read (CWE-125) in ValidateSalt. After confirming the
first byte is $, the function walks the pointer forward and dereferences fixed offsets
without re-checking the string's NUL terminator, so a salt/hash that starts with $ but
is shorter than a full $2b$NN$ prefix causes reads 1-2 bytes past the string.

File: src/bcrypt_node.cc

Function: ValidateSalt (reached per call from Compare/CompareSync/Encrypt/EncryptSync)

bool ValidateSalt(const char* salt) {
    if (!salt || *salt != '$') return false;   // checks salt[0]
    salt++;                                     // -> index 1
    if (*salt > BCRYPT_VERSION) return false;   // reads index 1
    if (salt[1] != '$') {                       // reads index 2  <-- can be past the NUL
        switch (salt[1]) { case 'a': case 'b': salt++; break; default: return false; }
    }
    salt += 2;                                  // -> index 3
    if (salt[2] != '$') return false;           // reads index 5  <-- can be past the NUL
    ...
}

For input "$" (backing std::string bytes ['$','\0'], valid indices 0-1):
salt[0]=='$' passes, salt++ -> index 1, *salt=='\0' passes the version check, then
salt[1] reads index 2 — one byte past the terminator. For "$2$" the later
salt[2] read lands two bytes past the terminator.

JS trigger (any short $-prefixed salt/hash argument):

const bcrypt = require('bcrypt')
bcrypt.compareSync('pw', '$')          // ValidateSalt reads past the string
bcrypt.compareSync('pw', '$2$')

The same forward-walk pattern also exists in bcrypt() itself (src/bcrypt.cc, salt[1]
/ salt[2] after salt++/salt += 2), and in EncryptAsyncWorker::Execute the code
does not return after SetError, so bcrypt() still runs on the invalid salt.

Severity — honest assessment: LOW

The over-read is 1-2 bytes into the std::string's (usually small-string-optimized)
inline buffer, so it reads adjacent in-object bytes — no reliable crash, and the bytes
only steer a boolean validation branch (they are not returned to JavaScript), so there is
no information disclosure. It is a genuine memory-safety defect but not weaponizable
beyond a theoretical page-boundary segfault. (Contrast with the re2 finding, where
over-read bytes are returned to JS.)

Suggested fix: validate the full expected prefix length before indexing, e.g. check
strlen(salt) (or scan for the terminator) so no fixed offset is read past the string.