Merge pull request #1326 from keepassxreboot/feature/refactor-keyfile-generator

Generate random 128-byte stream instead of legacy XML format when creating key files

src/keys/FileKey.cpp

@@ -1,4 +1,5 @@
 /*
+*  Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
 *  Copyright (C) 2011 Felix Geyer <debfx@fobos.de>
 *
 *  This program is free software: you can redistribute it and/or modify
@@ -18,16 +19,30 @@
 #include "FileKey.h"
 
 #include <QFile>
-#include <QXmlStreamReader>
 
 #include "core/Tools.h"
 #include "crypto/CryptoHash.h"
 #include "crypto/Random.h"
 
-FileKey::FileKey()
+/**
-{
+ * Read key file from device while trying to detect its file format.
-}
+ *
-
+ * If no legacy key file format was detected, the SHA-256 hash of the
+ * key file will be used, allowing usage of arbitrary files as key files.
+ * In case of a detected legacy key file format, the raw byte contents
+ * will be extracted from the file.
+ *
+ * Supported legacy formats are:
+ *  - KeePass 2 XML key file
+ *  - Fixed 32 byte binary
+ *  - Fixed 32 byte ASCII hex-encoded binary
+ *
+ * Usage of legacy formats is discouraged and support for them may be
+ * removed in a future version.
+ *
+ * @param device input device
+ * @return true if key file was loaded successfully
+ */
 bool FileKey::load(QIODevice* device)
 {
     // we may need to read the file multiple times
@@ -39,8 +54,7 @@ bool FileKey::load(QIODevice* device)
         return false;
     }
 
-    // try different key file formats
+    // try different legacy key file formats
-
     if (!device->reset()) {
         return false;
     }
@@ -65,13 +79,31 @@ bool FileKey::load(QIODevice* device)
     if (!device->reset()) {
         return false;
     }
-    if (loadHashed(device)) {
+
-        return true;
+    // if no legacy format was detected, generate SHA-256 hash of key file
-    }
+    return loadHashed(device);
-
-    return false;
 }
 
+/**
+ * Load key file from path while trying to detect its file format.
+ *
+ * If no legacy key file format was detected, the SHA-256 hash of the
+ * key file will be used, allowing usage of arbitrary files as key files.
+ * In case of a detected legacy key file format, the raw byte contents
+ * will be extracted from the file.
+ *
+ * Supported legacy formats are:
+ *  - KeePass 2 XML key file
+ *  - Fixed 32 byte binary
+ *  - Fixed 32 byte ASCII hex-encoded binary
+ *
+ * Usage of legacy formats is discouraged and support for them may be
+ * removed in a future version.
+ *
+ * @param fileName input file name
+ * @param errorMsg error message if loading failed
+ * @return true if key file was loaded successfully
+ */
 bool FileKey::load(const QString& fileName, QString* errorMsg)
 {
     QFile file(fileName);
@@ -95,41 +127,42 @@ bool FileKey::load(const QString& fileName, QString* errorMsg)
     return result;
 }
 
+/**
+ * @return key data as bytes
+ */
 QByteArray FileKey::rawKey() const
 {
     return m_key;
 }
 
+/**
+ * @return cloned \link FileKey instance
+ */
 FileKey* FileKey::clone() const
 {
     return new FileKey(*this);
 }
 
-void FileKey::create(QIODevice* device)
+/**
+ * Generate a new key file from random bytes.
+ *
+ * @param device output device
+ * @param number of random bytes to generate
+ */
+void FileKey::create(QIODevice* device, int size)
 {
-    QXmlStreamWriter xmlWriter(device);
+    device->write(randomGen()->randomArray(size));
-
-    xmlWriter.writeStartDocument("1.0");
-
-    xmlWriter.writeStartElement("KeyFile");
-
-    xmlWriter.writeStartElement("Meta");
-
-    xmlWriter.writeTextElement("Version", "1.00");
-
-    xmlWriter.writeEndElement();
-
-    xmlWriter.writeStartElement("Key");
-
-    QByteArray data = randomGen()->randomArray(32);
-    xmlWriter.writeTextElement("Data", QString::fromLatin1(data.toBase64()));
-
-    xmlWriter.writeEndElement();
-
-    xmlWriter.writeEndDocument();
 }
 
