Support for up to 256c passphrases, correct bitwise inversion in hashes

[gs-kamnas]

This PR updates the password buffer size to support up to 256 character passwords (the maximum supported by Microsoft in any known NTLM implementation) as well as addresses the following 2 issues:

• Hashes as generated and consumed have all bits complemented (making such disagree with all other NTHash implementations such as passlib). Therefore, this PR corrects this behavior however will require users using a pre-hashed password in config to recompute the hash.

• Passwords remain in memory (per the referenced FIXME) even when no longer necessary; this change adds an explicit memory barrier after the memset to zero on the password buffer in an event to prevent the compiler from optimizing away the memset.

[fralken]

• Hashes as generated and consumed have all bits complemented (making such disagree with all other NTHash implementations such as passlib). Therefore, this PR corrects this behavior however will require users using a pre-hashed password in config to recompute the hash.

Hello, this is a good catch! Actually the error is not in the hashing function but rather in the hexadecimal representation of a number, that indeed was wrong.

Regarding setting memory to zero for password buffers, rather than using __asm__ volatile ("" ::: "memory"); maybe it is better to use memset_s as explained in this SonarCube report .

[fralken]

I just checked on Godbolt and apparently with clang the asm directive has no effect. Indeed it works with gcc. Also the memset_s function is not very portable as explained in Zero'ing memory, compiler optimizations and memset_s.

They suggest using a function like

void erase_memory(void *pointer, size_t size_data, size_t size_to_erase) {
    if(size_to_erase > size_data) size_to_erase = size_data;
    volatile unsigned char *p = pointer;
    while (size_to_erase--){
        *p++ = 0;
    }
}

where the volatile pointer should not be optimised away.

[sonarcloud[bot]]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
2 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[gs-kamnas]

I just checked on Godbolt and apparently with clang the asm directive has no effect. Indeed it works with gcc. Also the memset_s function is not very portable as explained in Zero'ing memory, compiler optimizations and memset_s.

They suggest using a function like

void erase_memory(void *pointer, size_t size_data, size_t size_to_erase) {
  if(size_to_erase > size_data) size_to_erase = size_data;
  volatile unsigned char *p = pointer;
  while (size_to_erase--){
      *p++ = 0;
  }
}

where the volatile pointer should not be optimised away.

Interesting that clang handles as it does, therefore I have implemented a function that I'm calling "compat_memset_s" that uses your volatile pointer recommendation but should be otherwise API compatible to native memset_s; in case you later move to c11 with the not-so-portable stdlib extensions.

Appears to not be removed even when built with max optimizations(-O3): https://godbolt.org/z/T34Y8cr5n