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);