User,Utils: move ConstantTimeEquals to CUtils and add tests (#2011)

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 = "",

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<const unsigned char*>(a.data());
-    const unsigned char* pb = reinterpret_cast<const unsigned char*>(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) {

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<const unsigned char*>(a.data());
+    const unsigned char* pb = reinterpret_cast<const unsigned char*>(b.data());
+    for (size_t i = 0; i < a.length(); ++i) {
+        acc |= static_cast<unsigned char>(pa[i] ^ pb[i]);
+    }
+    return acc == 0;
+}
+
 CString CUtils::GetPass(const CString& sPrompt) {
 #ifdef HAVE_TCSETATTR
     // Disable echo

test/UserTest.cpp

@@ -16,6 +16,7 @@
 
 #include <gtest/gtest.h>
 #include <znc/User.h>
+#include <znc/Utils.h>
 #include <znc/znc.h>
 
 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));
+}

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