From b4bd63c1739900d94c04da03045e9445a5a5f54b Mon Sep 17 00:00:00 2001
From: Andre Heinecke <aheinecke@gnupg.org>
Date: Tue, 7 Jul 2020 14:39:29 +0200
Subject: [PATCH] Allow safe usage of query

To allow secure usage of query and search the parameters are
no longer parsed as value but instead of positional arguments.

This allows us to register "kleoptra --query -- $1" as an
URL handler for openpgp4fpr: without the risk of command
line injection through an unsescaped query string.

Similarly the double dash should be used for file handling
to avoid command line injection through filenames.
---
 src/kleopatra_options.h      | 19 ++++++++++++++-----
 src/kleopatraapplication.cpp | 25 ++++++++++++++-----------
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/src/kleopatra_options.h b/src/kleopatra_options.h
index 661c44d7..8ce7fccf 100644
--- a/src/kleopatra_options.h
+++ b/src/kleopatra_options.h
@@ -79,8 +79,7 @@ static void kleopatra_options(QCommandLineParser *parser)
                                   << QStringLiteral("D"),
                                   i18n("Decrypt and/or verify file(s)"))
             << QCommandLineOption(QStringList() << QStringLiteral("search"),
-                                  i18n("Search for a certificate on a keyserver"),
-                                  QStringLiteral("search string"))
+                                  i18n("Search for a certificate on a keyserver"))
             << QCommandLineOption(QStringList() << QStringLiteral("checksum"),
                                   i18n("Create or check a checksum file"))
             << QCommandLineOption(QStringList() << QStringLiteral("query")
@@ -88,8 +87,7 @@ static void kleopatra_options(QCommandLineParser *parser)
                                   i18nc("If a certificate is already known it shows the certificate details dialog."
                                         "Otherwise it brings up the certificate search dialog.",
                                         "Show details of a local certificate or search for it on a keyserver"
-                                        " by fingerprint"),
-                                  QStringLiteral("fingerprint"))
+                                        " by fingerprint"))
             << QCommandLineOption(QStringList() << QStringLiteral("gen-key"),
                                   i18n("Create a new key pair or certificate signing request"))
             << QCommandLineOption(QStringLiteral("parent-windowid"),
@@ -100,8 +98,19 @@ static void kleopatra_options(QCommandLineParser *parser)
 
     parser->addOptions(options);
 
+    /* Security note: To avoid code execution by shared library injection
+     * through e.g. -platformpluginpath any external input should be seperated
+     * by a double dash -- this is why query / search uses positional arguments.
+     *
+     * For example on Windows there is an URLhandler for openpgp4fpr:
+     * be opened with Kleopatra's query function. And while a browser should
+     * urlescape such a query there might be tricks to inject a quote character
+     * and as such inject command line options for Kleopatra in an URL. */
     parser->addPositionalArgument(QStringLiteral("files"),
                                   i18n("File(s) to process"),
-                                  QStringLiteral("[files..]"));
+                                  QStringLiteral("-- [files..]"));
+    parser->addPositionalArgument(QStringLiteral("query"),
+                                  i18n("String or Fingerprint for query and search"),
+                                  QStringLiteral("-- [query..]"));
 }
 #endif
diff --git a/src/kleopatraapplication.cpp b/src/kleopatraapplication.cpp
index 989f14b4..a8c5dd08 100644
--- a/src/kleopatraapplication.cpp
+++ b/src/kleopatraapplication.cpp
@@ -273,13 +273,18 @@ QString KleopatraApplication::newInstance(const QCommandLineParser &parser,
 
     QStringList files;
     const QDir cwd = QDir(workingDirectory);
-    Q_FOREACH (const QString &file, parser.positionalArguments()) {
-        // We do not check that file exists here. Better handle
-        // these errors in the UI.
-        if (QFileInfo(file).isAbsolute()) {
-            files << file;
-        } else {
-            files << cwd.absoluteFilePath(file);
+    bool queryMode = parser.isSet(QStringLiteral("query")) || parser.isSet(QStringLiteral("search"));
+
+    // Query and Search treat positional arguments differently, see below.
+    if (!queryMode) {
+        Q_FOREACH (const QString &file, parser.positionalArguments()) {
+            // We do not check that file exists here. Better handle
+            // these errors in the UI.
+            if (QFileInfo(file).isAbsolute()) {
+                files << file;
+            } else {
+                files << cwd.absoluteFilePath(file);
+            }
         }
     }
 
@@ -313,10 +318,8 @@ QString KleopatraApplication::newInstance(const QCommandLineParser &parser,
 
     // Handle openpgp4fpr URI scheme
     QString needle;
-    if (parser.isSet(QStringLiteral("search"))) {
-        needle = parser.value(QStringLiteral("search"));
-    } else if (parser.isSet(QStringLiteral("query"))) {
-        needle = parser.value(QStringLiteral("query"));
+    if (queryMode) {
+        needle = parser.positionalArguments().join(QLatin1Char(' '));
     }
     if (needle.startsWith(QLatin1String("openpgp4fpr:"))) {
         needle.remove(0, 12);
-- 
GitLab