diff --git a/src/cli/Utils.cpp b/src/cli/Utils.cpp index e25ffe02d..d35300b4b 100644 --- a/src/cli/Utils.cpp +++ b/src/cli/Utils.cpp @@ -132,9 +132,9 @@ namespace Utils return {}; } - if (fileKey->type() != FileKey::Hashed) { - err << QObject::tr("WARNING: You are using a legacy key file format which may become\n" - "unsupported in the future.\n\n" + if (fileKey->type() != FileKey::KeePass2XMLv2 && fileKey->type() != FileKey::Hashed) { + err << QObject::tr("WARNING: You are using an old key file format which KeePassXC may\n" + "stop supporting in the future.\n\n" "Please consider generating a new key file.") << endl; } diff --git a/src/gui/DatabaseOpenWidget.cpp b/src/gui/DatabaseOpenWidget.cpp index 6cf61e20a..b42555738 100644 --- a/src/gui/DatabaseOpenWidget.cpp +++ b/src/gui/DatabaseOpenWidget.cpp @@ -302,12 +302,14 @@ QSharedPointer DatabaseOpenWidget::buildDatabaseKey() m_ui->messageWidget->showMessage(tr("Failed to open key file: %1").arg(errorMsg), MessageWidget::Error); return {}; } - if (key->type() != FileKey::Hashed && !config()->get(Config::Messages_NoLegacyKeyFileWarning).toBool()) { + if (key->type() != FileKey::KeePass2XMLv2 && key->type() != FileKey::Hashed + && !config()->get(Config::Messages_NoLegacyKeyFileWarning).toBool()) { QMessageBox legacyWarning; - legacyWarning.setWindowTitle(tr("Legacy key file format")); - legacyWarning.setText(tr("You are using a legacy key file format which may become\n" - "unsupported in the future.\n\n" - "Please consider generating a new key file.")); + legacyWarning.setWindowTitle(tr("Old key file format")); + legacyWarning.setText(tr("You are using an old key file format which KeePassXC may
" + "stop supporting in the future.

" + "Please consider generating a new key file by going to:
" + "Database / Database Security / Change Key File.
")); legacyWarning.setIcon(QMessageBox::Icon::Warning); legacyWarning.addButton(QMessageBox::Ok); legacyWarning.setDefaultButton(QMessageBox::Ok); @@ -356,7 +358,7 @@ void DatabaseOpenWidget::reject() void DatabaseOpenWidget::browseKeyFile() { - QString filters = QString("%1 (*);;%2 (*.key)").arg(tr("All files"), tr("Key files")); + QString filters = QString("%1 (*);;%2 (*.keyx; *.key)").arg(tr("All files"), tr("Key files")); if (!config()->get(Config::RememberLastKeyFiles).toBool()) { fileDialog()->setNextForgetDialog(); } diff --git a/src/gui/databasekey/KeyFileEditWidget.cpp b/src/gui/databasekey/KeyFileEditWidget.cpp index e0486ae07..2fb0b3de2 100644 --- a/src/gui/databasekey/KeyFileEditWidget.cpp +++ b/src/gui/databasekey/KeyFileEditWidget.cpp @@ -47,12 +47,12 @@ bool KeyFileEditWidget::addToCompositeKey(QSharedPointer key) return false; } - if (fileKey->type() != FileKey::Hashed) { + if (fileKey->type() != FileKey::KeePass2XMLv2 && fileKey->type() != FileKey::Hashed) { QMessageBox::warning(getMainWindow(), - tr("Legacy key file format"), - tr("You are using a legacy key file format which may become\n" - "unsupported in the future.\n\n" - "Generate a new key file in the database security settings."), + tr("Old key file format"), + tr("You selected a key file in an old format which KeePassXC
" + "may stop supporting in the future.

" + "Please consider generating a new key file instead."), QMessageBox::Ok); } @@ -96,7 +96,7 @@ void KeyFileEditWidget::createKeyFile() if (!m_compEditWidget) { return; } - QString filters = QString("%1 (*.key);;%2 (*)").arg(tr("Key files"), tr("All files")); + QString filters = QString("%1 (*.keyx; *.key);;%2 (*)").arg(tr("Key files"), tr("All files")); QString fileName = fileDialog()->getSaveFileName(this, tr("Create Key File..."), QString(), filters); if (!fileName.isEmpty()) { @@ -119,7 +119,7 @@ void KeyFileEditWidget::browseKeyFile() if (!m_compEditWidget) { return; } - QString filters = QString("%1 (*.key);;%2 (*)").arg(tr("Key files"), tr("All files")); + QString filters = QString("%1 (*.keyx; *.key);;%2 (*)").arg(tr("Key files"), tr("All files")); QString fileName = fileDialog()->getOpenFileName(this, tr("Select a key file"), QString(), filters); if (QFileInfo(fileName).canonicalFilePath() == m_parent->getDatabase()->canonicalFilePath()) { diff --git a/src/keys/FileKey.cpp b/src/keys/FileKey.cpp index 6142d58d7..2ac52ae69 100644 --- a/src/keys/FileKey.cpp +++ b/src/keys/FileKey.cpp @@ -64,9 +64,10 @@ FileKey::~FileKey() * removed in a future version. * * @param device input device + * @param errorMsg error message in case of fatal failure * @return true if key file was loaded successfully */ -bool FileKey::load(QIODevice* device) +bool FileKey::load(QIODevice* device, QString* errorMsg) { m_type = None; @@ -75,32 +76,33 @@ bool FileKey::load(QIODevice* device) return false; } - if (device->size() == 0) { + if (device->size() == 0 || !device->reset()) { return false; } - // try different legacy key file formats - if (!device->reset()) { - return false; - } - if (loadXml(device)) { - m_type = KeePass2XML; + // load XML key file v1 or v2 + QString xmlError; + if (loadXml(device, &xmlError)) { return true; } - if (!device->reset()) { + if (!device->reset() || !xmlError.isEmpty()) { + if (errorMsg) { + *errorMsg = xmlError; + } return false; } + + // try legacy key file formats if (loadBinary(device)) { - m_type = FixedBinary; return true; } if (!device->reset()) { return false; } + if (loadHex(device)) { - m_type = FixedBinaryHex; return true; } @@ -109,7 +111,6 @@ bool FileKey::load(QIODevice* device) return false; } if (loadHashed(device)) { - m_type = Hashed; return true; } @@ -145,10 +146,14 @@ bool FileKey::load(const QString& fileName, QString* errorMsg) } return false; } - bool result = load(&file); + bool result = load(&file, errorMsg); file.close(); + if (errorMsg && !errorMsg->isEmpty()) { + return false; + } + if (file.error()) { result = false; if (errorMsg) { @@ -171,16 +176,64 @@ QByteArray FileKey::rawKey() const } /** - * Generate a new key file from random bytes. + * Generate a new key file with random bytes. * * @param device output device * @param number of random bytes to generate */ -void FileKey::create(QIODevice* device, int size) +void FileKey::createRandom(QIODevice* device, int size) { device->write(randomGen()->randomArray(size)); } +/** + * Generate a new key file in the KeePass2 XML format v2. + * + * @param device output device + * @param number of random bytes to generate + */ +void FileKey::createXMLv2(QIODevice* device, int size) +{ + QXmlStreamWriter w(device); + w.setAutoFormatting(true); + w.setAutoFormattingIndent(4); + w.writeStartDocument(); + + w.writeStartElement("KeyFile"); + + w.writeStartElement("Meta"); + w.writeTextElement("Version", "2.0"); + w.writeEndElement(); + + w.writeStartElement("Key"); + w.writeStartElement("Data"); + + QByteArray key = randomGen()->randomArray(size); + CryptoHash hash(CryptoHash::Sha256); + hash.addData(key); + QByteArray result = hash.result().left(4); + key = key.toHex().toUpper(); + + w.writeAttribute("Hash", result.toHex().toUpper()); + w.writeCharacters("\n "); + for (int i = 0; i < key.size(); ++i) { + // Pretty-print hex value (not strictly necessary, but nicer to read and KeePass2 does it) + if (i != 0 && i % 32 == 0) { + w.writeCharacters("\n "); + } else if (i != 0 && i % 8 == 0) { + w.writeCharacters(" "); + } + w.writeCharacters(QChar(key[i])); + } + sodium_memzero(key.data(), static_cast(key.capacity())); + w.writeCharacters("\n "); + + w.writeEndElement(); + w.writeEndElement(); + + w.writeEndDocument(); +} + /** * Create a new key file from random bytes. * @@ -189,7 +242,7 @@ void FileKey::create(QIODevice* device, int size) * @param number of random bytes to generate * @return true on successful creation */ -bool FileKey::create(const QString& fileName, QString* errorMsg, int size) +bool FileKey::create(const QString& fileName, QString* errorMsg) { QFile file(fileName); if (!file.open(QFile::WriteOnly)) { @@ -198,7 +251,11 @@ bool FileKey::create(const QString& fileName, QString* errorMsg, int size) } return false; } - create(&file, size); + if (fileName.endsWith(".keyx")) { + createXMLv2(&file); + } else { + createRandom(&file); + } file.close(); file.setPermissions(QFile::ReadUser); @@ -218,87 +275,86 @@ bool FileKey::create(const QString& fileName, QString* errorMsg, int size) * * @param device input device * @return true on success - * @deprecated */ -bool FileKey::loadXml(QIODevice* device) +bool FileKey::loadXml(QIODevice* device, QString* errorMsg) { QXmlStreamReader xmlReader(device); - if (!xmlReader.error() && xmlReader.readNextStartElement()) { - if (xmlReader.name() != "KeyFile") { - return false; - } - } else { + if (xmlReader.error()) { + return false; + } + if (xmlReader.readNextStartElement() && xmlReader.name() != "KeyFile") { return false; } - bool correctMeta = false; - QByteArray data; + struct + { + QString version; + QByteArray hash; + QByteArray data; + } keyFileData; while (!xmlReader.error() && xmlReader.readNextStartElement()) { if (xmlReader.name() == "Meta") { - correctMeta = loadXmlMeta(xmlReader); + while (!xmlReader.error() && xmlReader.readNextStartElement()) { + if (xmlReader.name() == "Version") { + keyFileData.version = xmlReader.readElementText(); + if (keyFileData.version.startsWith("1.0")) { + m_type = KeePass2XML; + } else if (keyFileData.version == "2.0") { + m_type = KeePass2XMLv2; + } else { + if (errorMsg) { + *errorMsg = QObject::tr("Unsupported key file version: %1").arg(keyFileData.version); + } + return false; + } + } + } } else if (xmlReader.name() == "Key") { - data = loadXmlKey(xmlReader); + while (!xmlReader.error() && xmlReader.readNextStartElement()) { + if (xmlReader.name() == "Data") { + keyFileData.hash = QByteArray::fromHex(xmlReader.attributes().value("Hash").toLatin1()); + QByteArray rawData = xmlReader.readElementText().simplified().replace(" ", "").toLatin1(); + + if (keyFileData.version.startsWith("1.0") && Tools::isBase64(rawData)) { + keyFileData.data = QByteArray::fromBase64(rawData); + } else if (keyFileData.version == "2.0" && Tools::isHex(rawData)) { + keyFileData.data = QByteArray::fromHex(rawData); + + CryptoHash hash(CryptoHash::Sha256); + hash.addData(keyFileData.data); + QByteArray result = hash.result().left(4); + if (keyFileData.hash != result) { + if (errorMsg) { + *errorMsg = QObject::tr("Checksum mismatch! Key file may be corrupt."); + } + return false; + } + } else { + if (errorMsg) { + *errorMsg = QObject::tr("Unexpected key file data! Key file may be corrupt."); + } + return false; + } + + sodium_memzero(rawData.data(), static_cast(rawData.capacity())); + } + } } } bool ok = false; - if (!xmlReader.error() && correctMeta && !data.isEmpty()) { - std::memcpy(m_key, data.data(), std::min(SHA256_SIZE, data.size())); + if (!xmlReader.error() && !keyFileData.data.isEmpty()) { + std::memcpy(m_key, keyFileData.data.data(), std::min(SHA256_SIZE, keyFileData.data.size())); ok = true; } - sodium_memzero(data.data(), static_cast(data.capacity())); + sodium_memzero(keyFileData.data.data(), static_cast(keyFileData.data.capacity())); return ok; } -/** - * Load meta data from legacy KeePass 2 XML key file. - * - * @param xmlReader input XML reader - * @return true on success - * @deprecated - */ -bool FileKey::loadXmlMeta(QXmlStreamReader& xmlReader) -{ - bool correctVersion = false; - - while (!xmlReader.error() && xmlReader.readNextStartElement()) { - if (xmlReader.name() == "Version") { - if (xmlReader.readElementText() == "1.00") { - correctVersion = true; - } - } - } - - return correctVersion; -} - -/** - * Load base64 key data from legacy KeePass 2 XML key file. - * - * @param xmlReader input XML reader - * @return true on success - * @deprecated - */ -QByteArray FileKey::loadXmlKey(QXmlStreamReader& xmlReader) -{ - QByteArray data; - - while (!xmlReader.error() && xmlReader.readNextStartElement()) { - if (xmlReader.name() == "Data") { - QByteArray rawData = xmlReader.readElementText().toLatin1(); - if (Tools::isBase64(rawData)) { - data = QByteArray::fromBase64(rawData); - } - } - } - - return data; -} - /** * Load fixed 32-bit binary key file. * @@ -315,11 +371,12 @@ bool FileKey::loadBinary(QIODevice* device) QByteArray data; if (!Tools::readAllFromDevice(device, data) || data.size() != 32) { return false; - } else { - std::memcpy(m_key, data.data(), std::min(SHA256_SIZE, data.size())); - sodium_memzero(data.data(), static_cast(data.capacity())); - return true; } + + std::memcpy(m_key, data.data(), std::min(SHA256_SIZE, data.size())); + sodium_memzero(data.data(), static_cast(data.capacity())); + m_type = FixedBinary; + return true; } /** @@ -354,6 +411,7 @@ bool FileKey::loadHex(QIODevice* device) std::memcpy(m_key, key.data(), std::min(SHA256_SIZE, key.size())); sodium_memzero(key.data(), static_cast(key.capacity())); + m_type = FixedBinaryHex; return true; } @@ -379,6 +437,7 @@ bool FileKey::loadHashed(QIODevice* device) std::memcpy(m_key, result.data(), std::min(SHA256_SIZE, result.size())); sodium_memzero(result.data(), static_cast(result.capacity())); + m_type = Hashed; return true; } diff --git a/src/keys/FileKey.h b/src/keys/FileKey.h index 290a04af0..540dde7d2 100644 --- a/src/keys/FileKey.h +++ b/src/keys/FileKey.h @@ -35,25 +35,25 @@ public: None, Hashed, KeePass2XML, + KeePass2XMLv2, FixedBinary, FixedBinaryHex }; FileKey(); ~FileKey() override; - bool load(QIODevice* device); + bool load(QIODevice* device, QString* errorMsg = nullptr); bool load(const QString& fileName, QString* errorMsg = nullptr); QByteArray rawKey() const override; Type type() const; - static void create(QIODevice* device, int size = 128); - static bool create(const QString& fileName, QString* errorMsg = nullptr, int size = 128); + static void createRandom(QIODevice* device, int size = 128); + static void createXMLv2(QIODevice* device, int size = 32); + static bool create(const QString& fileName, QString* errorMsg = nullptr); private: static constexpr int SHA256_SIZE = 32; - bool loadXml(QIODevice* device); - bool loadXmlMeta(QXmlStreamReader& xmlReader); - QByteArray loadXmlKey(QXmlStreamReader& xmlReader); + bool loadXml(QIODevice* device, QString* errorMsg = nullptr); bool loadBinary(QIODevice* device); bool loadHex(QIODevice* device); bool loadHashed(QIODevice* device); diff --git a/tests/TestKeys.cpp b/tests/TestKeys.cpp index d25a2bca8..cbbaae398 100644 --- a/tests/TestKeys.cpp +++ b/tests/TestKeys.cpp @@ -69,22 +69,36 @@ void TestKeys::testComposite() void TestKeys::testFileKey() { QFETCH(FileKey::Type, type); - QFETCH(QString, typeString); + QFETCH(QString, keyExt); + QFETCH(bool, fileKeyOk); - QString name = QString("FileKey").append(typeString); + QString name = QString("FileKey").append(QTest::currentDataTag()); KeePass2Reader reader; QString dbFilename = QString("%1/%2.kdbx").arg(QString(KEEPASSX_TEST_DATA_DIR), name); - QString keyFilename = QString("%1/%2.key").arg(QString(KEEPASSX_TEST_DATA_DIR), name); + QString keyFilename = QString("%1/%2.%3").arg(QString(KEEPASSX_TEST_DATA_DIR), name, keyExt); auto compositeKey = QSharedPointer::create(); auto fileKey = QSharedPointer::create(); - QVERIFY(fileKey->load(keyFilename)); - QCOMPARE(fileKey->rawKey().size(), 32); - + QString error; + QVERIFY(fileKey->load(keyFilename, &error) == fileKeyOk); + QVERIFY(error.isEmpty() == fileKeyOk); QCOMPARE(fileKey->type(), type); + // Test for same behaviour on code path without error parameter + auto fileKeyNoErrorParam = QSharedPointer::create(); + QVERIFY(fileKeyNoErrorParam->load(keyFilename) == fileKeyOk); + QCOMPARE(fileKeyNoErrorParam->type(), type); + + QCOMPARE(fileKey->rawKey(), fileKeyNoErrorParam->rawKey()); + + if (!fileKeyOk) { + return; + } + + QCOMPARE(fileKey->rawKey().size(), 32); + compositeKey->addKey(fileKey); auto db = QSharedPointer::create(); @@ -97,12 +111,16 @@ void TestKeys::testFileKey() void TestKeys::testFileKey_data() { QTest::addColumn("type"); - QTest::addColumn("typeString"); - QTest::newRow("Xml") << FileKey::KeePass2XML << QString("Xml"); - QTest::newRow("XmlBrokenBase64") << FileKey::Hashed << QString("XmlBrokenBase64"); - QTest::newRow("Binary") << FileKey::FixedBinary << QString("Binary"); - QTest::newRow("Hex") << FileKey::FixedBinaryHex << QString("Hex"); - QTest::newRow("Hashed") << FileKey::Hashed << QString("Hashed"); + QTest::addColumn("keyExt"); + QTest::addColumn("fileKeyOk"); + QTest::newRow("Xml") << FileKey::KeePass2XML << QString("key") << true; + QTest::newRow("XmlBrokenBase64") << FileKey::KeePass2XML << QString("key") << false; + QTest::newRow("XmlV2") << FileKey::KeePass2XMLv2 << QString("keyx") << true; + QTest::newRow("XmlV2HashFail") << FileKey::KeePass2XMLv2 << QString("keyx") << false; + QTest::newRow("XmlV2BrokenHex") << FileKey::KeePass2XMLv2 << QString("keyx") << false; + QTest::newRow("Binary") << FileKey::FixedBinary << QString("key") << true; + QTest::newRow("Hex") << FileKey::FixedBinaryHex << QString("key") << true; + QTest::newRow("Hashed") << FileKey::Hashed << QString("key") << true; } // clang-format on @@ -111,12 +129,12 @@ void TestKeys::testCreateFileKey() QBuffer keyBuffer1; keyBuffer1.open(QBuffer::ReadWrite); - FileKey::create(&keyBuffer1, 128); + FileKey::createRandom(&keyBuffer1, 128); QCOMPARE(keyBuffer1.size(), 128); QBuffer keyBuffer2; keyBuffer2.open(QBuffer::ReadWrite); - FileKey::create(&keyBuffer2, 64); + FileKey::createRandom(&keyBuffer2, 64); QCOMPARE(keyBuffer2.size(), 64); } @@ -127,7 +145,7 @@ void TestKeys::testCreateAndOpenFileKey() QBuffer keyBuffer; keyBuffer.open(QBuffer::ReadWrite); - FileKey::create(&keyBuffer); + FileKey::createRandom(&keyBuffer); keyBuffer.reset(); auto fileKey = QSharedPointer::create(); @@ -166,7 +184,7 @@ void TestKeys::testFileKeyHash() QBuffer keyBuffer; keyBuffer.open(QBuffer::ReadWrite); - FileKey::create(&keyBuffer); + FileKey::createRandom(&keyBuffer); CryptoHash cryptoHash(CryptoHash::Sha256); cryptoHash.addData(keyBuffer.data()); diff --git a/tests/data/FileKeyXmlV2.kdbx b/tests/data/FileKeyXmlV2.kdbx new file mode 100644 index 000000000..4b13c2ec1 Binary files /dev/null and b/tests/data/FileKeyXmlV2.kdbx differ diff --git a/tests/data/FileKeyXmlV2.keyx b/tests/data/FileKeyXmlV2.keyx new file mode 100644 index 000000000..ef38d4a89 --- /dev/null +++ b/tests/data/FileKeyXmlV2.keyx @@ -0,0 +1,12 @@ + + + + 2.0 + + + + A7007945 D07D54BA 28DF6434 1B4500FC + 9750DFB1 D36ADA2D 9C32DC19 4C7AB01B + + + \ No newline at end of file diff --git a/tests/data/FileKeyXmlV2BrokenHex.kdbx b/tests/data/FileKeyXmlV2BrokenHex.kdbx new file mode 100644 index 000000000..5a7e7355e Binary files /dev/null and b/tests/data/FileKeyXmlV2BrokenHex.kdbx differ diff --git a/tests/data/FileKeyXmlV2BrokenHex.keyx b/tests/data/FileKeyXmlV2BrokenHex.keyx new file mode 100644 index 000000000..a52803832 --- /dev/null +++ b/tests/data/FileKeyXmlV2BrokenHex.keyx @@ -0,0 +1,12 @@ + + + + 2.0 + + + + X7007945 D07D54BA 28DF6434 1B4500FC + 9750DFB1 D36ADA2D 9C32DC19 4C7AB01B + + + \ No newline at end of file diff --git a/tests/data/FileKeyXmlV2HashFail.kdbx b/tests/data/FileKeyXmlV2HashFail.kdbx new file mode 100644 index 000000000..9044b1288 Binary files /dev/null and b/tests/data/FileKeyXmlV2HashFail.kdbx differ diff --git a/tests/data/FileKeyXmlV2HashFail.keyx b/tests/data/FileKeyXmlV2HashFail.keyx new file mode 100644 index 000000000..a52e96f18 --- /dev/null +++ b/tests/data/FileKeyXmlV2HashFail.keyx @@ -0,0 +1,12 @@ + + + + 2.0 + + + + A7007945 D07D54BA 28DF6434 1B4500FC + 9750DFB1 D36ADA2D 9C32DC19 4C7AB01B + + + \ No newline at end of file