adam/znc
1
0 Fork 0
mirror of https://github.com/znc/znc.git synced 2026-10-31 01:32:02 +01:00
Code Issues 362 Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-10-05 05:02:45 +02:00 by adam · 1 comment
adam commented 2026-10-05 05:02:45 +02:00
Owner
Copy link
Copy source

CUser::CheckPass: legacy MD5/SHA256 path uses strcmp-based hash compare

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.

# `CUser::CheckPass`: legacy MD5/SHA256 path uses `strcmp`-based hash compare 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. <!-- github-sync-id:4326794172 -->
adam closed this issue 2026-10-05 05:02:45 +02:00
adam commented 2026-10-05 05:02:46 +02:00
Author
Owner
Copy link
Copy source

Sorry, this was an accidental duplicate submission on my side. #2011 has the canonical content, please track that one and feel free to close this.

Sorry, this was an accidental duplicate submission on my side. #2011 has the canonical content, please track that one and feel free to close this. <!-- github-comment-id:4318630730 -->
Sign in to join this conversation.
No Branch/Tag Specified
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
adam (Adam Wojcieszonek)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: adam/znc#1089