-bool FileKey::create(const QString& fileName, QString* errorMsg)
+/**
+ * Create a new key file from random bytes.
+ *
+ * @param fileName output file name
+ * @param errorMsg error message if generation failed
+ * @param number of random bytes to generate
+ * @return true on successful creation
+ */
+bool FileKey::create(const QString& fileName, QString* errorMsg, int size)
 {
     QFile file(fileName);
     if (!file.open(QFile::WriteOnly)) {
@@ -138,8 +171,7 @@ bool FileKey::create(const QString& fileName, QString* errorMsg)
         }
         return false;
     }
-    create(&file);
+    create(&file, size);
-
     file.close();
 
     if (file.error()) {
@@ -149,11 +181,17 @@ bool FileKey::create(const QString& fileName, QString* errorMsg)
 
         return false;
     }
-    else {
+
     return true;
 }
-}
 
+/**
+ * Load key file in legacy KeePass 2 XML format.
+ *
+ * @param device input device
+ * @return true on success
+ * @deprecated
+ */
 bool FileKey::loadXml(QIODevice* device)
 {
     QXmlStreamReader xmlReader(device);
@@ -162,8 +200,7 @@ bool FileKey::loadXml(QIODevice* device)
         if (xmlReader.name() != "KeyFile") {
             return false;
         }
-    }
+    } else {
-    else {
         return false;
     }
 
@@ -173,8 +210,7 @@ bool FileKey::loadXml(QIODevice* device)
     while (!xmlReader.error() && xmlReader.readNextStartElement()) {
         if (xmlReader.name() == "Meta") {
             correctMeta = loadXmlMeta(xmlReader);
-        }
+        } else if (xmlReader.name() == "Key") {
-        else if (xmlReader.name() == "Key") {
             data = loadXmlKey(xmlReader);
         }
     }
@@ -183,18 +219,23 @@ bool FileKey::loadXml(QIODevice* device)
         m_key = data;
         return true;
     }
-    else {
+
     return false;
 }
-}
 
+/**
+ * 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") {
-            // TODO: error message about incompatible key file version
             if (xmlReader.readElementText() == "1.00") {
                 correctVersion = true;
             }
@@ -204,13 +245,19 @@ bool FileKey::loadXmlMeta(QXmlStreamReader& xmlReader)
     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") {
-            // TODO: do we need to enforce a specific data.size()?
             QByteArray rawData = xmlReader.readElementText().toLatin1();
             if (Tools::isBase64(rawData)) {
                 data = QByteArray::fromBase64(rawData);
@@ -221,6 +268,13 @@ QByteArray FileKey::loadXmlKey(QXmlStreamReader& xmlReader)
     return data;
 }
 
+/**
+ * Load fixed 32-bit binary key file.
+ *
+ * @param device input device
+ * @return true on success
+ * @deprecated
+ */
 bool FileKey::loadBinary(QIODevice* device)
 {
     if (device->size() != 32) {
@@ -230,13 +284,19 @@ bool FileKey::loadBinary(QIODevice* device)
     QByteArray data;
     if (!Tools::readAllFromDevice(device, data) || data.size() != 32) {
         return false;
-    }
+    } else {
-    else {
         m_key = data;
         return true;
     }
 }
 
+/**
+ * Load hex-encoded representation of fixed 32-bit binary key file.
+ *
+ * @param device input device
+ * @return true on success
+ * @deprecated
+ */
 bool FileKey::loadHex(QIODevice* device)
 {
     if (device->size() != 64) {
@@ -262,6 +322,12 @@ bool FileKey::loadHex(QIODevice* device)
     return true;
 }
 
+/**
+ * Generate SHA-256 hash of arbitrary text or binary key file.
+ *
+ * @param device input device
+ * @return true on success
+ */
 bool FileKey::loadHashed(QIODevice* device)
 {
     CryptoHash cryptoHash(CryptoHash::Sha256);
@@ -272,7 +338,8 @@ bool FileKey::loadHashed(QIODevice* device)
             return false;
         }
         cryptoHash.addData(buffer);
-    } while (!buffer.isEmpty());
+    }
+    while (!buffer.isEmpty());
 
     m_key = cryptoHash.result();
 

