From aece8753625832ce1931b93e436eae839aae8a39 Mon Sep 17 00:00:00 2001
From: scatterflower <2956568+scatterflower@users.noreply.github.com>
Date: Thu, 30 Jun 2022 15:13:22 -0500
Subject: [PATCH] Add more secure password hashing support (#293)
* Separate crypto operations into new class
* Implement PBKDF2 backend
* Add auto-update to stronger hashes
* clang-format pass
* documentation and cleanup
* Add unit tests for CryptoHelper
Co-authored-by: scatterflower <marisa@scatterflower.online>
---
core/include/aoclient.h | 3 -
core/include/crypto_helper.h | 160 ++++++++++++++++++
core/include/db_manager.h | 6 +-
core/src/commands/authentication.cpp | 22 +--
core/src/db_manager.cpp | 47 ++---
tests/tests.pro | 3 +-
tests/unittest_crypto/tst_unittest_crypto.cpp | 47 +++++
tests/unittest_crypto/unittest_crypto.pro | 5 +
8 files changed, 236 insertions(+), 57 deletions(-)
create mode 100644 core/include/crypto_helper.h
create mode 100644 tests/unittest_crypto/tst_unittest_crypto.cpp
create mode 100644 tests/unittest_crypto/unittest_crypto.pro
diff --git a/core/include/aoclient.h b/core/include/aoclient.h
index b7af48f..ee8037b 100644
--- a/core/include/aoclient.h
+++ b/core/include/aoclient.h
@@ -25,9 +25,6 @@
#include <QRegularExpression>
#include <QTimer>
#include <QtGlobal>
-#if QT_VERSION > QT_VERSION_CHECK(5, 10, 0)
-#include <QRandomGenerator>
-#endif
#include "include/acl_roles_handler.h"
#include "include/network/aopacket.h"
diff --git a/core/include/crypto_helper.h b/core/include/crypto_helper.h
new file mode 100644
index 0000000..1cd4e00
--- /dev/null
+++ b/core/include/crypto_helper.h
@@ -0,0 +1,160 @@
+//////////////////////////////////////////////////////////////////////////////////////
+// akashi - a server for Attorney Online 2 //
+// Copyright (C) 2022 scatterflower //
+// //
+// This program is free software: you can redistribute it and/or modify //
+// it under the terms of the GNU Affero General Public License as //
+// published by the Free Software Foundation, either version 3 of the //
+// License, or (at your option) any later version. //
+// //
+// This program is distributed in the hope that it will be useful, //
+// but WITHOUT ANY WARRANTY; without even the implied warranty of //
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the //
+// GNU Affero General Public License for more details. //
+// //
+// You should have received a copy of the GNU Affero General Public License //
+// along with this program. If not, see <https://www.gnu.org/licenses/>. //
+//////////////////////////////////////////////////////////////////////////////////////
+#ifndef CRYPTO_HELPER_H
+#define CRYPTO_HELPER_H
+
+#include <QMessageAuthenticationCode>
+#include <QString>
+#if QT_VERSION > QT_VERSION_CHECK(5, 10, 0)
+#include <QRandomGenerator>
+#endif
+
+/**
+ * @brief Simple header library for basic cryptographic functionality
+ */
+class CryptoHelper
+{
+ private:
+ /**
+ * @brief Length of the output of PBKDF2
+ */
+ static constexpr qint32 pbkdf2_output_len = 32; // 32 bytes (SHA-256)
+ /**
+ * @brief Configurable cost parameter of PBKDF2
+ */
+ static constexpr quint32 pbkdf2_cost = 100000;
+
+ /**
+ * @brief Compute the SHA-256 HMAC of given data
+ *
+ * @param salt HMAC key
+ * @param password HMAC data
+ * @return QByteArray HMAC result
+ */
+ static QByteArray hmac(QByteArray salt, QByteArray password)
+ {
+ QMessageAuthenticationCode hmac(QCryptographicHash::Sha256);
+ hmac.setKey(salt);
+ hmac.addData(password);
+ return hmac.result();
+ }
+
+ /**
+ * @brief Compute the SHA-256 HMAC of strings
+ *
+ * @param salt Salt value
+ * @param password Password value
+ * @return QString HMAC result, hex encoded
+ */
+ static QString password_hmac(QString salt, QString password)
+ {
+ return hmac(salt.toUtf8(), password.toUtf8()).toHex();
+ }
+
+ /**
+ * @brief Perform the PBKDF2 key-derivation function
+ *
+ * @details PBKDF2 is a very configurable algorithm, but most of its functionality
+ * does not apply here. Instead, we fix the output to the size of the underlying
+ * hash function, which greatly simplifies the algorithm.
+ *
+ * @param salt Salt value
+ * @param password Password value
+ * @return QString PBKDF2 result, hex encoded
+ */
+ static QString pbkdf2(QByteArray salt, QString password)
+ {
+ QByteArray bigendian_one("\x00\x00\x00\x01", 4);
+ QByteArray last_block = salt;
+ last_block.append(bigendian_one);
+
+ QByteArray result(pbkdf2_output_len, '\0');
+
+ for (unsigned int i = 0; i < pbkdf2_cost; i++) {
+ last_block = hmac(password.toUtf8(), last_block);
+ for (unsigned int n = 0; n < pbkdf2_output_len; n++)
+ result[n] = result[n] ^ last_block[n];
+ }
+
+ return result.toHex();
+ }
+
+ public:
+ /**
+ * @brief The length of the salt value used for PBKDF2, in bytes
+ */
+ static constexpr qint32 pbkdf2_salt_len = 16;
+
+ /**
+ * @brief Compute the hash of a password to store, given a salt
+ *
+ * @details This function selects one of two hashing algorithm backends to use,
+ * dependent on the length of the salt. Older versions of this program used an
+ * 8-byte salt, but newer versions use a 16-byte salt. This allows us to version
+ * the algorithm being used without the need to change the schema of the database.
+ *
+ * @param salt Salt value
+ * @param password Password value
+ * @return QString Hashed password, hex encoded
+ */
+ static QString hash_password(QByteArray salt, QString password)
+ {
+ // Select the correct hash backend based on the salt length
+ if (salt.length() < pbkdf2_salt_len) {
+ // Due to an implementation oversight, the old HMAC algorithm
+ // does not correctly handle salts. Instead of treating the hex string
+ // as binary data, it is used directly. We have to handle this case.
+ return password_hmac(salt.toHex(), password);
+ }
+
+ return pbkdf2(salt, password);
+ }
+
+ /**
+ * @brief Generate a random octet
+ *
+ * @return quint8 A random 8-bit value
+ */
+ static quint8 rand8()
+ {
+#if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
+ qsrand(QDateTime::currentMSecsSinceEpoch());
+ quint32 l_rand = qrand();
+#else
+ quint32 l_rand = QRandomGenerator::system()->generate();
+#endif
+ return (quint8)(l_rand & 0xFF);
+ }
+
+ /**
+ * @brief Generate an arbitrary amount of random data
+ *
+ * @param n Number of bytes to generate
+ * @return QByteArray Random bytes
+ */
+ static QByteArray randbytes(int n)
+ {
+ QByteArray output;
+ for (int i = 0; i < n; i++) {
+ output.append(CryptoHelper::rand8());
+ }
+ return output;
+ }
+};
+
+#endif
\ No newline at end of file
diff --git a/core/include/db_manager.h b/core/include/db_manager.h
index 3f0fdbe..ca29a8f 100644
--- a/core/include/db_manager.h
+++ b/core/include/db_manager.h
@@ -23,13 +23,13 @@
#include <QDateTime>
#include <QFileInfo>
#include <QHostAddress>
-#include <QMessageAuthenticationCode>
#include <QSqlDatabase>
#include <QSqlDriver>
#include <QSqlError>
#include <QSqlQuery>
#include "include/acl_roles_handler.h"
+#include "include/crypto_helper.h"
/**
* @brief A class used to handle database interaction.
@@ -147,7 +147,7 @@ class DBManager : public QObject
* @see AOClient#cmdLogin and AOClient#cmdLogout for the username and password's contexts.
* @see ACLRolesHandler for details regarding ACL roles and ACL role identifiers.
*/
- bool createUser(QString username, QString salt, QString password, QString acl);
+ bool createUser(QString username, QByteArray salt, QString password, QString acl);
/**
* @brief Deletes an authorised user from the database.
@@ -274,4 +274,4 @@ class DBManager : public QObject
void updateDB(int current_version);
};
-#endif // BAN_MANAGER_H
+#endif // BAN_MANAGER_H
\ No newline at end of file
diff --git a/core/src/commands/authentication.cpp b/core/src/commands/authentication.cpp
index a404191..e1f2ae5 100644
--- a/core/src/commands/authentication.cpp
+++ b/core/src/commands/authentication.cpp
@@ -18,6 +18,7 @@
#include "include/aoclient.h"
#include "include/config_manager.h"
+#include "include/crypto_helper.h"
#include "include/db_manager.h"
#include "include/server.h"
@@ -80,15 +81,7 @@ void AOClient::cmdSetRootPass(int argc, QStringList argv)
m_authenticated = false;
ConfigManager::setAuthType(DataTypes::AuthType::ADVANCED);
-#if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
- qsrand(QDateTime::currentMSecsSinceEpoch());
- quint32 l_upper_salt = qrand();
- quint32 l_lower_salt = qrand();
- quint64 l_salt_number = (upper_salt << 32) | lower_salt;
-#else
- quint64 l_salt_number = QRandomGenerator::system()->generate64();
-#endif
- QString l_salt = QStringLiteral("%1").arg(l_salt_number, 16, 16, QLatin1Char('0'));
+ QByteArray l_salt = CryptoHelper::randbytes(16);
server->getDatabaseManager()->createUser("root", l_salt, argv[0], ACLRolesHandler::SUPER_ID);
}
@@ -101,15 +94,8 @@ void AOClient::cmdAddUser(int argc, QStringList argv)
sendServerMessage("Password does not meet server requirements.");
return;
}
-#if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
- qsrand(QDateTime::currentMSecsSinceEpoch());
- quint32 l_upper_salt = qrand();
- quint32 l_lower_salt = qrand();
- quint64 l_salt_number = (upper_salt << 32) | lower_salt;
-#else
- quint64 l_salt_number = QRandomGenerator::system()->generate64();
-#endif
- QString l_salt = QStringLiteral("%1").arg(l_salt_number, 16, 16, QLatin1Char('0'));
+
+ QByteArray l_salt = CryptoHelper::randbytes(16);
if (server->getDatabaseManager()->createUser(argv[0], l_salt, argv[1], ACLRolesHandler::NONE_ID))
sendServerMessage("Created user " + argv[0] + ".\nUse /setperms to modify their permissions.");
diff --git a/core/src/db_manager.cpp b/core/src/db_manager.cpp
index 34aec81..9b2b11d 100644
--- a/core/src/db_manager.cpp
+++ b/core/src/db_manager.cpp
@@ -171,7 +171,7 @@ bool DBManager::invalidateBan(int id)
return true;
}
-bool DBManager::createUser(QString f_username, QString f_salt, QString f_password, QString f_acl)
+bool DBManager::createUser(QString f_username, QByteArray f_salt, QString f_password, QString f_acl)
{
QSqlQuery username_exists;
username_exists.prepare("SELECT ACL FROM users WHERE USERNAME = ?");
@@ -183,15 +183,11 @@ bool DBManager::createUser(QString f_username, QString f_salt, QString f_passwor
QSqlQuery query;
- QString salted_password;
- QMessageAuthenticationCode hmac(QCryptographicHash::Sha256);
- hmac.setKey(f_salt.toUtf8());
- hmac.addData(f_password.toUtf8());
- salted_password = hmac.result().toHex();
+ QString salted_password = CryptoHelper::hash_password(f_salt, f_password);
query.prepare("INSERT INTO users(USERNAME, SALT, PASSWORD, ACL) VALUES(?, ?, ?, ?)");
query.addBindValue(f_username);
- query.addBindValue(f_salt);
+ query.addBindValue(f_salt.toHex());
query.addBindValue(salted_password);
query.addBindValue(f_acl);
query.exec();
@@ -247,19 +243,20 @@ bool DBManager::authenticate(QString username, QString password)
return false;
QString salt = query_salt.value(0).toString();
- QString salted_password;
- QMessageAuthenticationCode hmac(QCryptographicHash::Sha256);
- hmac.setKey(salt.toUtf8());
- hmac.addData(password.toUtf8());
- salted_password = hmac.result().toHex();
+ QString salted_password = CryptoHelper::hash_password(QByteArray::fromHex(salt.toUtf8()), password);
- QSqlQuery query_pass("SELECT PASSWORD FROM users WHERE SALT = ?");
- query_pass.addBindValue(salt);
+ QSqlQuery query_pass("SELECT PASSWORD FROM users WHERE USERNAME = ?");
+ query_pass.addBindValue(username);
query_pass.exec();
if (!query_pass.first())
return false;
QString stored_pass = query_pass.value(0).toString();
+ // Update old-style hashes to new ones on the fly
+ if (QByteArray::fromHex(salt.toUtf8()).length() < CryptoHelper::pbkdf2_salt_len && salted_password == stored_pass) {
+ updatePassword(username, password);
+ }
+
return salted_password == stored_pass;
}
@@ -353,27 +350,13 @@ bool DBManager::updateBan(int ban_id, QString field, QVariant updated_info)
bool DBManager::updatePassword(QString username, QString password)
{
- QString salt;
- QSqlQuery salt_check;
- salt_check.prepare("SELECT SALT FROM users WHERE USERNAME = ?");
- salt_check.addBindValue(username);
- salt_check.exec();
-
- if (!salt_check.first())
- return false;
- else
- salt = salt_check.value(0).toString();
+ QByteArray salt = CryptoHelper::randbytes(16);
+ QString salted_password = CryptoHelper::hash_password(salt, password);
QSqlQuery query;
-
- QString salted_password;
- QMessageAuthenticationCode hmac(QCryptographicHash::Sha256);
- hmac.setKey(salt.toUtf8());
- hmac.addData(password.toUtf8());
- salted_password = hmac.result().toHex();
-
- query.prepare("UPDATE users SET PASSWORD = ? WHERE USERNAME = ?");
+ query.prepare("UPDATE users SET PASSWORD = ?, SALT = ? WHERE USERNAME = ?");
query.addBindValue(salted_password);
+ query.addBindValue(salt.toHex());
query.addBindValue(username);
query.exec();
return true;
diff --git a/tests/tests.pro b/tests/tests.pro
index c7132ca..0f45ffc 100644
--- a/tests/tests.pro
+++ b/tests/tests.pro
@@ -5,4 +5,5 @@ SUBDIRS += \
unittest_music_manager \
unittest_acl_roles_handler \
unittest_command_extension \
- unittest_aopacket
\ No newline at end of file
+ unittest_aopacket \
+ unittest_crypto
\ No newline at end of file
diff --git a/tests/unittest_crypto/tst_unittest_crypto.cpp b/tests/unittest_crypto/tst_unittest_crypto.cpp
new file mode 100644
index 0000000..0f107a2
--- /dev/null
+++ b/tests/unittest_crypto/tst_unittest_crypto.cpp
@@ -0,0 +1,47 @@
+#include <QTest>
+
+#include <include/crypto_helper.h>
+
+namespace tests {
+namespace unittests {
+
+class tst_Crypto : public QObject
+{
+ Q_OBJECT
+
+ public:
+ private slots:
+ void checkHash();
+ void checkHash_data();
+};
+
+void tst_Crypto::checkHash_data()
+{
+ QTest::addColumn<QString>("password");
+ QTest::addColumn<QString>("salt_hex");
+ QTest::addColumn<QString>("expected_hash");
+
+ QTest::newRow("HMAC only (old algorithm)") << "password"
+ << "73616c7473616c74"
+ << "4128058d074264779ec23ee1ffed65d7c4e16c93003315cd5cb85170770b254a";
+
+ QTest::newRow("PBKDF2 (new algorithm)") << "password"
+ << "73616c7473616c7473616c7473616c74"
+ << "4fbf2d122fe6afc61a81e9f2fe393ab39f906a78ddddc797763c0e784857e9b4";
+}
+
+void tst_Crypto::checkHash()
+{
+ QFETCH(QString, password);
+ QFETCH(QString, salt_hex);
+ QFETCH(QString, expected_hash);
+
+ QCOMPARE(CryptoHelper::hash_password(QByteArray::fromHex(salt_hex.toUtf8()), password), expected_hash);
+}
+
+}
+}
+
+QTEST_APPLESS_MAIN(tests::unittests::tst_Crypto)
+
+#include "tst_unittest_crypto.moc"
\ No newline at end of file
diff --git a/tests/unittest_crypto/unittest_crypto.pro b/tests/unittest_crypto/unittest_crypto.pro
new file mode 100644
index 0000000..d9cb213
--- /dev/null
+++ b/tests/unittest_crypto/unittest_crypto.pro
@@ -0,0 +1,5 @@
+QT -= gui
+
+include(../tests_common.pri)
+
+SOURCES += tst_unittest_crypto.cpp