User: legacy MD5/SHA256 CheckPass uses non-constant-time compare #1095

New Issue

2026-10-05T05:02:58+02:00

adam commented

2026-10-05 05:02:58 +02:00

For HASH_MD5 and HASH_SHA256, CheckPass ends up doing m_sPass.Equals(salted_hash) which routes through strcmp (CString::StrCmp at src/ZNCString.cpp:148-150). That's not a constant-time compare.

// src/User.cpp
1057: bool CUser::CheckPass(const CString& sPass) {
...
1064:     switch (m_eHashType) {
1065:         case HASH_MD5:
1066:             bResult = m_sPass.Equals(CUtils::SaltedMD5Hash(sPass, m_sPassSalt));
...
1069:         case HASH_SHA256:
1070:             bResult = m_sPass.Equals(CUtils::SaltedSHA256Hash(sPass, m_sPassSalt));
...
1077:         case HASH_ARGON2ID:
1078:             return argon2id_verify(...) == ARGON2_OK;     // constant-time
...
1085:         case HASH_NONE:
1086:             return (sPass == m_sPass);                    // also not CT

The Argon2id branch is fine; only the legacy hashes are affected.

The leak via remote timing is the stored hash digest, not the password itself. Recovering 64 hex chars of a SHA256 hash via remote timing requires a lot of measurements and ZNC has fail2ban available for rate-limiting, so this is more "code quality / future-proofing" than an exploitable vulnerability. Filing because every other piece of crypto-adjacent code in the project uses constant-time comparisons.

Fix

case HASH_SHA256: {
    CString expected = CUtils::SaltedSHA256Hash(sPass, m_sPassSalt);
    bResult = expected.length() == m_sPass.length() &&
              CRYPTO_memcmp(expected.data(), m_sPass.data(),
                            m_sPass.length()) == 0;
#if ZNC_HAVE_ARGON
    bUpgrade = true;
#endif
    break;
}

Same idea for HASH_MD5. CRYPTO_memcmp is in OpenSSL and ZNC already links against OpenSSL when WANT_OPENSSL=ON. For builds without OpenSSL, a two-line fallback that XORs all bytes and ORs the result works.

Could PR if you'd like.

For `HASH_MD5` and `HASH_SHA256`, `CheckPass` ends up doing `m_sPass.Equals(salted_hash)` which routes through `strcmp` (`CString::StrCmp` at `src/ZNCString.cpp:148-150`). That's not a constant-time compare. ```cpp // src/User.cpp 1057: bool CUser::CheckPass(const CString& sPass) { ... 1064: switch (m_eHashType) { 1065: case HASH_MD5: 1066: bResult = m_sPass.Equals(CUtils::SaltedMD5Hash(sPass, m_sPassSalt)); ... 1069: case HASH_SHA256: 1070: bResult = m_sPass.Equals(CUtils::SaltedSHA256Hash(sPass, m_sPassSalt)); ... 1077: case HASH_ARGON2ID: 1078: return argon2id_verify(...) == ARGON2_OK; // constant-time ... 1085: case HASH_NONE: 1086: return (sPass == m_sPass); // also not CT ``` The Argon2id branch is fine; only the legacy hashes are affected. The leak via remote timing is the stored hash digest, not the password itself. Recovering 64 hex chars of a SHA256 hash via remote timing requires a lot of measurements and ZNC has fail2ban available for rate-limiting, so this is more "code quality / future-proofing" than an exploitable vulnerability. Filing because every other piece of crypto-adjacent code in the project uses constant-time comparisons. ## Fix ```cpp case HASH_SHA256: { CString expected = CUtils::SaltedSHA256Hash(sPass, m_sPassSalt); bResult = expected.length() == m_sPass.length() && CRYPTO_memcmp(expected.data(), m_sPass.data(), m_sPass.length()) == 0; #if ZNC_HAVE_ARGON bUpgrade = true; #endif break; } ``` Same idea for `HASH_MD5`. `CRYPTO_memcmp` is in OpenSSL and ZNC already links against OpenSSL when `WANT_OPENSSL=ON`. For builds without OpenSSL, a two-line fallback that XORs all bytes and ORs the result works. Could PR if you'd like.

adam closed this issue

2026-10-05 05:02:58 +02:00

adam commented

2026-10-05 05:02:59 +02:00

same as #2005 ?

same as #2005 ?

adam commented

2026-10-05 05:03:00 +02:00

Sorry, #2005 was an accidental duplicate submission on my side and I have closed it now. Please refer to this issue since it has the canonical content.

Sorry, #2005 was an accidental duplicate submission on my side and I have closed it now. Please refer to this issue since it has the canonical content.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: adam/znc#1095