From a90289b0962663bc1d247bbbd31b9e65b2ca000e Mon Sep 17 00:00:00 2001 From: Albert Astals Cid Date: Wed, 10 May 2017 10:21:02 +0200 Subject: Find the mount/umount commands in the helper Instead of trusting what we get passed in CVE-2017-8849 --- core/smb4kglobal.cpp | 32 +++++++++++++++++++ core/smb4kglobal.h | 14 +++++++++ core/smb4kmounter.cpp | 42 +++---------------------- helpers/CMakeLists.txt | 7 +++++ helpers/smb4kmounthelper.cpp | 75 ++++++++++++++++++++++++++++++++++++++++---- helpers/smb4kmounthelper.h | 2 +- 6 files changed, 127 insertions(+), 45 deletions(-) diff --git a/core/smb4kglobal.cpp b/core/smb4kglobal.cpp index 765203d..cfa7ba5 100644 --- a/core/smb4kglobal.cpp +++ b/core/smb4kglobal.cpp @@ -864,3 +864,35 @@ QStringList Smb4KGlobal::whitelistedMountArguments() #endif +const QString Smb4KGlobal::findMountExecutable() +{ + QStringList paths; + paths << "/bin"; + paths << "/sbin"; + paths << "/usr/bin"; + paths << "/usr/sbin"; + paths << "/usr/local/bin"; + paths << "/usr/local/sbin"; + +#if defined(Q_OS_LINUX) + return QStandardPaths::findExecutable("mount.cifs", paths); +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD) + return QStandardPaths::findExecutable("mount_smbfs", paths); +#else + return QString(); +#endif +} + + +const QString Smb4KGlobal::findUmountExecutable() +{ + QStringList paths; + paths << "/bin"; + paths << "/sbin"; + paths << "/usr/bin"; + paths << "/usr/sbin"; + paths << "/usr/local/bin"; + paths << "/usr/local/sbin"; + + return QStandardPaths::findExecutable("umount", paths); +} diff --git a/core/smb4kglobal.h b/core/smb4kglobal.h index 63b6294..f29e342 100644 --- a/core/smb4kglobal.h +++ b/core/smb4kglobal.h @@ -454,6 +454,20 @@ namespace Smb4KGlobal */ Q_DECL_EXPORT QStringList whitelistedMountArguments(); #endif + + /** + * Find the mount executable on the system. + * + * @returns the path of the mount executable. + */ + Q_DECL_EXPORT const QString findMountExecutable(); + + /** + * Find the umount executable on the system. + * + * @returns the path of the umount executable. + */ + Q_DECL_EXPORT const QString findUmountExecutable(); }; #endif diff --git a/core/smb4kmounter.cpp b/core/smb4kmounter.cpp index 91f3863..0bc71aa 100644 --- a/core/smb4kmounter.cpp +++ b/core/smb4kmounter.cpp @@ -1104,16 +1104,7 @@ void Smb4KMounter::timerEvent(QTimerEvent *) bool Smb4KMounter::fillMountActionArgs(Smb4KShare *share, QVariantMap& map) { // Find the mount program. - QString mount; - QStringList paths; - paths << "/bin"; - paths << "/sbin"; - paths << "/usr/bin"; - paths << "/usr/sbin"; - paths << "/usr/local/bin"; - paths << "/usr/local/sbin"; - - mount = QStandardPaths::findExecutable("mount.cifs", paths); + const QString mount = findMountExecutable(); if (!mount.isEmpty()) { @@ -1645,16 +1636,7 @@ bool Smb4KMounter::fillMountActionArgs(Smb4KShare *share, QVariantMap& map) bool Smb4KMounter::fillMountActionArgs(Smb4KShare *share, QVariantMap& map) { // Find the mount program. - QString mount; - QStringList paths; - paths << "/bin"; - paths << "/sbin"; - paths << "/usr/bin"; - paths << "/usr/sbin"; - paths << "/usr/local/bin"; - paths << "/usr/local/sbin"; - - mount = QStandardPaths::findExecutable("mount_smbfs", paths); + const QString mount = findMountExecutable(); if (!mount.isEmpty()) { @@ -1823,15 +1805,7 @@ bool Smb4KMounter::fillUnmountActionArgs(Smb4KShare *share, bool force, bool sil // // The umount program // - QStringList paths; - paths << "/bin"; - paths << "/sbin"; - paths << "/usr/bin"; - paths << "/usr/sbin"; - paths << "/usr/local/bin"; - paths << "/usr/local/sbin"; - - QString umount = QStandardPaths::findExecutable("umount", paths); + const QString umount = findUmountExecutable(); if (umount.isEmpty() && !silent) { @@ -1884,15 +1858,7 @@ bool Smb4KMounter::fillUnmountActionArgs(Smb4KShare *share, bool force, bool sil // // The umount program // - QStringList paths; - paths << "/bin"; - paths << "/sbin"; - paths << "/usr/bin"; - paths << "/usr/sbin"; - paths << "/usr/local/bin"; - paths << "/usr/local/sbin"; - - QString umount = QStandardPaths::findExecutable("umount", paths); + const QString umount = findUmountExecutable(); if (umount.isEmpty() && !silent) { diff --git a/helpers/CMakeLists.txt b/helpers/CMakeLists.txt index 77bb3a5..015c8b6 100644 --- a/helpers/CMakeLists.txt +++ b/helpers/CMakeLists.txt @@ -1,8 +1,15 @@ +include_directories( + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_BINARY_DIR} + ${CMAKE_SOURCE_DIR}/core + ${CMAKE_BINARY_DIR}/core) + set(smb4kmounthelper_SRCS smb4kmounthelper.cpp) add_executable(mounthelper ${smb4kmounthelper_SRCS}) target_link_libraries(mounthelper + smb4kcore Qt5::Core KF5::Auth KF5::CoreAddons diff --git a/helpers/smb4kmounthelper.cpp b/helpers/smb4kmounthelper.cpp index 641530e..0a1e215 100644 --- a/helpers/smb4kmounthelper.cpp +++ b/helpers/smb4kmounthelper.cpp @@ -2,7 +2,7 @@ The helper that mounts and unmounts shares. ------------------- begin : Sa Okt 16 2010 - copyright : (C) 2010-2016 by Alexander Reinholdt + copyright : (C) 2010-2017 by Alexander Reinholdt email : alexander.reinholdt@kdemail.net ***************************************************************************/ @@ -29,6 +29,7 @@ // application specific includes #include "smb4kmounthelper.h" +#include "../core/smb4kglobal.h" // Qt includes #include @@ -42,14 +43,24 @@ #include #include +using namespace Smb4KGlobal; + KAUTH_HELPER_MAIN("org.kde.smb4k.mounthelper", Smb4KMountHelper); ActionReply Smb4KMountHelper::mount(const QVariantMap &args) { + // + // The action reply + // ActionReply reply; // + // Get the mount executable + // + const QString mount = findMountExecutable(); + + // // Iterate through the entries. // QMapIterator it(args); @@ -61,6 +72,20 @@ ActionReply Smb4KMountHelper::mount(const QVariantMap &args) QVariantMap entry = it.value().toMap(); // + // Check the executable + // + if (mount != entry["mh_command"].toString()) + { + // Something weird is going on, bail out. + reply.setType(ActionReply::HelperErrorType); + return reply; + } + else + { + // Do nothing + } + + // // The process // KProcess proc(this); @@ -87,12 +112,12 @@ ActionReply Smb4KMountHelper::mount(const QVariantMap &args) // QStringList command; #if defined(Q_OS_LINUX) - command << entry["mh_command"].toString(); + command << mount; command << entry["mh_unc"].toString(); command << entry["mh_mountpoint"].toString(); command << entry["mh_options"].toStringList(); #elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD) - command << entry["mh_command"].toString(); + command << mount; command << entry["mh_options"].toStringList(); command << entry["mh_unc"].toString(); command << entry["mh_mountpoint"].toString(); @@ -208,6 +233,11 @@ ActionReply Smb4KMountHelper::unmountOneByOne(const QVariantMap& args) ActionReply reply; // + // Get the mount executable + // + const QString umount = findUmountExecutable(); + + // // Iterate through the entries. // QMapIterator it(args); @@ -217,6 +247,20 @@ ActionReply Smb4KMountHelper::unmountOneByOne(const QVariantMap& args) it.next(); QString index = it.key(); QVariantMap entry = it.value().toMap(); + + // + // Check the executable + // + if (umount != entry["mh_command"].toString()) + { + // Something weird is going on, bail out. + reply.setType(ActionReply::HelperErrorType); + return reply; + } + else + { + // Do nothing + } // // Check if the mountpoint is valid and the filesystem is correct. @@ -261,7 +305,7 @@ ActionReply Smb4KMountHelper::unmountOneByOne(const QVariantMap& args) // The command // QStringList command; - command << entry["mh_command"].toString(); + command << umount; command << entry["mh_options"].toStringList(); command << entry["mh_mountpoint"].toString(); @@ -342,6 +386,11 @@ ActionReply Smb4KMountHelper::unmountAtOnce(const QVariantMap& args) ActionReply reply; // + // Get the mount executable + // + const QString umount = findUmountExecutable(); + + // // Check the mountpoints and put the valid ones into a string list // QStringList validMountPoints; @@ -351,7 +400,7 @@ ActionReply Smb4KMountHelper::unmountAtOnce(const QVariantMap& args) { it.next(); QVariantMap entry = it.value().toMap(); - + bool mountPointOk = false; KMountPoint::List mountPoints = KMountPoint::currentMountPoints(KMountPoint::BasicInfoNeeded|KMountPoint::NeedMountOptions); @@ -399,9 +448,23 @@ ActionReply Smb4KMountHelper::unmountAtOnce(const QVariantMap& args) if (!validMountPoints.isEmpty()) { + // + // Check the executable + // + if (umount != args.first().toMap().value("mh_command").toString()) + { + // Something weird is going on, bail output + reply.setType(ActionReply::HelperErrorType); + return reply; + } + else + { + // Do nothing + } + // The command QStringList command; - command << args.first().toMap().value("mh_command").toString(); + command << umount; command << args.first().toMap().value("mh_options").toStringList(); command << validMountPoints; diff --git a/helpers/smb4kmounthelper.h b/helpers/smb4kmounthelper.h index f3bc573..4b735af 100644 --- a/helpers/smb4kmounthelper.h +++ b/helpers/smb4kmounthelper.h @@ -2,7 +2,7 @@ The helper that mounts and unmounts shares. ------------------- begin : Sa Okt 16 2010 - copyright : (C) 2010-2016 by Alexander Reinholdt + copyright : (C) 2010-2017 by Alexander Reinholdt email : alexander.reinholdt@kdemail.net ***************************************************************************/ -- cgit v0.11.2