From b7373d6c3060817a0ecf7f4d9a06c8a9aa16548a Mon Sep 17 00:00:00 2001
From: Harald Sitter <sitter@kde.org>
Date: Thu, 8 Oct 2020 11:45:10 +0200
Subject: [PATCH] pass device names to the helper

paths are somewhat trivial to exploit. instead resolve them to the
actual block device names under /dev/ and pass that into the privileged
helper. the helper then only needs to verify that $name is in fact a
block device under /dev/.
since unprivileged processes cannot create files in /dev/ directly, let
alone block devices, this should give us a very reliable way of
preventing abuse.
---
 src/helper.cpp   | 68 ++++++++++++++++++++++++++++++++++++++----------
 src/smartctl.cpp | 11 +++++++-
 2 files changed, 64 insertions(+), 15 deletions(-)

diff --git a/src/helper.cpp b/src/helper.cpp
index 5418b25..5a9ce47 100644
--- a/src/helper.cpp
+++ b/src/helper.cpp
@@ -6,28 +6,68 @@
 #include <QDebug>
 #include <QProcess>
 #include <QFileInfo>
+#include <QScopeGuard>
 
-QString pathFrom(const QVariantMap &args)
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+#include <sys/types.h>
+#include <dirent.h>
+
+// Append name to /dev/ and ensure it is a trustable block device.
+static QString nameToPath(const QString &name)
 {
-    const auto devicePath = args.value(QStringLiteral("devicePath")).toString();
-    QFileInfo info(devicePath);
-    return info.absoluteFilePath();
+    if (name.isEmpty()) {
+        return {};
+    }
+
+    // This also excludes relative path shenanigans as they'd all need to contain a separator.
+    if (name.contains(QLatin1Char('/'))) {
+        qWarning() << "Device names must not contain slashes";
+        return {};
+    }
+
+    const QString path = QStringLiteral("/dev/%1").arg(name);
+
+    int blockFD = open(QFile::encodeName(path), O_PATH | O_NOFOLLOW);
+    auto blockFDClose = qScopeGuard([blockFD] { close(blockFD); });
+    if (blockFD == -1) {
+        const int err = errno;
+        qWarning() << "Failed to open block device" << name << strerror(err);
+        return {};
+    }
+
+    struct stat sb;
+    if (fstat(blockFD, &sb) == -1) {
+        const int err = errno;
+        qWarning() << "Failed to stat block device" << name << strerror(err);
+        return {};
+    }
+
+    if (!S_ISBLK(sb.st_mode)) {
+        qWarning() << "Device is not actually a block device" << name;
+        return {};
+    }
+
+    if (sb.st_uid != 0) {
+        qWarning() << "Device is not owned by root" << name;
+        return {};
+    }
+
+    return path;
 }
 
 ActionReply SMARTHelper::smartctl(const QVariantMap &args)
 {
-    // I may be better overall to also spin up solid on the root end and only allow
-    // UDIs as input. We can then assert expected input. Not sure it makes much
-    // of a difference though.
-    const QString devicePath = pathFrom(args);
-    if (devicePath.isEmpty() || !QFile::exists(devicePath)) {
-        qDebug() << "bad path";
+    // For security reasons we only accept fully resolved device names which
+    // we use to construct the final /dev/$name path.
+    const QString name = args.value(QStringLiteral("deviceName")).toString();
+    const QString devicePath = nameToPath(name);
+    if (devicePath.isEmpty()) {
         return ActionReply::HelperErrorReply();
     }
-    if (!devicePath.startsWith(QStringLiteral("/dev/"))) {
-        qDebug() << "unauthorized path";
-        return ActionReply::HelperErrorReply(KAuth::ActionReply::AuthorizationDeniedError);
-    }
 
     // PATH is super minimal when invoked through dbus
     setenv("PATH", "/usr/sbin:/sbin", 1);
diff --git a/src/smartctl.cpp b/src/smartctl.cpp
index b214fff..a8f66ce 100644
--- a/src/smartctl.cpp
+++ b/src/smartctl.cpp
@@ -4,6 +4,7 @@
 #include "smartctl.h"
 
 #include <QDebug>
+#include <QFileInfo>
 #include <KAuthAction>
 #include <KAuthExecuteJob>
 #include <KLocalizedString>
@@ -32,7 +33,15 @@ void SMARTCtl::run(const QString &devicePath)
                               devicePath) }
                         });
     action.setHelperId(QStringLiteral("org.kde.kded.smart"));
-    action.addArgument(QStringLiteral("devicePath"), devicePath);
+
+    // The helper only consumes names, ensure we fully resolve the name of the
+    // device to /dev/$name.
+    const QString canonicalDevicePath = QFileInfo(devicePath).canonicalFilePath();
+    Q_ASSERT(!canonicalDevicePath.isEmpty());
+    const QFileInfo canonicalDeviceInfo(canonicalDevicePath);
+    Q_ASSERT(canonicalDeviceInfo.absolutePath() == QLatin1String("/dev"));
+
+    action.addArgument(QStringLiteral("deviceName"), canonicalDeviceInfo.fileName());
     qCDebug(KDED) << action.isValid()
                   << action.hasHelper()
                   << action.helperId()
-- 
2.25.1