Fix out-of-bounds read in ValidateSalt for short salts

src/bcrypt_node.cc

@@ -22,7 +22,10 @@ namespace {
         // discard $
         salt++;
 
-        if (*salt > BCRYPT_VERSION) {
+        // A short, truncated salt such as "$" or "$2" must be rejected before
+        // reading further offsets, otherwise the fixed lookups below would read
+        // past the terminating NUL byte.
+        if (*salt == '\0' || *salt > BCRYPT_VERSION) {
             return false;
         }
 
@@ -37,10 +40,16 @@ namespace {
             }
         }
 
+        // the version must be followed by the '$' separator; bail out here so a
+        // salt like "$2b" cannot advance the pointer past its NUL terminator
+        if (salt[1] != '$') {
+            return false;
+        }
+
         // discard version + $
         salt += 2;
 
-        if (salt[2] != '$') {
+        if (salt[0] == '\0' || salt[1] == '\0' || salt[2] != '$') {
             return false;
         }
 

test/sync.test.js

@@ -70,6 +70,16 @@ test('hash_salt_validity', () => {
     expect(() => bcrypt.hashSync('password', 'some$value')).toThrow('Invalid salt. Salt must be in the form of: $Vers$log2(NumRounds)$saltvalue')
 })
 
+test('hash_short_salt_prefix', () => {
+    // Truncated "$"-prefixed salts must be rejected without reading past the
+    // end of the string (see ValidateSalt bounds handling).
+    const truncated = ['$', '$2', '$2b', '$2b$', '$2b$1', '$2b$10', '$2b$10$'];
+    for (const salt of truncated) {
+        expect(() => bcrypt.hashSync('password', salt)).toThrow('Invalid salt. Salt must be in the form of: $Vers$log2(NumRounds)$saltvalue')
+        expect(bcrypt.compareSync('password', salt)).toBe(false)
+    }
+})
+
 test('verify_salt', () => {
     const salt = bcrypt.genSaltSync(10);
     const split_salt = salt.split('$');