From 7ee56676289e75f9079c541fd877cc285edf060d Mon Sep 17 00:00:00 2001 From: www-data Date: Sun, 30 Jul 2023 13:14:00 +0200 Subject: [PATCH] Avoid shell command injection in `main.cpp` --- main.cpp | 25 ++++++++++++++++++++++--- website/websocket.php | 8 ++++---- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/main.cpp b/main.cpp index ff5dcb8..3d5de85 100644 --- a/main.cpp +++ b/main.cpp @@ -27,7 +27,9 @@ void createDirectory(string path), markChannelAsRequiringTreatmentIfNeeded(unsigned short threadId, string channelId), execute(unsigned short threadId, string command, bool debug = true); string getHttps(string url), - join(vector parts, string delimiter); + join(vector parts, string delimiter), + escapeShellArgument(string shellArgument), + replaceAll(string str, const string& from, const string& to); size_t writeCallback(void* contents, size_t size, size_t nmemb, void* userp); bool doesFileExist(string filePath), writeFile(unsigned short threadId, string filePath, string option, string toWrite); @@ -242,7 +244,7 @@ void treatChannels(unsigned short threadId) // As I haven't found any well-known library that compress easily a directory, I have chosen to rely on `zip` cli. // We precise no `debug`ging, as otherwise the zipping operation doesn't work as expected. // As the zipping process isn't recursive, we can't just rely on `ls`, but we are obliged to use `find`. - execute(threadId, "cd " + channelToTreatDirectory + " && find | zip ../" + channelToTreat + ".zip -@"); + execute(threadId, "cd " + escapeShellArgument(channelToTreatDirectory) + " && find | zip " + escapeShellArgument("../" + channelToTreat + ".zip") + " -@"); PRINT("Compression finished, started deleting initial directory...") // Get rid of the uncompressed data. @@ -681,7 +683,7 @@ void treatChannelOrVideo(unsigned short threadId, bool isIdAChannelId, string id // The underscore in `-o` argument is used to not end up with hidden files. // We are obliged to precise the video id after `--`, otherwise if the video id starts with `-` it's considered as an argument. string commandCommonPrefix = "yt-dlp --skip-download ", - commandCommonPostfix = " -o '" + channelCaptionsToTreatDirectory + "_' -- " + videoId; + commandCommonPostfix = " -o " + escapeShellArgument(channelCaptionsToTreatDirectory + "_") + " -- " + escapeShellArgument(videoId); string command = commandCommonPrefix + "--write-sub --sub-lang all,-live_chat" + commandCommonPostfix; execute(threadId, command); @@ -929,3 +931,20 @@ size_t writeCallback(void* contents, size_t size, size_t nmemb, void* userp) ((string*)userp)->append((char*)contents, size * nmemb); return size * nmemb; } + +// Source: https://stackoverflow.com/a/3669819 +string escapeShellArgument(string shellArgument) +{ + return "'" + replaceAll(shellArgument, "'", "'\\''") + "'"; +} + +string replaceAll(string str, const string& from, const string& to) +{ + size_t start_pos = 0; + while((start_pos = str.find(from, start_pos)) != string::npos) + { + str.replace(start_pos, from.length(), to); + start_pos += to.length(); // Handles case where 'to' is a substring of 'from' + } + return str; +} diff --git a/website/websocket.php b/website/websocket.php index 5db94ac..285daf8 100644 --- a/website/websocket.php +++ b/website/websocket.php @@ -27,7 +27,7 @@ class Client posix_kill($this->pid, SIGTERM); $clientFilePath = getClientFilePath($this->id); if (file_exists($clientFilePath)) { - $fp = fopen($clientFilePath, "r+"); + $fp = fopen($clientFilePath, 'r+'); if (flock($fp, LOCK_EX, $WAIT_IF_LOCKED)) { // acquire an exclusive lock unlink($clientFilePath); // delete file flock($fp, LOCK_UN); // release the lock @@ -92,8 +92,6 @@ class MyProcess implements MessageComponentInterface public function onMessage(ConnectionInterface $from, $msg) { - // As we are going to use this argument in a shell command, we escape it. - $msg = escapeshellarg($msg); $client = $this->clients->offsetGet($from); // If a previous request was received, we execute the new one with another client for simplicity otherwise with current file deletion approach, we can't tell the worker `search.py` that we don't care about its execution anymore. if ($client->pid !== null) { @@ -105,6 +103,8 @@ class MyProcess implements MessageComponentInterface $clientFilePath = getClientFilePath($clientId); // Create the worker output file otherwise it would believe that we don't need this worker anymore. file_put_contents($clientFilePath, ''); + // As we are going to use this argument in a shell command, we escape it. + $msg = escapeshellarg($msg); // Start the independent worker. // Redirecting `stdout` is mandatory otherwise `exec` is blocking. $client->pid = exec("./search.py $clientId $msg > /dev/null & echo $!"); @@ -114,7 +114,7 @@ class MyProcess implements MessageComponentInterface // If the worker output file doesn't exist anymore, then it means that the worker have finished its work and acknowledged that `websocket.php` completely read its output. if (file_exists($clientFilePath)) { // `flock` requires `r`eading permission and we need `w`riting one due to `ftruncate` usage. - $fp = fopen($clientFilePath, "r+"); + $fp = fopen($clientFilePath, 'r+'); $read = null; if (flock($fp, LOCK_EX, $WAIT_IF_LOCKED)) { // acquire an exclusive lock // We assume that the temporary output is less than 1 MB long.