src/keys/FileKey.h

@@ -1,4 +1,5 @@
 /*
+*  Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
 *  Copyright (C) 2011 Felix Geyer <debfx@fobos.de>
 *
 *  This program is free software: you can redistribute it and/or modify
@@ -27,13 +28,12 @@ class QIODevice;
 class FileKey: public Key
 {
 public:
-    FileKey();
     bool load(QIODevice* device);
     bool load(const QString& fileName, QString* errorMsg = nullptr);
-    QByteArray rawKey() const;
+    QByteArray rawKey() const override;
-    FileKey* clone() const;
+    FileKey* clone() const override;
-    static void create(QIODevice* device);
+    static void create(QIODevice* device, int size = 128);
-    static bool create(const QString& fileName, QString* errorMsg = nullptr);
+    static bool create(const QString& fileName, QString* errorMsg = nullptr, int size = 128);
 
 private:
     bool loadXml(QIODevice* device);

tests/TestKeys.cpp

@@ -1,6 +1,6 @@
 /*
- *  Copyright (C) 2011 Felix Geyer <debfx@fobos.de>
  *  Copyright (C) 2017 KeePassXC Team <team@keepassxc.org>
+ *  Copyright (C) 2011 Felix Geyer <debfx@fobos.de>
  *
  *  This program is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -22,12 +22,14 @@
 #include <QTest>
 
 #include "config-keepassx-tests.h"
+
 #include "core/Database.h"
 #include "core/Metadata.h"
+#include "core/Tools.h"
 #include "crypto/Crypto.h"
+#include "crypto/CryptoHash.h"
 #include "format/KeePass2Reader.h"
 #include "format/KeePass2Writer.h"
-#include "keys/CompositeKey.h"
 #include "keys/FileKey.h"
 #include "keys/PasswordKey.h"
 
@@ -40,31 +42,27 @@ void TestKeys::initTestCase()
 
 void TestKeys::testComposite()
 {
-    CompositeKey* compositeKey1 = new CompositeKey();
+    QScopedPointer<CompositeKey> compositeKey1(new CompositeKey());
-    PasswordKey* passwordKey1 = new PasswordKey();
+    QScopedPointer<PasswordKey> passwordKey1(new PasswordKey());
-    PasswordKey* passwordKey2 = new PasswordKey("test");
+    QScopedPointer<PasswordKey> passwordKey2(new PasswordKey("test"));
     bool ok;
     QString errorString;
 
     // make sure that addKey() creates a copy of the keys
     compositeKey1->addKey(*passwordKey1);
     compositeKey1->addKey(*passwordKey2);
-    delete passwordKey1;
-    delete passwordKey2;
 
     QByteArray transformed = compositeKey1->transform(QByteArray(32, '\0'), 1, &ok, &errorString);
     QVERIFY(ok);
     QCOMPARE(transformed.size(), 32);
 
     // make sure the subkeys are copied
-    CompositeKey* compositeKey2 = compositeKey1->clone();
+    QScopedPointer<CompositeKey> compositeKey2(compositeKey1->clone());
-    delete compositeKey1;
     QCOMPARE(compositeKey2->transform(QByteArray(32, '\0'), 1, &ok, &errorString), transformed);
     QVERIFY(ok);
-    delete compositeKey2;
 
-    CompositeKey* compositeKey3 = new CompositeKey();
+    QScopedPointer<CompositeKey> compositeKey3(new CompositeKey());
-    CompositeKey* compositeKey4 = new CompositeKey();
+    QScopedPointer<CompositeKey> compositeKey4(new CompositeKey());
 
     // test clear()
     compositeKey3->addKey(PasswordKey("test"));
@@ -79,9 +77,6 @@ void TestKeys::testComposite()
     // test self-assignment
     *compositeKey3 = *compositeKey3;
     QCOMPARE(compositeKey3->rawKey(), compositeKey4->rawKey());
-
-    delete compositeKey3;
-    delete compositeKey4;
 }
 
 void TestKeys::testFileKey()
@@ -101,12 +96,10 @@ void TestKeys::testFileKey()
     QCOMPARE(fileKey.rawKey().size(), 32);
     compositeKey.addKey(fileKey);
 
-    Database* db = reader.readDatabase(dbFilename, compositeKey);
+    QScopedPointer<Database> db(reader.readDatabase(dbFilename, compositeKey));
     QVERIFY(db);
     QVERIFY(!reader.hasError());
     QCOMPARE(db->metadata()->name(), QString("%1 Database").arg(name));
-
-    delete db;
 }
 
 void TestKeys::testFileKey_data()
@@ -120,6 +113,20 @@ void TestKeys::testFileKey_data()
 }
 
 void TestKeys::testCreateFileKey()
+{
+    QBuffer keyBuffer1;
+    keyBuffer1.open(QBuffer::ReadWrite);
+
+    FileKey::create(&keyBuffer1, 128);
+    QCOMPARE(keyBuffer1.size(), 128);
+
+    QBuffer keyBuffer2;
+    keyBuffer2.open(QBuffer::ReadWrite);
+    FileKey::create(&keyBuffer2, 64);
+    QCOMPARE(keyBuffer2.size(), 64);
+}
+
+void TestKeys::testCreateAndOpenFileKey()
 {
     const QString dbName("testCreateFileKey database");
 
@@ -134,7 +141,7 @@ void TestKeys::testCreateFileKey()
     CompositeKey compositeKey;
     compositeKey.addKey(fileKey);
 
-    Database* dbOrg = new Database();
+    QScopedPointer<Database> dbOrg(new Database());
     QVERIFY(dbOrg->setKey(compositeKey));
     dbOrg->metadata()->setName(dbName);
 
@@ -142,16 +149,30 @@ void TestKeys::testCreateFileKey()
     dbBuffer.open(QBuffer::ReadWrite);
 
     KeePass2Writer writer;
-    writer.writeDatabase(&dbBuffer, dbOrg);
+    writer.writeDatabase(&dbBuffer, dbOrg.data());
     dbBuffer.reset();
-    delete dbOrg;
 
     KeePass2Reader reader;
-    Database* dbRead = reader.readDatabase(&dbBuffer, compositeKey);
+    QScopedPointer<Database> dbRead(reader.readDatabase(&dbBuffer, compositeKey));
     QVERIFY(dbRead);
     QVERIFY(!reader.hasError());
     QCOMPARE(dbRead->metadata()->name(), dbName);
-    delete dbRead;
+}
+
+void TestKeys::testFileKeyHash()
+{
+    QBuffer keyBuffer;
+    keyBuffer.open(QBuffer::ReadWrite);
+
+    FileKey::create(&keyBuffer);
+
+    CryptoHash cryptoHash(CryptoHash::Sha256);
+    cryptoHash.addData(keyBuffer.data());
+
+    FileKey fileKey;
+    fileKey.load(&keyBuffer);
+
+    QCOMPARE(fileKey.rawKey(), cryptoHash.result());
 }
 
 void TestKeys::testFileKeyError()

tests/TestKeys.h

@@ -31,6 +31,8 @@ private slots:
     void testFileKey();
     void testFileKey_data();
     void testCreateFileKey();
+    void testCreateAndOpenFileKey();
+    void testFileKeyHash();
     void testFileKeyError();
     void benchmarkTransformKey();
 };