Merge pull request #2017 from MarkLee131/fix/checkpass-constant-time

User: use constant-time compare for MD5/SHA256/plain password paths

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

@@ -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) {

src/Utils.cpp

@@ -28,6 +28,7 @@
 #ifdef HAVE_LIBSSL
 #include <openssl/ssl.h>
 #include <openssl/bn.h>
+#include <openssl/crypto.h>
 #include <openssl/rsa.h>
 #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<const volatile unsigned char*>(a.data());
+    const volatile unsigned char* pb =
+        reinterpret_cast<const volatile 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;
+#endif
+}
+
 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);