Refactor database extraction (#2698)

Previously, extracting the XML from a database was done with the
`saveXml` attribute in the `KeePass2Reader` class.

This had several unfortunate consequences:
* The `KdbxReader` class had to import the `KdbxXmlWriter` class
in order to perform the export (bad separation of concerns);
* The CLI database unlocking logic had to be duplicated only
for the `Extract` command;
* The `xmlData` had to be stored in the `KeePass2Reader` as
a temporary result.
* Lots of `setSaveXml` functions were implemented only
to trickle down this functionality.

Also, the naming of the `saveXml` variable was not really
helpful to understand it's role.

Overall, this change will make it easier to maintain and expand
the CLI database unlocking logic (for example, adding a `--no-password`
option as requested in https://github.com/keepassxreboot/keepassxc/issues/1873)
It also opens to door to other types of extraction/exporting (for
example exporting to CSV, as requested in
https://github.com/keepassxreboot/keepassxc/issues/2572)

src/cli/Extract.cpp

@@ -26,10 +26,6 @@
 #include "cli/TextStream.h"
 #include "cli/Utils.h"
 #include "core/Database.h"
-#include "format/KeePass2Reader.h"
-#include "keys/CompositeKey.h"
-#include "keys/FileKey.h"
-#include "keys/PasswordKey.h"
 
 Extract::Extract()
 {
@@ -60,66 +56,20 @@ int Extract::execute(const QStringList& arguments)
         return EXIT_FAILURE;
     }
 
-    if (!parser.isSet(Command::QuietOption)) {
+    auto db = Utils::unlockDatabase(args.at(0),
-        outputTextStream << QObject::tr("Insert password to unlock %1: ").arg(args.at(0)) << flush;
+                                    parser.value(Command::KeyFileOption),
-    }
+                                    parser.isSet(Command::QuietOption) ? Utils::DEVNULL : Utils::STDOUT,
-
+                                    Utils::STDERR);
-    auto compositeKey = QSharedPointer<CompositeKey>::create();
+    if (!db) {
-
-    QString line = Utils::getPassword(parser.isSet(Command::QuietOption) ? Utils::DEVNULL : Utils::STDOUT);
-    auto passwordKey = QSharedPointer<PasswordKey>::create();
-    passwordKey->setPassword(line);
-    compositeKey->addKey(passwordKey);
-
-    QString keyFilePath = parser.value(Command::KeyFileOption);
-    if (!keyFilePath.isEmpty()) {
-        // LCOV_EXCL_START
-        auto fileKey = QSharedPointer<FileKey>::create();
-        QString errorMsg;
-        if (!fileKey->load(keyFilePath, &errorMsg)) {
-            errorTextStream << QObject::tr("Failed to load key file %1: %2").arg(keyFilePath, errorMsg) << endl;
         return EXIT_FAILURE;
     }
 
-        if (fileKey->type() != FileKey::Hashed) {
+    QByteArray xmlData;
-            errorTextStream << QObject::tr("WARNING: You are using a legacy key file format which may become\n"
+    QString errorMessage;
-                                           "unsupported in the future.\n\n"
+    if (!db->extract(xmlData, &errorMessage)) {
-                                           "Please consider generating a new key file.")
+        errorTextStream << QObject::tr("Unable to extract database %1").arg(errorMessage) << endl;
-                            << endl;
-        }
-        // LCOV_EXCL_STOP
-
-        compositeKey->addKey(fileKey);
-    }
-
-    const QString& databaseFilename = args.at(0);
-    QFile dbFile(databaseFilename);
-    if (!dbFile.exists()) {
-        errorTextStream << QObject::tr("File %1 does not exist.").arg(databaseFilename) << endl;
         return EXIT_FAILURE;
     }
-    if (!dbFile.open(QIODevice::ReadOnly)) {
-        errorTextStream << QObject::tr("Unable to open file %1.").arg(databaseFilename) << endl;
-        return EXIT_FAILURE;
-    }
-
-    KeePass2Reader reader;
-    reader.setSaveXml(true);
-    auto db = QSharedPointer<Database>::create();
-    reader.readDatabase(&dbFile, compositeKey, db.data());
-
-    QByteArray xmlData = reader.reader()->xmlData();
-
-    if (reader.hasError()) {
-        if (xmlData.isEmpty()) {
-            errorTextStream << QObject::tr("Error while reading the database:\n%1").arg(reader.errorString()) << endl;
-        } else {
-            errorTextStream << QObject::tr("Error while parsing the database:\n%1").arg(reader.errorString()) << endl;
-        }
-        return EXIT_FAILURE;
-    }
-
     outputTextStream << xmlData.constData() << endl;
-
     return EXIT_SUCCESS;
 }

src/core/Database.cpp

@@ -281,7 +281,6 @@ bool Database::writeDatabase(QIODevice* device, QString* error)
     setEmitModified(true);
 
     if (writer.hasError()) {
-        // the writer failed
         if (error) {
             *error = writer.errorString();
         }
@@ -292,6 +291,20 @@ bool Database::writeDatabase(QIODevice* device, QString* error)
     return true;
 }
 
+bool Database::extract(QByteArray& xmlOutput, QString* error)
+{
+    KeePass2Writer writer;
+    writer.extractDatabase(this, xmlOutput);
+    if (writer.hasError()) {
+        if (error) {
+            *error = writer.errorString();
+        }
+        return false;
+    }
+
+    return true;
+}
+
 /**
  * Remove the old backup and replace it with a new one
  * backups are named <filename>.old.kdbx

src/core/Database.h

@@ -72,6 +72,7 @@ public:
               bool readOnly = false);
     bool save(QString* error = nullptr, bool atomic = true, bool backup = false);
     bool save(const QString& filePath, QString* error = nullptr, bool atomic = true, bool backup = false);
+    bool extract(QByteArray&, QString* error = nullptr);
 
     bool isInitialized() const;
     void setInitialized(bool initialized);

src/format/Kdbx3Writer.cpp

@@ -63,7 +63,7 @@ bool Kdbx3Writer::writeDatabase(QIODevice* device, Database* db)
     QBuffer header;
     header.open(QIODevice::WriteOnly);
 
-    writeMagicNumbers(&header, KeePass2::SIGNATURE_1, KeePass2::SIGNATURE_2, KeePass2::FILE_VERSION_3_1);
+    writeMagicNumbers(&header, KeePass2::SIGNATURE_1, KeePass2::SIGNATURE_2, formatVersion());
 
     CHECK_RETURN_FALSE(writeHeaderField<quint16>(&header, KeePass2::HeaderFieldID::CipherID, db->cipher().toRfc4122()));
     CHECK_RETURN_FALSE(
@@ -133,7 +133,7 @@ bool Kdbx3Writer::writeDatabase(QIODevice* device, Database* db)
         return false;
     }
 
-    KdbxXmlWriter xmlWriter(KeePass2::FILE_VERSION_3_1);
+    KdbxXmlWriter xmlWriter(formatVersion());
     xmlWriter.writeDatabase(outputDevice, db, &randomStream, headerHash);
 
     // Explicitly close/reset streams so they are flushed and we can detect
@@ -157,3 +157,8 @@ bool Kdbx3Writer::writeDatabase(QIODevice* device, Database* db)
 
     return true;
 }
+
+quint32 Kdbx3Writer::formatVersion()
+{
+    return KeePass2::FILE_VERSION_3_1;
+}

src/format/Kdbx3Writer.h

@@ -29,6 +29,7 @@ class Kdbx3Writer : public KdbxWriter
 
 public:
     bool writeDatabase(QIODevice* device, Database* db) override;
+    quint32 formatVersion() override;
 };
 
 #endif // KEEPASSX_KDBX3WRITER_H

src/format/Kdbx4Writer.cpp

@@ -70,7 +70,7 @@ bool Kdbx4Writer::writeDatabase(QIODevice* device, Database* db)
         QBuffer header;
         header.open(QIODevice::WriteOnly);
 
-        writeMagicNumbers(&header, KeePass2::SIGNATURE_1, KeePass2::SIGNATURE_2, KeePass2::FILE_VERSION_4);
+        writeMagicNumbers(&header, KeePass2::SIGNATURE_1, KeePass2::SIGNATURE_2, formatVersion());
 
         CHECK_RETURN_FALSE(
             writeHeaderField<quint32>(&header, KeePass2::HeaderFieldID::CipherID, db->cipher().toRfc4122()));
@@ -171,7 +171,7 @@ bool Kdbx4Writer::writeDatabase(QIODevice* device, Database* db)
         return false;
     }
 
-    KdbxXmlWriter xmlWriter(KeePass2::FILE_VERSION_4);
+    KdbxXmlWriter xmlWriter(formatVersion());
     xmlWriter.writeDatabase(outputDevice, db, &randomStream, headerHash);
 
     // Explicitly close/reset streams so they are flushed and we can detect
@@ -311,3 +311,8 @@ bool Kdbx4Writer::serializeVariantMap(const QVariantMap& map, QByteArray& output
     CHECK_RETURN_FALSE(buf.write(endBytes) == 1);
     return true;
 }
+
+quint32 Kdbx4Writer::formatVersion()
+{
+    return KeePass2::FILE_VERSION_4;
+}

src/format/Kdbx4Writer.h

@@ -29,6 +29,7 @@ class Kdbx4Writer : public KdbxWriter
 
 public:
     bool writeDatabase(QIODevice* device, Database* db) override;
+    quint32 formatVersion() override;
 
 private:
     bool writeInnerHeaderField(QIODevice* device, KeePass2::InnerHeaderFieldID fieldId, const QByteArray& data);

src/format/KdbxReader.cpp

@@ -20,7 +20,6 @@
 #include "KdbxReader.h"
 #include "core/Database.h"
 #include "core/Endian.h"
-#include "format/KdbxXmlWriter.h"
 
 #include <QBuffer>
 
@@ -67,7 +66,6 @@ bool KdbxReader::readDatabase(QIODevice* device, QSharedPointer<const CompositeK
     device->seek(0);
 
     m_db = db;
-    m_xmlData.clear();
     m_masterSeed.clear();
     m_encryptionIV.clear();
     m_streamStartBytes.clear();
@@ -97,14 +95,7 @@ bool KdbxReader::readDatabase(QIODevice* device, QSharedPointer<const CompositeK
     }
 
     // read payload
-    bool ok = readDatabaseImpl(device, headerStream.storedData(), std::move(key), db);
+    return readDatabaseImpl(device, headerStream.storedData(), std::move(key), db);
-
-    if (saveXml()) {
-        m_xmlData.clear();
-        decryptXmlInnerStream(m_xmlData, db);
-    }
-
-    return ok;
 }
 
 bool KdbxReader::hasError() const
@@ -117,21 +108,6 @@ QString KdbxReader::errorString() const
     return m_errorStr;
 }
 
-bool KdbxReader::saveXml() const
-{
-    return m_saveXml;
-}
-
-void KdbxReader::setSaveXml(bool save)
-{
-    m_saveXml = save;
-}
-
-QByteArray KdbxReader::xmlData() const
-{
-    return m_xmlData;
-}
-
 KeePass2::ProtectedStreamAlgo KdbxReader::protectedStreamAlgo() const
 {
     return m_irsAlgo;
@@ -270,23 +246,6 @@ void KdbxReader::setInnerRandomStreamID(const QByteArray& data)
     m_irsAlgo = irsAlgo;
 }
 
-/**
- * Decrypt protected inner stream fields in XML dump on demand.
- * Without the stream key from the KDBX header, the values become worthless.
- *
- * @param xmlOutput XML dump with decrypted fields
- * @param db the database object for which to generate the decrypted XML dump
- */
-void KdbxReader::decryptXmlInnerStream(QByteArray& xmlOutput, Database* db) const
-{
-    QBuffer buffer;
-    buffer.setBuffer(&xmlOutput);
-    buffer.open(QIODevice::WriteOnly);
-    KdbxXmlWriter writer(m_kdbxVersion);
-    writer.disableInnerStreamProtection(true);
-    writer.writeDatabase(&buffer, db);
-}
-
 /**
  * Raise an error. Use in case of an unexpected read error.
  *

src/format/KdbxReader.h

@@ -45,9 +45,6 @@ public:
     bool hasError() const;
     QString errorString() const;
 
-    bool saveXml() const;
-    void setSaveXml(bool save);
-    QByteArray xmlData() const;
     KeePass2::ProtectedStreamAlgo protectedStreamAlgo() const;
 
 protected:
@@ -86,8 +83,6 @@ protected:
 
     void raiseError(const QString& errorMessage);
 
-    void decryptXmlInnerStream(QByteArray& xmlOutput, Database* db) const;
-
     quint32 m_kdbxVersion = 0;
 
     QByteArray m_masterSeed;
@@ -96,13 +91,10 @@ protected:
     QByteArray m_protectedStreamKey;
     KeePass2::ProtectedStreamAlgo m_irsAlgo = KeePass2::ProtectedStreamAlgo::InvalidProtectedStreamAlgo;
 
-    QByteArray m_xmlData;
-
 private:
     QPair<quint32, quint32> m_kdbxSignature;
     QPointer<Database> m_db;
 
-    bool m_saveXml = false;
     bool m_error = false;
     QString m_errorStr = "";
 };

src/format/KdbxWriter.cpp

@@ -17,6 +17,10 @@
 
 #include "KdbxWriter.h"
 
+#include <QBuffer>
+
+#include "format/KdbxXmlWriter.h"
+
 bool KdbxWriter::hasError() const
 {
     return m_error;
@@ -62,6 +66,16 @@ bool KdbxWriter::writeData(QIODevice* device, const QByteArray& data)
     return true;
 }
 
+void KdbxWriter::extractDatabase(QByteArray& xmlOutput, Database* db)
+{
+    QBuffer buffer;
+    buffer.setBuffer(&xmlOutput);
+    buffer.open(QIODevice::WriteOnly);
+    KdbxXmlWriter writer(formatVersion());
+    writer.disableInnerStreamProtection(true);
+    writer.writeDatabase(&buffer, db);
+}
+
 /**
  * Raise an error. Use in case of an unexpected write error.
  *

src/format/KdbxWriter.h

@@ -52,6 +52,13 @@ public:
      */
     virtual bool writeDatabase(QIODevice* device, Database* db) = 0;
 
+    /**
+     * Get the database format version for the writer.
+     */
+    virtual quint32 formatVersion() = 0;
+
+    void extractDatabase(QByteArray& xmlOutput, Database* db);
+
     bool hasError() const;
     QString errorString() const;
 

src/format/KeePass2Reader.cpp

@@ -96,7 +96,6 @@ bool KeePass2Reader::readDatabase(QIODevice* device, QSharedPointer<const Compos
         m_reader.reset(new Kdbx4Reader());
     }
 
-    m_reader->setSaveXml(m_saveXml);
     return m_reader->readDatabase(device, std::move(key), db);
 }
 
@@ -110,16 +109,6 @@ QString KeePass2Reader::errorString() const
     return !m_reader.isNull() ? m_reader->errorString() : m_errorStr;
 }
 
-bool KeePass2Reader::saveXml() const
-{
-    return m_saveXml;
-}
-
-void KeePass2Reader::setSaveXml(bool save)
-{
-    m_saveXml = save;
-}
-
 /**
  * @return detected KDBX version
  */

src/format/KeePass2Reader.h

@@ -41,16 +41,12 @@ public:
     bool hasError() const;
     QString errorString() const;
 
-    bool saveXml() const;
-    void setSaveXml(bool save);
-
     QSharedPointer<KdbxReader> reader() const;
     quint32 version() const;
 
 private:
     void raiseError(const QString& errorMessage);
 
-    bool m_saveXml = false;
     bool m_error = false;
     QString m_errorStr = "";
 

src/format/KeePass2Writer.cpp

@@ -84,7 +84,6 @@ bool KeePass2Writer::implicitUpgradeNeeded(Database const* db) const
  * @param db source database
  * @return true on success
  */
-
 bool KeePass2Writer::writeDatabase(QIODevice* device, Database* db)
 {
     m_error = false;
@@ -109,6 +108,22 @@ bool KeePass2Writer::writeDatabase(QIODevice* device, Database* db)
     return m_writer->writeDatabase(device, db);
 }
 
+void KeePass2Writer::extractDatabase(Database* db, QByteArray& xmlOutput)
+{
+    m_error = false;
+    m_errorStr.clear();
+
+    if (db->kdf()->uuid() == KeePass2::KDF_AES_KDBX3) {
+        m_version = KeePass2::FILE_VERSION_3_1;
+        m_writer.reset(new Kdbx3Writer());
+    } else {
+        m_version = KeePass2::FILE_VERSION_4;
+        m_writer.reset(new Kdbx4Writer());
+    }
+
+    m_writer->extractDatabase(xmlOutput, db);
+}
+
 bool KeePass2Writer::hasError() const
 {
     return m_error || (m_writer && m_writer->hasError());

src/format/KeePass2Writer.h

@@ -33,6 +33,7 @@ class KeePass2Writer
 public:
     bool writeDatabase(const QString& filename, Database* db);
     bool writeDatabase(QIODevice* device, Database* db);
+    void extractDatabase(Database* db, QByteArray& xmlOutput);
 
     QSharedPointer<KdbxWriter> writer() const;
     quint32 version() const;