Commit 6d2a5fc8 authored by Harald Sitter's avatar Harald Sitter 💣

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.
parent 66efc179
......@@ -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);
......
......@@ -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()
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment