From 8070f36deec723de71e7557441acb17e478204d3 Mon Sep 17 00:00:00 2001 From: Vaxry Date: Fri, 15 Nov 2024 20:43:31 +0000 Subject: [PATCH] core: move to CProcess from hyprutils this enhances security, avoids calling the shell when invoking the picker --- CMakeLists.txt | 2 +- hyprland-share-picker/CMakeLists.txt | 9 ++++++- hyprland-share-picker/main.cpp | 19 +++++++-------- src/helpers/MiscFunctions.cpp | 23 ++++++++---------- src/shared/ScreencopyShared.cpp | 35 +++++++++++++++++++--------- 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2812d2c..3f88645 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -64,7 +64,7 @@ pkg_check_modules( libdrm gbm hyprlang>=0.2.0 - hyprutils + hyprutils>=0.2.6 hyprwayland-scanner>=0.4.2) # check whether we can find sdbus-c++ through pkg-config diff --git a/hyprland-share-picker/CMakeLists.txt b/hyprland-share-picker/CMakeLists.txt index d9546c3..609863c 100644 --- a/hyprland-share-picker/CMakeLists.txt +++ b/hyprland-share-picker/CMakeLists.txt @@ -17,6 +17,13 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON) find_package(QT NAMES Qt6 REQUIRED COMPONENTS Widgets) find_package(Qt6 REQUIRED COMPONENTS Widgets) +find_package(PkgConfig REQUIRED) +pkg_check_modules( + deps + REQUIRED + IMPORTED_TARGET + hyprutils>=0.2.6) + set(PROJECT_SOURCES main.cpp mainpicker.cpp mainpicker.h mainpicker.ui elidedbutton.h elidedbutton.cpp) @@ -38,7 +45,7 @@ else() endif() target_link_libraries(hyprland-share-picker - PRIVATE Qt${QT_VERSION_MAJOR}::Widgets) + PRIVATE Qt${QT_VERSION_MAJOR}::Widgets PkgConfig::deps) set_target_properties( hyprland-share-picker diff --git a/hyprland-share-picker/main.cpp b/hyprland-share-picker/main.cpp index ff8f206..a361138 100644 --- a/hyprland-share-picker/main.cpp +++ b/hyprland-share-picker/main.cpp @@ -15,21 +15,20 @@ #include #include #include +#include +using namespace Hyprutils::OS; #include "mainpicker.h" #include "elidedbutton.h" std::string execAndGet(const char* cmd) { - std::array buffer; - std::string result; - std::unique_ptr pipe(popen(cmd, "r"), pclose); - if (!pipe) { - throw std::runtime_error("popen() failed!"); - } - while (fgets(buffer.data(), buffer.size(), pipe.get()) != nullptr) { - result += buffer.data(); - } - return result; + std::string command = cmd + std::string{" 2>&1"}; + CProcess proc("/bin/sh", {"-c", cmd}); + + if (!proc.runSync()) + return "error"; + + return proc.stdOut(); } QApplication* pickerPtr = nullptr; diff --git a/src/helpers/MiscFunctions.cpp b/src/helpers/MiscFunctions.cpp index 336e450..7307ba3 100644 --- a/src/helpers/MiscFunctions.cpp +++ b/src/helpers/MiscFunctions.cpp @@ -8,20 +8,17 @@ #include #include -std::string execAndGet(const char* cmd) { - Debug::log(LOG, "execAndGet: {}", cmd); +#include +using namespace Hyprutils::OS; - std::array buffer; - std::string result; - const std::unique_ptr pipe(popen(cmd, "r"), pclose); - if (!pipe) { - Debug::log(ERR, "execAndGet: failed in pipe"); - return ""; - } - while (fgets(buffer.data(), buffer.size(), pipe.get()) != nullptr) { - result += buffer.data(); - } - return result; +std::string execAndGet(const char* cmd) { + std::string command = cmd + std::string{" 2>&1"}; + CProcess proc("/bin/sh", {"-c", cmd}); + + if (!proc.runSync()) + return "error"; + + return proc.stdOut(); } void addHyprlandNotification(const std::string& icon, float timeMs, const std::string& color, const std::string& message) { diff --git a/src/shared/ScreencopyShared.cpp b/src/shared/ScreencopyShared.cpp index 0810fe7..22064a6 100644 --- a/src/shared/ScreencopyShared.cpp +++ b/src/shared/ScreencopyShared.cpp @@ -9,10 +9,15 @@ #include #include +#include +using namespace Hyprutils::OS; + std::string sanitizeNameForWindowList(const std::string& name) { std::string result = name; std::replace(result.begin(), result.end(), '\'', ' '); std::replace(result.begin(), result.end(), '\"', ' '); + std::replace(result.begin(), result.end(), '$', ' '); + std::replace(result.begin(), result.end(), '`', ' '); for (size_t i = 1; i < result.size(); ++i) { if (result[i - 1] == '>' && result[i] == ']') result[i] = ' '; @@ -43,19 +48,28 @@ SSelectionData promptForScreencopySelection() { static auto* const* PALLOWTOKENBYDEFAULT = (Hyprlang::INT* const*)g_pPortalManager->m_sConfig.config->getConfigValuePtr("screencopy:allow_token_by_default")->getDataStaticPtr(); - // DANGEROUS: we are sending a list of app IDs and titles via env. Make sure it's in 'singlequotes' to avoid something like $(rm -rf /) - // TODO: this is dumb, use a pipe or something. - std::string cmd = - std::format("WAYLAND_DISPLAY='{}' QT_QPA_PLATFORM='wayland' XCURSOR_SIZE='{}' HYPRLAND_INSTANCE_SIGNATURE='{}' XDPH_WINDOW_SHARING_LIST='{}' hyprland-share-picker{} 2>&1", - WAYLAND_DISPLAY ? WAYLAND_DISPLAY : "", XCURSOR_SIZE ? XCURSOR_SIZE : "24", HYPRLAND_INSTANCE_SIGNATURE ? HYPRLAND_INSTANCE_SIGNATURE : "0", buildWindowList(), - (**PALLOWTOKENBYDEFAULT ? " --allow-token" : "")); + std::vector args; + if (**PALLOWTOKENBYDEFAULT) + args.emplace_back("--allow-token"); - const auto RETVAL = execAndGet(cmd.c_str()); + CProcess proc("hyprland-share-picker", args); + proc.addEnv("WAYLAND_DISPLAY", WAYLAND_DISPLAY ? WAYLAND_DISPLAY : ""); + proc.addEnv("QT_QPA_PLATFORM", "wayland"); + proc.addEnv("XCURSOR_SIZE", XCURSOR_SIZE ? XCURSOR_SIZE : "24"); + proc.addEnv("HYPRLAND_INSTANCE_SIGNATURE", HYPRLAND_INSTANCE_SIGNATURE ? HYPRLAND_INSTANCE_SIGNATURE : "0"); + proc.addEnv("XDPH_WINDOW_SHARING_LIST", buildWindowList()); // buildWindowList will sanitize any shell stuff in case the picker (qt) does something funky? It shouldn't. + + if (!proc.runSync()) + return data; + + const auto RETVAL = proc.stdOut(); + const auto RETVALERR = proc.stdErr(); if (!RETVAL.contains("[SELECTION]")) { // failed + constexpr const char* QPA_ERR = "qt.qpa.plugin: Could not find the Qt platform plugin"; - if (RETVAL.contains("qt.qpa.plugin: Could not find the Qt platform plugin")) { + if (RETVAL.contains(QPA_ERR) || RETVALERR.contains(QPA_ERR)) { // prompt the user to install qt5-wayland and qt6-wayland addHyprlandNotification("3", 7000, "0", "[xdph] Could not open the picker: qt5-wayland or qt6-wayland doesn't seem to be installed."); } @@ -71,11 +85,10 @@ SSelectionData promptForScreencopySelection() { const auto SEL = SELECTION.substr(SELECTION.find_first_of('/') + 1); for (auto& flag : FLAGS) { - if (flag == 'r') { + if (flag == 'r') data.allowToken = true; - } else { + else Debug::log(LOG, "[screencopy] unknown flag from share-picker: {}", flag); - } } if (SEL.find("screen:") == 0) {