From 794ae6cd60cc9c3d2bff81ea9ca704ba7c8fb6c0 Mon Sep 17 00:00:00 2001 From: MarkLee131 Date: Sat, 25 Apr 2026 10:39:13 +0800 Subject: [PATCH 1/3] User: use constant-time compare for MD5/SHA256/plain password paths CString::Equals falls through to strcmp, which short-circuits on the first differing byte. That leaks the length of the common prefix between the stored hash (or plain password) and the attacker's guess via response timing. Argon2id already uses argon2id_verify which is constant-time. Add a small ConstantTimeEquals helper and use it for the legacy MD5, SHA256 and plain HASH_NONE branches. No new dependency: the helper is ~10 lines and works on builds without OpenSSL. --- src/User.cpp | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/User.cpp b/src/User.cpp index 42c7c5ec..bc438182 100644 --- a/src/User.cpp +++ b/src/User.cpp @@ -1054,6 +1054,25 @@ CConfig CUser::ToConfig() const { return config; } +// Constant-time byte-wise compare of two strings. Returns true only if the +// strings have the same length and the same contents. Runs in time +// proportional to the shorter of the two lengths and does not short-circuit +// on the first differing byte, so a remote attacker can't learn the stored +// hash byte-by-byte via timing. Local helper to avoid a new OpenSSL +// dependency for builds without libssl. +static bool ConstantTimeEquals(const CString& a, const CString& b) { + if (a.length() != b.length()) { + return false; + } + unsigned char acc = 0; + const unsigned char* pa = reinterpret_cast(a.data()); + const unsigned char* pb = reinterpret_cast(b.data()); + for (size_t i = 0; i < a.length(); ++i) { + acc |= pa[i] ^ pb[i]; + } + return acc == 0; +} + bool CUser::CheckPass(const CString& sPass) { if(AuthOnlyViaModule() || CZNC::Get().GetAuthOnlyViaModule()) { return false; @@ -1063,11 +1082,13 @@ bool CUser::CheckPass(const CString& sPass) { bool bUpgrade = false; switch (m_eHashType) { case HASH_MD5: - bResult = m_sPass.Equals(CUtils::SaltedMD5Hash(sPass, m_sPassSalt)); + bResult = ConstantTimeEquals( + m_sPass, CUtils::SaltedMD5Hash(sPass, m_sPassSalt)); bUpgrade = true; break; case HASH_SHA256: - bResult = m_sPass.Equals(CUtils::SaltedSHA256Hash(sPass, m_sPassSalt)); + bResult = ConstantTimeEquals( + m_sPass, CUtils::SaltedSHA256Hash(sPass, m_sPassSalt)); #if ZNC_HAVE_ARGON bUpgrade = true; #endif @@ -1082,7 +1103,7 @@ bool CUser::CheckPass(const CString& sPass) { case HASH_NONE: // Don't upgrade hash, since the only valid use case for plain are // manual tests, where it's simpler this way - return (sPass == m_sPass); + return ConstantTimeEquals(sPass, m_sPass); } if (bResult && bUpgrade) { From f0c3341e35731425f890fe7bf2e63efb53e6863b Mon Sep 17 00:00:00 2001 From: MarkLee131 Date: Sat, 25 Apr 2026 17:37:33 +0800 Subject: [PATCH 2/3] User,Utils: move ConstantTimeEquals to CUtils and add tests (#2011) --- include/znc/Utils.h | 5 +++++ src/User.cpp | 25 +++-------------------- src/Utils.cpp | 17 ++++++++++++++++ test/UserTest.cpp | 49 +++++++++++++++++++++++++++++++++++++++++++++ test/UtilsTest.cpp | 28 ++++++++++++++++++++++++++ 5 files changed, 102 insertions(+), 22 deletions(-) diff --git a/include/znc/Utils.h b/include/znc/Utils.h index dffdc226..d7fdc4b4 100644 --- a/include/znc/Utils.h +++ b/include/znc/Utils.h @@ -62,6 +62,11 @@ class CUtils { static CString SaltedMD5Hash(const CString& sPass, const CString& sSalt); static CString SaltedSHA256Hash(const CString& sPass, const CString& sSalt); static CString SaltedHash(const CString& sPass, const CString& sSalt); + /** Byte-wise compare in time proportional to the shorter of the two + * lengths, without short-circuiting on the first differing byte. Used + * for password / hash comparisons so a remote attacker can't recover + * the stored secret one byte at a time via response timing. */ + static bool ConstantTimeEquals(const CString& a, const CString& b); static CString GetPass(const CString& sPrompt); static bool GetInput(const CString& sPrompt, CString& sRet, const CString& sDefault = "", diff --git a/src/User.cpp b/src/User.cpp index bc438182..ded6d19a 100644 --- a/src/User.cpp +++ b/src/User.cpp @@ -1054,25 +1054,6 @@ CConfig CUser::ToConfig() const { return config; } -// Constant-time byte-wise compare of two strings. Returns true only if the -// strings have the same length and the same contents. Runs in time -// proportional to the shorter of the two lengths and does not short-circuit -// on the first differing byte, so a remote attacker can't learn the stored -// hash byte-by-byte via timing. Local helper to avoid a new OpenSSL -// dependency for builds without libssl. -static bool ConstantTimeEquals(const CString& a, const CString& b) { - if (a.length() != b.length()) { - return false; - } - unsigned char acc = 0; - const unsigned char* pa = reinterpret_cast(a.data()); - const unsigned char* pb = reinterpret_cast(b.data()); - for (size_t i = 0; i < a.length(); ++i) { - acc |= pa[i] ^ pb[i]; - } - return acc == 0; -} - bool CUser::CheckPass(const CString& sPass) { if(AuthOnlyViaModule() || CZNC::Get().GetAuthOnlyViaModule()) { return false; @@ -1082,12 +1063,12 @@ bool CUser::CheckPass(const CString& sPass) { bool bUpgrade = false; switch (m_eHashType) { case HASH_MD5: - bResult = ConstantTimeEquals( + bResult = CUtils::ConstantTimeEquals( m_sPass, CUtils::SaltedMD5Hash(sPass, m_sPassSalt)); bUpgrade = true; break; case HASH_SHA256: - bResult = ConstantTimeEquals( + bResult = CUtils::ConstantTimeEquals( m_sPass, CUtils::SaltedSHA256Hash(sPass, m_sPassSalt)); #if ZNC_HAVE_ARGON bUpgrade = true; @@ -1103,7 +1084,7 @@ bool CUser::CheckPass(const CString& sPass) { case HASH_NONE: // Don't upgrade hash, since the only valid use case for plain are // manual tests, where it's simpler this way - return ConstantTimeEquals(sPass, m_sPass); + return CUtils::ConstantTimeEquals(sPass, m_sPass); } if (bResult && bUpgrade) { diff --git a/src/Utils.cpp b/src/Utils.cpp index 95d6db71..d83e3ee5 100644 --- a/src/Utils.cpp +++ b/src/Utils.cpp @@ -271,6 +271,23 @@ CString CUtils::SaltedHash(const CString& sPass, const CString& sSalt) { #endif } +bool CUtils::ConstantTimeEquals(const CString& a, const CString& b) { + // Length is leaked, but for the cases this is used in (fixed-size + // hex hashes for MD5 / SHA256) the lengths are constant. Plain-text + // mode does leak length, but plain-text passwords are deprecated and + // discouraged in znc.conf. + if (a.length() != b.length()) { + return false; + } + unsigned char acc = 0; + const unsigned char* pa = reinterpret_cast(a.data()); + const unsigned char* pb = reinterpret_cast(b.data()); + for (size_t i = 0; i < a.length(); ++i) { + acc |= static_cast(pa[i] ^ pb[i]); + } + return acc == 0; +} + CString CUtils::GetPass(const CString& sPrompt) { #ifdef HAVE_TCSETATTR // Disable echo diff --git a/test/UserTest.cpp b/test/UserTest.cpp index 4c7fb084..edcf5d5e 100644 --- a/test/UserTest.cpp +++ b/test/UserTest.cpp @@ -16,6 +16,7 @@ #include #include +#include #include class UserTest : public ::testing::Test { @@ -149,3 +150,51 @@ TEST_F(UserTest, TestAuthOnlyViaModule) { CZNC::Get().SetAuthOnlyViaModule(bAuthOnlyViaModuleDefault); } + +// Functional regression for the constant-time CheckPass paths (#2011). +// We can't measure timing in a unit test, but we verify the boolean +// contract for each hash mode: correct password matches, wrong password +// (including same-length and length-mismatch variants) does not. +TEST_F(UserTest, CheckPassPlain) { + CUser user("user"); + user.SetPass("plaintext", CUser::HASH_NONE); + + EXPECT_FALSE(user.CheckPass("")); + EXPECT_FALSE(user.CheckPass("plaintex")); // shorter + EXPECT_FALSE(user.CheckPass("plaintextX")); // longer + EXPECT_FALSE(user.CheckPass("plaintexT")); // case-sensitive + EXPECT_FALSE(user.CheckPass("Xlaintext")); // first byte differs + EXPECT_FALSE(user.CheckPass("plaintexX")); // last byte differs + EXPECT_TRUE(user.CheckPass("plaintext")); +} + +TEST_F(UserTest, CheckPassMD5) { + CUser user("user"); + CString sSalt = "the-salt"; + CString sCorrect = "correct-password"; + user.SetPass(CUtils::SaltedMD5Hash(sCorrect, sSalt), CUser::HASH_MD5, + sSalt); + + // Wrong inputs first; the success path may upgrade the stored hash + // to argon2id, after which the hash type is no longer MD5. + EXPECT_FALSE(user.CheckPass("")); + EXPECT_FALSE(user.CheckPass("wrong-password")); + EXPECT_FALSE(user.CheckPass("correct-passworX")); // last byte differs + EXPECT_FALSE(user.CheckPass("Xorrect-password")); // first byte differs + EXPECT_FALSE(user.CheckPass("Correct-password")); // case-sensitive + EXPECT_TRUE(user.CheckPass(sCorrect)); +} + +TEST_F(UserTest, CheckPassSHA256) { + CUser user("user"); + CString sSalt = "another-salt"; + CString sCorrect = "another-password"; + user.SetPass(CUtils::SaltedSHA256Hash(sCorrect, sSalt), + CUser::HASH_SHA256, sSalt); + + EXPECT_FALSE(user.CheckPass("")); + EXPECT_FALSE(user.CheckPass("wrong")); + EXPECT_FALSE(user.CheckPass("another-passworX")); + EXPECT_FALSE(user.CheckPass("Another-password")); + EXPECT_TRUE(user.CheckPass(sCorrect)); +} diff --git a/test/UtilsTest.cpp b/test/UtilsTest.cpp index 30571a67..fe0d3041 100644 --- a/test/UtilsTest.cpp +++ b/test/UtilsTest.cpp @@ -141,6 +141,34 @@ TEST(UtilsTest, ServerTime) { tzset(); } +TEST(UtilsTest, ConstantTimeEquals) { + // Functional correctness for the helper introduced for #2011. + // We can't measure timing in a unit test, so we verify the boolean + // contract: equal inputs match, any difference (length or content) + // does not. + EXPECT_TRUE(CUtils::ConstantTimeEquals("", "")); + EXPECT_TRUE(CUtils::ConstantTimeEquals("abc", "abc")); + EXPECT_TRUE(CUtils::ConstantTimeEquals(CString("\x00\x01\x02", 3), + CString("\x00\x01\x02", 3))); + + // Differs in last byte (the hardest case for short-circuit compare). + EXPECT_FALSE(CUtils::ConstantTimeEquals("abc", "abd")); + // Differs in first byte. + EXPECT_FALSE(CUtils::ConstantTimeEquals("abc", "Xbc")); + // Length mismatch on either side. + EXPECT_FALSE(CUtils::ConstantTimeEquals("abc", "abcd")); + EXPECT_FALSE(CUtils::ConstantTimeEquals("abcd", "abc")); + EXPECT_FALSE(CUtils::ConstantTimeEquals("", "x")); + EXPECT_FALSE(CUtils::ConstantTimeEquals("x", "")); + // Case-sensitive (unlike CString::Equals default). + EXPECT_FALSE(CUtils::ConstantTimeEquals("abc", "ABC")); + // Embedded NUL is compared, not used as a terminator. + EXPECT_FALSE(CUtils::ConstantTimeEquals(CString("a\x00""c", 3), + CString("a\x00""d", 3))); + EXPECT_TRUE(CUtils::ConstantTimeEquals(CString("a\x00""c", 3), + CString("a\x00""c", 3))); +} + TEST(UtilsTest, ParseServerTime) { char* oldTZ = getenv("TZ"); if (oldTZ) oldTZ = strdup(oldTZ); From c9d0677e4fe2be07af0938aabac2fe6ee1635545 Mon Sep 17 00:00:00 2001 From: MarkLee131 Date: Wed, 29 Apr 2026 17:06:49 +0800 Subject: [PATCH 3/3] Utils: prefer CRYPTO_memcmp in ConstantTimeEquals when OpenSSL is available Per review feedback on #2017: a hand-rolled byte loop can in principle be folded back into a short-circuiting compare by an aggressive optimizer. Use CRYPTO_memcmp under HAVE_LIBSSL since OpenSSL is already a build dependency for the SHA256 path. For non-OpenSSL builds, mark the accumulator and pointers volatile and note in a comment that this is best-effort. --- src/Utils.cpp | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/Utils.cpp b/src/Utils.cpp index d83e3ee5..22d264c7 100644 --- a/src/Utils.cpp +++ b/src/Utils.cpp @@ -28,6 +28,7 @@ #ifdef HAVE_LIBSSL #include #include +#include #include #if (OPENSSL_VERSION_NUMBER < 0x10100000L) || (defined(LIBRESSL_VERSION_NUMBER) && (LIBRESSL_VERSION_NUMBER < 0x20700000L)) #define X509_getm_notBefore X509_get_notBefore @@ -279,13 +280,22 @@ bool CUtils::ConstantTimeEquals(const CString& a, const CString& b) { if (a.length() != b.length()) { return false; } - unsigned char acc = 0; - const unsigned char* pa = reinterpret_cast(a.data()); - const unsigned char* pb = reinterpret_cast(b.data()); +#ifdef HAVE_LIBSSL + return CRYPTO_memcmp(a.data(), b.data(), a.length()) == 0; +#else + // Best-effort fallback when OpenSSL is unavailable: an optimizer is + // in principle allowed to short-circuit this loop, so the volatile + // accumulator and pointers are a hint rather than a guarantee. + volatile unsigned char acc = 0; + const volatile unsigned char* pa = + reinterpret_cast(a.data()); + const volatile unsigned char* pb = + reinterpret_cast(b.data()); for (size_t i = 0; i < a.length(); ++i) { acc |= static_cast(pa[i] ^ pb[i]); } return acc == 0; +#endif } CString CUtils::GetPass(const CString& sPrompt) {