Add support for version 2 XML key files.

As discussed in #4317, the next KeePass2 release will ship with
support for a new generation of XML key files which enable
hash integrity checks.

This patch adds support for reading and generating this new format.
By default, KeePass2 now uses the .keyx extension for generated
key files, which was added to KeePassXC's key generation file chooser
filter. We continue to generate hashed binary key files by default,
but the user can explicitly save the file with the new .keyx
extension to generate an XML v2 key file (currently undocumented).

When opening a database, the key file type is still determined
by content negotation, so the file extension has no impact here.

As an additional change, the legacy key file warnings have been
improved slightly to be less confusing and more helpful.

src/cli/Utils.cpp

@@ -132,9 +132,9 @@ namespace Utils
                 return {};
             }
 
-            if (fileKey->type() != FileKey::Hashed) {
+            if (fileKey->type() != FileKey::KeePass2XMLv2 && fileKey->type() != FileKey::Hashed) {
-                err << QObject::tr("WARNING: You are using a legacy key file format which may become\n"
+                err << QObject::tr("WARNING: You are using an old key file format which KeePassXC may\n"
-                                   "unsupported in the future.\n\n"
+                                   "stop supporting in the future.\n\n"
                                    "Please consider generating a new key file.")
                     << endl;
             }

src/gui/DatabaseOpenWidget.cpp

@@ -302,12 +302,14 @@ QSharedPointer<CompositeKey> 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.setWindowTitle(tr("Old key file format"));
-            legacyWarning.setText(tr("You are using a legacy key file format which may become\n"
+            legacyWarning.setText(tr("You are using an old key file format which KeePassXC may<br>"
-                                     "unsupported in the future.\n\n"
+                                     "stop supporting in the future.<br><br>"
-                                     "Please consider generating a new key file."));
+                                     "Please consider generating a new key file by going to:<br>"
+                                     "<strong>Database / Database Security / Change Key File.</strong><br>"));
             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();
     }

src/gui/databasekey/KeyFileEditWidget.cpp

@@ -47,12 +47,12 @@ bool KeyFileEditWidget::addToCompositeKey(QSharedPointer<CompositeKey> 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("Old key file format"),
-                             tr("You are using a legacy key file format which may become\n"
+                             tr("You selected a key file in an old format which KeePassXC<br>"
-                                "unsupported in the future.\n\n"
+                                "may stop supporting in the future.<br><br>"
-                                "Generate a new key file in the database security settings."),
+                                "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()) {

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
+    // load XML key file v1 or v2
-    if (!device->reset()) {
+    QString xmlError;
-        return false;
+    if (loadXml(device, &xmlError)) {
-    }
-    if (loadXml(device)) {
-        m_type = KeePass2XML;
         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<std::size_t>(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.error()) {
-        if (xmlReader.name() != "KeyFile") {
         return false;
     }
-    } else {
+    if (xmlReader.readNextStartElement() && xmlReader.name() != "KeyFile") {
         return false;
     }
 
-    bool correctMeta = false;
+    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<std::size_t>(rawData.capacity()));
+                }
+            }
         }
     }
 
     bool ok = false;
-    if (!xmlReader.error() && correctMeta && !data.isEmpty()) {
+    if (!xmlReader.error() && !keyFileData.data.isEmpty()) {
-        std::memcpy(m_key, data.data(), std::min(SHA256_SIZE, data.size()));
+        std::memcpy(m_key, keyFileData.data.data(), std::min(SHA256_SIZE, keyFileData.data.size()));
         ok = true;
     }
 
-    sodium_memzero(data.data(), static_cast<std::size_t>(data.capacity()));
+    sodium_memzero(keyFileData.data.data(), static_cast<std::size_t>(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,12 +371,13 @@ 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<std::size_t>(data.capacity()));
+    m_type = FixedBinary;
     return true;
 }
-}
 
 /**
  * Load hex-encoded representation of fixed 32-bit binary key file.
@@ -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<std::size_t>(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<std::size_t>(result.capacity()));
 
+    m_type = Hashed;
     return true;
 }
 

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 void createRandom(QIODevice* device, int size = 128);
-    static bool create(const QString& fileName, QString* errorMsg = nullptr, 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 loadXml(QIODevice* device, QString* errorMsg = nullptr);
-    bool loadXmlMeta(QXmlStreamReader& xmlReader);
-    QByteArray loadXmlKey(QXmlStreamReader& xmlReader);
     bool loadBinary(QIODevice* device);
     bool loadHex(QIODevice* device);
     bool loadHashed(QIODevice* device);

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<CompositeKey>::create();
     auto fileKey = QSharedPointer<FileKey>::create();
-    QVERIFY(fileKey->load(keyFilename));
+    QString error;
-    QCOMPARE(fileKey->rawKey().size(), 32);
+    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<FileKey>::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<Database>::create();
@@ -97,12 +111,16 @@ void TestKeys::testFileKey()
 void TestKeys::testFileKey_data()
 {
     QTest::addColumn<FileKey::Type>("type");
-    QTest::addColumn<QString>("typeString");
+    QTest::addColumn<QString>("keyExt");
-    QTest::newRow("Xml")             << FileKey::KeePass2XML    << QString("Xml");
+    QTest::addColumn<bool>("fileKeyOk");
-    QTest::newRow("XmlBrokenBase64") << FileKey::Hashed         << QString("XmlBrokenBase64");
+    QTest::newRow("Xml")             << FileKey::KeePass2XML    << QString("key")  << true;
-    QTest::newRow("Binary")          << FileKey::FixedBinary    << QString("Binary");
+    QTest::newRow("XmlBrokenBase64") << FileKey::KeePass2XML    << QString("key")  << false;
-    QTest::newRow("Hex")             << FileKey::FixedBinaryHex << QString("Hex");
+    QTest::newRow("XmlV2")           << FileKey::KeePass2XMLv2  << QString("keyx") << true;
-    QTest::newRow("Hashed")          << FileKey::Hashed         << QString("Hashed");
+    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<FileKey>::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());

tests/data/FileKeyXmlV2.keyx

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="utf-8"?>
+<KeyFile xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+	<Meta>
+		<Version>2.0</Version>
+	</Meta>
+	<Key>
+		<Data Hash="FE2949B8">
+			A7007945 D07D54BA 28DF6434 1B4500FC
+			9750DFB1 D36ADA2D 9C32DC19 4C7AB01B
+		</Data>
+	</Key>
+</KeyFile>

tests/data/FileKeyXmlV2BrokenHex.keyx

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="utf-8"?>
+<KeyFile xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+	<Meta>
+		<Version>2.0</Version>
+	</Meta>
+	<Key>
+		<Data Hash="FE2949B8">
+			X7007945 D07D54BA 28DF6434 1B4500FC
+			9750DFB1 D36ADA2D 9C32DC19 4C7AB01B
+		</Data>
+	</Key>
+</KeyFile>

tests/data/FileKeyXmlV2HashFail.keyx

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="utf-8"?>
+<KeyFile xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+	<Meta>
+		<Version>2.0</Version>
+	</Meta>
+	<Key>
+		<Data Hash="FE2949B9">
+			A7007945 D07D54BA 28DF6434 1B4500FC
+			9750DFB1 D36ADA2D 9C32DC19 4C7AB01B
+		</Data>
+	</Key>
+</KeyFile>