From a7532d4041de7de6c7df7fbe1fb7fd0a50fcab96 Mon Sep 17 00:00:00 2001 From: Shubham Jangra Date: Sun, 19 May 2019 00:20:10 +0530 Subject: [PATCH] Scrap Public Key Cryptography code based on QCA as QDbus is secure enough Summary: QDbus already provides enough security to the calls made by the application to the helper. Hence no need to encrypt, sign the requests of the application and verify their integrity at the helper side. Reviewers: stikonas, cjlcarvalho Reviewed By: stikonas Subscribers: #kde_partition_manager Tags: #kde_partition_manager Differential Revision: https://phabricator.kde.org/D21275 --- src/util/externalcommand.cpp | 116 ++++------------------------ src/util/externalcommand.h | 10 +-- src/util/externalcommandhelper.cpp | 120 +++++------------------------ src/util/externalcommandhelper.h | 19 ++--- 4 files changed, 41 insertions(+), 224 deletions(-) diff --git a/src/util/externalcommand.cpp b/src/util/externalcommand.cpp index 4ea5112..360a5dc 100644 --- a/src/util/externalcommand.cpp +++ b/src/util/externalcommand.cpp @@ -42,8 +42,6 @@ #include #include -#include - #include #include #include @@ -61,8 +59,6 @@ struct ExternalCommandPrivate }; KAuth::ExecuteJob* ExternalCommand::m_job; -QCA::PrivateKey* ExternalCommand::privateKey; -QCA::Initializer* ExternalCommand::init; bool ExternalCommand::helperStarted = false; QWidget* ExternalCommand::parent; @@ -106,13 +102,16 @@ ExternalCommand::ExternalCommand(Report& report, const QString& cmd, const QStri ExternalCommand::~ExternalCommand() { + } -// void ExternalCommand::setup() -// { -// connect(this, qOverload(&QProcess::finished), this, &ExternalCommand::onFinished); -// connect(this, &ExternalCommand::readyReadStandardOutput, this, &ExternalCommand::onReadOutput); -// } +/* +void ExternalCommand::setup() +{ + connect(this, qOverload(&QProcess::finished), this, &ExternalCommand::onFinished); + connect(this, &ExternalCommand::readyReadStandardOutput, this, &ExternalCommand::onReadOutput); +} +*/ /** Executes the external command. @param timeout timeout to wait for the process to start @@ -146,19 +145,8 @@ bool ExternalCommand::start(int timeout) interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days bool rval = false; - QByteArray request; - const quint64 nonce = interface->getNonce(); - request.setNum(nonce); - request.append(cmd.toUtf8()); - for (const auto &argument : qAsConst(d->m_Args)) - request.append(argument.toUtf8()); - request.append(d->m_Input); - request.append(d->processChannelMode); - QByteArray hash = QCryptographicHash::hash(request, QCryptographicHash::Sha512); - - QDBusPendingCall pcall = interface->start(privateKey->signMessage(hash, QCA::EMSA3_Raw), - nonce, cmd, args(), d->m_Input, d->processChannelMode); + QDBusPendingCall pcall = interface->start(cmd, args(), d->m_Input, d->processChannelMode); QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this); QEventLoop loop; @@ -200,22 +188,9 @@ bool ExternalCommand::copyBlocks(const CopySource& source, CopyTarget& target) auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), QStringLiteral("/Helper"), QDBusConnection::systemBus(), this); interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days - QByteArray request; - const quint64 nonce = interface->getNonce(); - request.setNum(nonce); - request.append(source.path().toUtf8()); - request.append(QByteArray::number(source.firstByte())); - request.append(QByteArray::number(source.length())); - request.append(target.path().toUtf8()); - request.append(QByteArray::number(target.firstByte())); - request.append(QByteArray::number(blockSize)); - - QByteArray hash = QCryptographicHash::hash(request, QCryptographicHash::Sha512); - - QDBusPendingCall pcall = interface->copyblocks(privateKey->signMessage(hash, QCA::EMSA3_Raw), nonce, - source.path(), source.firstByte(), source.length(), - target.path(), target.firstByte(), blockSize); + QDBusPendingCall pcall = interface->copyblocks(source.path(), source.firstByte(), source.length(), + target.path(), target.firstByte(), blockSize); QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this); QEventLoop loop; @@ -257,18 +232,8 @@ bool ExternalCommand::writeData(Report& commandReport, const QByteArray& buffer, auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), QStringLiteral("/Helper"), QDBusConnection::systemBus(), this); interface->setTimeout(10 * 24 * 3600 * 1000); // 10 days - QByteArray request; - - const quint64 nonce = interface->getNonce(); - request.setNum(nonce); - request.append(buffer); - request.append(deviceNode.toUtf8()); - request.append(QByteArray::number(firstByte)); - - QByteArray hash = QCryptographicHash::hash(request, QCryptographicHash::Sha512); - - QDBusPendingCall pcall = interface->writeData(privateKey->signMessage(hash, QCA::EMSA3_Raw), nonce, - buffer, deviceNode, firstByte); + + QDBusPendingCall pcall = interface->writeData(buffer, deviceNode, firstByte); QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this); QEventLoop loop; @@ -389,33 +354,11 @@ bool ExternalCommand::startHelper() d->m_thread = new DBusThread; d->m_thread->start(); - init = new QCA::Initializer; - // Generate RSA key pair for signing external command requests - if (!QCA::isSupported("pkey") || !QCA::PKey::supportedIOTypes().contains(QCA::PKey::RSA)) { - qCritical() << xi18n("QCA does not support RSA."); - return false; - } - - privateKey = new QCA::PrivateKey; - *privateKey = QCA::KeyGenerator().createRSA(4096); - if(privateKey->isNull()) { - qCritical() << xi18n("Failed to make private RSA key."); - return false; - } - - if (!privateKey->canSign()) { - qCritical() << xi18n("Generated key cannot be used for signatures."); - return false; - } - - QCA::PublicKey pubkey = privateKey->toPublicKey(); - KAuth::Action action = KAuth::Action(QStringLiteral("org.kde.kpmcore.externalcommand.init")); action.setHelperId(QStringLiteral("org.kde.kpmcore.externalcommand")); action.setTimeout(10 * 24 * 3600 * 1000); // 10 days action.setParentWidget(parent); QVariantMap arguments; - arguments.insert(QStringLiteral("pubkey"), pubkey.toDER()); action.setArguments(arguments); m_job = action.execute(); m_job->start(); @@ -435,38 +378,9 @@ bool ExternalCommand::startHelper() void ExternalCommand::stopHelper() { auto *interface = new org::kde::kpmcore::externalcommand(QStringLiteral("org.kde.kpmcore.externalcommand"), - QStringLiteral("/Helper"), QDBusConnection::systemBus()); - QByteArray request; - const quint64 nonce = interface->getNonce(); - request.setNum(nonce); - QByteArray hash = QCryptographicHash::hash(request, QCryptographicHash::Sha512); - interface->exit(privateKey->signMessage(hash, QCA::EMSA3_Raw), nonce); + QStringLiteral("/Helper"), QDBusConnection::systemBus()); + interface->exit(); - delete privateKey; - delete init; -} - -quint64 ExternalCommand::getNonce(QDBusInterface& iface) -{ - QDBusPendingCall pcall = iface.asyncCall(QStringLiteral("getNonce")); - QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall); - QEventLoop loop; - quint64 rval = 0; - - auto exitLoop = [&] (QDBusPendingCallWatcher *watcher) { - loop.exit(); - - if (watcher->isError()) - qWarning() << watcher->error(); - else { - QDBusPendingReply reply = *watcher; - rval = reply; - } - }; - - connect(watcher, &QDBusPendingCallWatcher::finished, exitLoop); - loop.exec(); - return rval; } void DBusThread::run() diff --git a/src/util/externalcommand.h b/src/util/externalcommand.h index 305c166..2c3e390 100644 --- a/src/util/externalcommand.h +++ b/src/util/externalcommand.h @@ -31,13 +31,14 @@ #include -class KJob; namespace KAuth { class ExecuteJob; } -namespace QCA { class PrivateKey; class Initializer; } + +class KJob; class Report; class CopySource; class CopyTarget; class QDBusInterface; + struct ExternalCommandPrivate; class DBusThread : public QThread @@ -126,18 +127,13 @@ public Q_SLOTS: private: void setExitCode(int i); - void onReadOutput(); - static quint64 getNonce(QDBusInterface& iface); private: std::unique_ptr d; // KAuth - static quint64 m_Nonce; static KAuth::ExecuteJob *m_job; - static QCA::Initializer *init; - static QCA::PrivateKey *privateKey; static bool helperStarted; static QWidget *parent; }; diff --git a/src/util/externalcommandhelper.cpp b/src/util/externalcommandhelper.cpp index 4a918c5..6b35482 100644 --- a/src/util/externalcommandhelper.cpp +++ b/src/util/externalcommandhelper.cpp @@ -39,11 +39,12 @@ * * This helper also starts another DBus interface where it listens to * command execution requests from the application that started the helper. - * These requests are validated using public key cryptography, to prevent - * other unprivileged applications from gaining root privileges. + * */ ActionReply ExternalCommandHelper::init(const QVariantMap& args) { + Q_UNUSED(args) + ActionReply reply; if (!QDBusConnection::systemBus().isConnected() || !QDBusConnection::systemBus().registerService(QStringLiteral("org.kde.kpmcore.helperinterface")) || @@ -52,9 +53,7 @@ ActionReply ExternalCommandHelper::init(const QVariantMap& args) reply.addData(QStringLiteral("success"), false); return reply; } - - m_publicKey = QCA::PublicKey::fromDER(args[QStringLiteral("pubkey")].toByteArray()); - + m_loop = std::make_unique(); HelperSupport::progressStep(QVariantMap()); @@ -75,16 +74,6 @@ ActionReply ExternalCommandHelper::init(const QVariantMap& args) return reply; } -/** Generates cryptographic nonce - * @return nonce -*/ -quint64 ExternalCommandHelper::getNonce() -{ - const quint64 nonce = m_Generator.generate(); - m_Nonces.insert(nonce); - return nonce; -} - /** Reads the given number of bytes from the sourceDevice into the given buffer. @param sourceDevice device or file to read from @param buffer buffer to store the bytes read in @@ -145,35 +134,11 @@ bool ExternalCommandHelper::writeData(const QString &targetDevice, const QByteAr } // If targetDevice is empty then return QByteArray with data that was read from disk. -QVariantMap ExternalCommandHelper::copyblocks(const QByteArray& signature, const quint64 nonce, const QString& sourceDevice, const qint64 sourceFirstByte, const qint64 sourceLength, const QString& targetDevice, const qint64 targetFirstByte, const qint64 blockSize) +QVariantMap ExternalCommandHelper::copyblocks(const QString& sourceDevice, const qint64 sourceFirstByte, const qint64 sourceLength, const QString& targetDevice, const qint64 targetFirstByte, const qint64 blockSize) { QVariantMap reply; reply[QStringLiteral("success")] = true; - if (m_Nonces.find(nonce) != m_Nonces.end()) - m_Nonces.erase( nonce ); - else { - reply[QStringLiteral("success")] = false; - return reply; - } - - QByteArray request; - - request.setNum(nonce); - request.append(sourceDevice.toUtf8()); - request.append(QByteArray::number(sourceFirstByte)); - request.append(QByteArray::number(sourceLength)); - request.append(targetDevice.toUtf8()); - request.append(QByteArray::number(targetFirstByte)); - request.append(QByteArray::number(blockSize)); - - QByteArray hash = QCryptographicHash::hash(request, QCryptographicHash::Sha512); - if (!m_publicKey.verifyMessage(hash, signature, QCA::EMSA3_Raw)) { - qCritical() << xi18n("Invalid cryptographic signature"); - reply[QStringLiteral("success")] = false; - return reply; - } - const qint64 blocksToCopy = sourceLength / blockSize; qint64 readOffset = sourceFirstByte; qint64 writeOffset = targetFirstByte; @@ -258,65 +223,27 @@ QVariantMap ExternalCommandHelper::copyblocks(const QByteArray& signature, const return reply; } -bool ExternalCommandHelper::writeData(const QByteArray& signature, const quint64 nonce, const QByteArray& buffer, const QString& targetDevice, const qint64 targetFirstByte) +bool ExternalCommandHelper::writeData(const QByteArray& buffer, const QString& targetDevice, const qint64 targetFirstByte) { - if (m_Nonces.find(nonce) != m_Nonces.end()) - m_Nonces.erase( nonce ); - else - return false; - - QByteArray request; - request.setNum(nonce); - request.append(buffer); - request.append(targetDevice.toUtf8()); - request.append(QByteArray::number(targetFirstByte)); - // Do not allow using this helper for writing to arbitrary location if ( targetDevice.left(5) != QStringLiteral("/dev/") && !targetDevice.contains(QStringLiteral("/etc/fstab"))) return false; - QByteArray hash = QCryptographicHash::hash(request, QCryptographicHash::Sha512); - if (!m_publicKey.verifyMessage(hash, signature, QCA::EMSA3_Raw)) { - qCritical() << xi18n("Invalid cryptographic signature"); - return false; - } - return writeData(targetDevice, buffer, targetFirstByte); } -QVariantMap ExternalCommandHelper::start(const QByteArray& signature, const quint64 nonce, const QString& command, const QStringList& arguments, const QByteArray& input, const int processChannelMode) +QVariantMap ExternalCommandHelper::start(const QString& command, const QStringList& arguments, const QByteArray& input, const int processChannelMode) { QTextCodec::setCodecForLocale(QTextCodec::codecForName("UTF-8")); QVariantMap reply; reply[QStringLiteral("success")] = true; - if (m_Nonces.find(nonce) != m_Nonces.end()) - m_Nonces.erase( nonce ); - else { - reply[QStringLiteral("success")] = false; - return reply; - } - if (command.isEmpty()) { reply[QStringLiteral("success")] = false; return reply; } - QByteArray request; - request.setNum(nonce); - request.append(command.toUtf8()); - for (const auto &argument : arguments) - request.append(argument.toUtf8()); - request.append(input); - request.append(processChannelMode); - QByteArray hash = QCryptographicHash::hash(request, QCryptographicHash::Sha512); - if (!m_publicKey.verifyMessage(hash, signature, QCA::EMSA3_Raw)) { - qCritical() << xi18n("Invalid cryptographic signature"); - reply[QStringLiteral("success")] = false; - return reply; - } - // Compare with command whitelist QString basename = command.mid(command.lastIndexOf(QLatin1Char('/')) + 1); if (std::find(std::begin(allowedCommands), std::end(allowedCommands), basename) == std::end(allowedCommands)) { @@ -326,7 +253,7 @@ QVariantMap ExternalCommandHelper::start(const QByteArray& signature, const quin return reply; } -// connect(&cmd, &QProcess::readyReadStandardOutput, this, &ExternalCommandHelper::onReadOutput); +// connect(&cmd, &QProcess::readyReadStandardOutput, this, &ExternalCommandHelper::onReadOutput); m_cmd.setEnvironment( { QStringLiteral("LVM_SUPPRESS_FD_WARNINGS=1") } ); m_cmd.setProcessChannelMode(static_cast(processChannelMode)); @@ -341,19 +268,8 @@ QVariantMap ExternalCommandHelper::start(const QByteArray& signature, const quin return reply; } -void ExternalCommandHelper::exit(const QByteArray& signature, const quint64 nonce) +void ExternalCommandHelper::exit() { - QByteArray request; - if (m_Nonces.find(nonce) == m_Nonces.end()) - return; - - request.setNum(nonce); - QByteArray hash = QCryptographicHash::hash(request, QCryptographicHash::Sha512); - if (!m_publicKey.verifyMessage(hash, signature, QCA::EMSA3_Raw)) { - qCritical() << xi18n("Invalid cryptographic signature"); - return; - } - m_loop->exit(); QDBusConnection::systemBus().unregisterObject(QStringLiteral("/Helper")); @@ -362,18 +278,18 @@ void ExternalCommandHelper::exit(const QByteArray& signature, const quint64 nonc void ExternalCommandHelper::onReadOutput() { -// const QByteArray s = cmd.readAllStandardOutput(); +/* const QByteArray s = cmd.readAllStandardOutput(); -// if(output.length() > 10*1024*1024) { // prevent memory overflow for badly corrupted file systems -// if (report()) -// report()->line() << xi18nc("@info:status", "(Command is printing too much output)"); -// return; -// } + if(output.length() > 10*1024*1024) { // prevent memory overflow for badly corrupted file systems + if (report()) + report()->line() << xi18nc("@info:status", "(Command is printing too much output)"); + return; + } -// output += s; + output += s; -// if (report()) -// *report() << QString::fromLocal8Bit(s); + if (report()) + *report() << QString::fromLocal8Bit(s);*/ } KAUTH_HELPER_MAIN("org.kde.kpmcore.externalcommand", ExternalCommandHelper) diff --git a/src/util/externalcommandhelper.h b/src/util/externalcommandhelper.h index 1b0359d..6d7029b 100644 --- a/src/util/externalcommandhelper.h +++ b/src/util/externalcommandhelper.h @@ -24,12 +24,9 @@ #include #include -#include #include #include -#include - using namespace KAuth; class ExternalCommandHelper : public QObject @@ -47,23 +44,17 @@ public: public Q_SLOTS: ActionReply init(const QVariantMap& args); - Q_SCRIPTABLE quint64 getNonce(); - Q_SCRIPTABLE QVariantMap start(const QByteArray& signature, const quint64 nonce, const QString& command, const QStringList& arguments, const QByteArray& input, const int processChannelMode); - Q_SCRIPTABLE QVariantMap copyblocks(const QByteArray& signature, const quint64 nonce, const QString& sourceDevice, const qint64 sourceFirstByte, const qint64 sourceLength, const QString& targetDevice, const qint64 targetFirstByte, const qint64 blockSize); - Q_SCRIPTABLE bool writeData(const QByteArray& signature, const quint64 nonce, const QByteArray& buffer, const QString& targetDevice, const qint64 targetFirstByte); - Q_SCRIPTABLE void exit(const QByteArray& signature, const quint64 nonce); + Q_SCRIPTABLE QVariantMap start(const QString& command, const QStringList& arguments, const QByteArray& input, const int processChannelMode); + Q_SCRIPTABLE QVariantMap copyblocks(const QString& sourceDevice, const qint64 sourceFirstByte, const qint64 sourceLength, const QString& targetDevice, const qint64 targetFirstByte, const qint64 blockSize); + Q_SCRIPTABLE bool writeData(const QByteArray& buffer, const QString& targetDevice, const qint64 targetFirstByte); + Q_SCRIPTABLE void exit(); private: void onReadOutput(); std::unique_ptr m_loop; - QCA::Initializer initializer; - QCA::PublicKey m_publicKey; - QRandomGenerator64 m_Generator; - std::unordered_set m_Nonces; QProcess m_cmd; - -// QByteArray output; +// QByteArray output; }; #endif