Fix command injection on PDB download (#16966)

* Fix r_sys_mkdirp with absolute path on Windows
* Fix build with --with-openssl
* Use RBuffer in r_socket_http_answer()
* r_socket_http_answer: Fix read for big responses
* Implement r_str_escape_sh()
* Cleanup r_socket_connect() on Windows
* Fix socket being created without a protocol
* Fix socket connect with SSL ##socket
* Use select() in r_socket_ready()
* Fix read failing if received only protocol answer
* Fix double-free
* r_socket_http_get: Fail if req. SSL with no support
* Follow redirects in r_socket_http_answer()
* Fix r_socket_http_get result length with R2_CURL=1
* Also follow redirects
* Avoid using curl for downloading PDBs
* Use r_socket_http_get() on UNIXs
* Use WinINet API on Windows for r_socket_http_get()
* Fix command injection
* Fix r_sys_cmd_str_full output for binary data
* Validate GUID on PDB download
* Pass depth to socket_http_get_recursive()
* Remove 'r_' and '__' from static function names
* Fix is_valid_guid
* Fix for comments

libr/bin/pdb/pdb_downloader.c

@@ -18,150 +18,107 @@ static bool checkExtract() {
 	return true;
 }
 
-static bool checkCurl() {
-	const char nul[] = R_SYS_DEVNULL;
-	if (r_sys_cmdf ("curl --version > %s", nul) != 0) {
+static bool download_and_write(SPDBDownloaderOpt *opt, const char *file) {
+	char *dir = r_str_newf ("%s%s%s%s%s",
+		opt->symbol_store_path, R_SYS_DIR,
+		opt->dbg_file, R_SYS_DIR,
+		opt->guid);
+	if (!r_sys_mkdirp (dir)) {
+		free (dir);
 		return false;
 	}
+	char *url = r_str_newf ("%s/%s/%s/%s", opt->symbol_server, opt->dbg_file, opt->guid, file);
+	int len;
+	char *file_buf = r_socket_http_get (url, NULL, &len);
+	free (url);
+	if (!len || R_STR_ISEMPTY (file_buf)) {
+		free (dir);
+		free (file_buf);
+		return false;
+	}
+	char *path = r_str_newf ("%s%s%s", dir, R_SYS_DIR, opt->dbg_file);
+	FILE *f = fopen (path, "wb");
+	if (f) {
+		fwrite (file_buf, sizeof (char), (size_t)len, f);
+		fclose (f);
+	}
+	free (dir);
+	free (path);
+	free (file_buf);
 	return true;
 }
 
 static int download(struct SPDBDownloader *pd) {
 	SPDBDownloaderOpt *opt = pd->opt;
-	char *curl_cmd = NULL;
-	char *extractor_cmd = NULL;
-	char *abspath_to_archive = NULL;
-	char *abspath_to_file = NULL;
-	char *archive_name = NULL;
-	size_t archive_name_len = 0;
-	char *symbol_store_path = NULL;
-	char *dbg_file = NULL;
-	char *guid = NULL;
-	char *archive_name_escaped  = NULL;
-	char *user_agent = NULL;
-	char *symbol_server = NULL;
-
 	int res = 0;
 	int cmd_ret;
+
 	if (!opt->dbg_file || !*opt->dbg_file) {
 		// no pdb debug file
 		return 0;
 	}
-	if (!checkCurl ()) {
-		return 0;
-	}
-	// dbg_file len is > 0
-	archive_name_len = strlen (opt->dbg_file);
-	archive_name = malloc (archive_name_len + 1);
-	if (!archive_name) {
-		return 0;
-	}
-	memcpy (archive_name, opt->dbg_file, archive_name_len + 1);
-	archive_name[archive_name_len - 1] = '_';
-	symbol_store_path = r_str_escape (opt->symbol_store_path);
-	dbg_file = r_str_escape (opt->dbg_file);
-	guid = r_str_escape (opt->guid);
-	archive_name_escaped = r_str_escape (archive_name);
-	user_agent = r_str_escape (opt->user_agent);
-	symbol_server = r_str_escape (opt->symbol_server);
-
-	abspath_to_archive = r_str_newf ("%s%s%s%s%s%s%s",
-			    symbol_store_path, R_SYS_DIR,
-			    dbg_file, R_SYS_DIR,
-			    guid, R_SYS_DIR,
-			    archive_name_escaped);
-
-	abspath_to_file = strdup (abspath_to_archive);
-	abspath_to_file[strlen (abspath_to_file) - 1] = 'b';
+
+	char *abspath_to_file = r_str_newf ("%s%s%s%s%s%s%s",
+		opt->symbol_store_path, R_SYS_DIR,
+		opt->dbg_file, R_SYS_DIR,
+		opt->guid, R_SYS_DIR,
+		opt->dbg_file);
+
 	if (r_file_exists (abspath_to_file)) {
 		eprintf ("File already downloaded.\n");
-		R_FREE (user_agent);
-		R_FREE (abspath_to_archive);
-		R_FREE (archive_name_escaped);
-		R_FREE (symbol_store_path);
-		R_FREE (dbg_file);
-		R_FREE (guid);
-		R_FREE (archive_name);
-		R_FREE (abspath_to_file);
-		R_FREE (symbol_server);
+		free (abspath_to_file);
 		return 1;
 	}
 
 	if (checkExtract () || opt->extract == 0) {
-		res = 1;
-
-		curl_cmd = r_str_newf ("curl -sfLA \"%s\" \"%s/%s/%s/%s\" --create-dirs -o \"%s\"",
-		                       user_agent,
-		                       symbol_server,
-							   dbg_file,
-							   guid,
-		                       archive_name_escaped,
-		                       abspath_to_archive);
+		char *extractor_cmd = NULL;
+		char *archive_name = strdup (opt->dbg_file);
+		archive_name[strlen (archive_name) - 1] = '_';
+		char *abspath_to_archive = r_str_newf ("%s%s%s%s%s%s%s",
+			opt->symbol_store_path, R_SYS_DIR,
+			opt->dbg_file, R_SYS_DIR,
+			opt->guid, R_SYS_DIR,
+			archive_name);
+
+		eprintf ("Attempting to download compressed pdb in %s\n", abspath_to_archive);
+		char *abs_arch_esc = r_str_escape_sh (abspath_to_archive);
 #if __WINDOWS__
-		const char *cabextractor = "expand";
-		const char *format = "%s %s %s";
-
-		// extractor_cmd -> %1 %2 %3
-		// %1 - 'expand'
-		// %2 - absolute path to archive
-		// %3 - absolute path to file that will be dearchive
-		extractor_cmd = r_str_newf (format, cabextractor,
-			abspath_to_archive, abspath_to_file);
+		char *abs_file_esc = r_str_escape_sh (abspath_to_file);
+		// expand %1 %2
+		// %1 - absolute path to archive
+		// %2 - absolute path to file that will be dearchive
+		extractor_cmd = r_str_newf ("expand \"%s\" \"%s\"", abs_arch_esc, abs_file_esc);
+		free (abs_file_esc);
 #else
-		const char *cabextractor = "cabextract";
-		const char *format = "%s -d \"%s\" \"%s\"";
 		char *abspath_to_dir = r_file_dirname (abspath_to_archive);
+		char *abs_dir_esc = r_str_escape_sh (abspath_to_dir);
 		// cabextract -d %1 %2
 		// %1 - path to directory where to extract all files from cab archive
 		// %2 - absolute path to cab archive
-		extractor_cmd = r_str_newf (format, cabextractor, abspath_to_dir, abspath_to_archive);
-		R_FREE (abspath_to_dir);
+		extractor_cmd = r_str_newf ("cabextract -d \"%s\" \"%s\"", abs_arch_esc, abs_dir_esc);
+		free (abs_dir_esc);
+		free (abspath_to_dir);
 #endif
-		eprintf ("Attempting to download compressed pdb in %s\n", abspath_to_archive);
-		if ((cmd_ret = r_sys_cmd (curl_cmd) != 0)) {
-			eprintf("curl exited with error %d\n", cmd_ret);
-			res = 0;
-		}
-		eprintf ("Attempting to decompress pdb\n");
-		if (opt->extract > 0) {
+		free (abs_arch_esc);
+		res = download_and_write (opt, archive_name);
+
+		if (opt->extract > 0 && res) {
+			eprintf ("Attempting to decompress pdb\n");
 			if (res && ((cmd_ret = r_sys_cmd (extractor_cmd)) != 0)) {
 				eprintf ("cab extractor exited with error %d\n", cmd_ret);
 				res = 0;
 			}
 			r_file_rm (abspath_to_archive);
 		}
-		R_FREE (curl_cmd);
+		free (archive_name);
+		free (abspath_to_archive);
 	}
 	if (res == 0) {
 		eprintf ("Falling back to uncompressed pdb\n");
-		res = 1;
-
-		archive_name_escaped[strlen (archive_name_escaped) - 1] = 'b';
-
-		curl_cmd = r_str_newf ("curl -sfLA \"%s\" \"%s/%s/%s/%s\" --create-dirs -o \"%s\"",
-		                       opt->user_agent,
-		                       opt->symbol_server,
-		                       opt->dbg_file,
-		                       opt->guid,
-		                       archive_name_escaped,
-		                       abspath_to_file);
 		eprintf ("Attempting to download uncompressed pdb in %s\n", abspath_to_file);
-		if ((cmd_ret = r_sys_cmd (curl_cmd) != 0)) {
-			eprintf("curl exited with error %d\n", cmd_ret);
-			res = 0;
-		}
-		R_FREE (curl_cmd);
-	}
-	R_FREE (abspath_to_archive);
-	R_FREE (abspath_to_file);
-	R_FREE (archive_name);
-	R_FREE (extractor_cmd);
-	R_FREE (symbol_store_path);
-	R_FREE (dbg_file);
-	R_FREE (guid);
-	R_FREE (archive_name_escaped);
-	R_FREE (user_agent);
-	R_FREE (symbol_server);
+		res = download_and_write (opt, opt->dbg_file);
+	}
+	free (abspath_to_file);
 	return res;
 }
 
@@ -191,6 +148,19 @@ void deinit_pdb_downloader(SPDBDownloader *pd) {
 	pd->download = 0;
 }
 
+static bool is_valid_guid(const char *guid) {
+	if (!guid) {
+		return false;
+	}
+	size_t i;
+	for (i = 0; guid[i]; i++) {
+		if (!isxdigit (guid[i])) {
+			return false;
+		}
+	}
+	return i >= 33; // len of GUID and age
+}
+
 int r_bin_pdb_download(RCore *core, int isradjson, int *actions_done, SPDBOptions *options) {
 	int ret;
 	SPDBDownloaderOpt opt;
@@ -202,6 +172,11 @@ int r_bin_pdb_download(RCore *core, int isradjson, int *actions_done, SPDBOption
 		return 1;
 	}
 
+	if (!is_valid_guid (info->guid)) {
+		eprintf ("Invalid GUID for file\n");
+		return 1;
+	}
+
 	if (!options || !options->symbol_server || !options->user_agent) {
 		eprintf ("Can't retrieve pdb configurations\n");
 		return 1;

libr/config.mk.tail

@@ -52,6 +52,11 @@ CFLAGS+=-DHAVE_LIB_GMP=1
 BN_LIBS=-lgmp
 endif
 
+# open-ssl
+ifeq (${HAVE_LIB_SSL},1)
+BN_LIBS=${SSL_LDFLAGS}
+endif
+
 #both of these need ssl includes
 ifneq (,$(filter r_socket r_util,$(BINDEPS)))
 ifeq (${HAVE_LIB_SSL},1)

libr/include/r_socket.h

@@ -1,11 +1,6 @@
 #ifndef R2_SOCKET_H
 #define R2_SOCKET_H
 
-/* Must be included before windows.h (r_types) */
-#if defined(__WINDOWS__)
-#include <ws2tcpip.h>
-#endif
-
 #include "r_types.h"
 #include "r_bind.h"
 #include "r_list.h"

libr/include/r_types.h

@@ -152,6 +152,7 @@
   #ifdef _MSC_VER
   /* Must be included before windows.h */
   #include <winsock2.h>
+  #include <ws2tcpip.h>
   #ifndef WIN32_LEAN_AND_MEAN
   #define WIN32_LEAN_AND_MEAN
   #endif

libr/include/r_util/r_str.h

@@ -148,6 +148,7 @@ R_API int r_str_path_unescape(char *path);
 R_API char *r_str_path_escape(const char *path);
 R_API int r_str_unescape(char *buf);
 R_API char *r_str_escape(const char *buf);
+R_API char *r_str_escape_sh(const char *buf);
 R_API char *r_str_escape_dot(const char *buf);
 R_API char *r_str_escape_latin1(const char *buf, bool show_asciidot, bool esc_bslash, bool colors);
 R_API char *r_str_escape_utf8(const char *buf, bool show_asciidot, bool esc_bslash);