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 42c7c5ec..ded6d19a 100644 --- a/src/User.cpp +++ b/src/User.cpp @@ -1063,11 +1063,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 = CUtils::ConstantTimeEquals( + m_sPass, CUtils::SaltedMD5Hash(sPass, m_sPassSalt)); bUpgrade = true; break; case HASH_SHA256: - bResult = m_sPass.Equals(CUtils::SaltedSHA256Hash(sPass, m_sPassSalt)); + bResult = CUtils::ConstantTimeEquals( + m_sPass, CUtils::SaltedSHA256Hash(sPass, m_sPassSalt)); #if ZNC_HAVE_ARGON bUpgrade = true; #endif @@ -1082,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 (sPass == m_sPass); + return CUtils::ConstantTimeEquals(sPass, m_sPass); } if (bResult && bUpgrade) { diff --git a/src/Utils.cpp b/src/Utils.cpp index fe726c3f..ed2c9c83 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 @@ -271,6 +272,32 @@ 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; + } +#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) { #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 37ac2027..61c016b3 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);