From aceb9c08df80da9c929af2998c54870455b79a03 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Mon, 4 May 2015 19:29:32 -0700 Subject: [PATCH] Implement the ssh(1) escaping rules. The first rule of ssh(1) escaping is that there is no escaping. This doesn't undo any of my recent security fixes because they're all calling escape_arg themselves. This fixes "adb shell rm /data/dalvik-cache/arm/*". Also remove do_cmd which caused concern during code review. Bug: http://b/20564385 Change-Id: I4588fd949d51e2a50cff47ea171ed2d75f402d0d (cherry picked from commit 2b10111d25adcfe3627103b73edad22f188c97ba) --- adb/commandline.cpp | 109 +++++++++++++++--------------------------- adb/tests/test_adb.py | 11 +++++ 2 files changed, 50 insertions(+), 70 deletions(-) diff --git a/adb/commandline.cpp b/adb/commandline.cpp index b3855177a..aa31bfd83 100644 --- a/adb/commandline.cpp +++ b/adb/commandline.cpp @@ -47,14 +47,9 @@ #include "adb_utils.h" #include "file_sync_service.h" -static int do_cmd(transport_type ttype, const char* serial, const char *cmd, ...); - -static int install_app(transport_type transport, const char* serial, int argc, - const char** argv); -static int install_multiple_app(transport_type transport, const char* serial, int argc, - const char** argv); -static int uninstall_app(transport_type transport, const char* serial, int argc, - const char** argv); +static int install_app(transport_type t, const char* serial, int argc, const char** argv); +static int install_multiple_app(transport_type t, const char* serial, int argc, const char** argv); +static int uninstall_app(transport_type t, const char* serial, int argc, const char** argv); static std::string gProductOutPath; extern int gListenAll; @@ -678,7 +673,31 @@ static int ppp(int argc, const char** argv) { #endif /* !defined(_WIN32) */ } -static int send_shell_command(transport_type transport, const char* serial, +static bool wait_for_device(const char* service, transport_type t, const char* serial) { + // Was the caller vague about what they'd like us to wait for? + // If so, check they weren't more specific in their choice of transport type. + if (strcmp(service, "wait-for-device") == 0) { + if (t == kTransportUsb) { + service = "wait-for-usb"; + } else if (t == kTransportLocal) { + service = "wait-for-local"; + } else { + service = "wait-for-any"; + } + } + + std::string cmd = format_host_command(service, t, serial); + std::string error; + if (adb_command(cmd, &error)) { + D("failure: %s *\n", error.c_str()); + fprintf(stderr,"error: %s\n", error.c_str()); + return false; + } + + return true; +} + +static int send_shell_command(transport_type transport_type, const char* serial, const std::string& command) { int fd; while (true) { @@ -689,7 +708,7 @@ static int send_shell_command(transport_type transport, const char* serial, } fprintf(stderr,"- waiting for device -\n"); adb_sleep_ms(1000); - do_cmd(transport, serial, "wait-for-device", 0); + wait_for_device("wait-for-device", transport_type, serial); } read_and_dump(fd); @@ -1070,27 +1089,13 @@ int adb_commandline(int argc, const char **argv) { /* handle wait-for-* prefix */ if (!strncmp(argv[0], "wait-for-", strlen("wait-for-"))) { const char* service = argv[0]; - if (!strncmp(service, "wait-for-device", strlen("wait-for-device"))) { - if (ttype == kTransportUsb) { - service = "wait-for-usb"; - } else if (ttype == kTransportLocal) { - service = "wait-for-local"; - } else { - service = "wait-for-any"; - } - } - std::string cmd = format_host_command(service, ttype, serial); - std::string error; - if (adb_command(cmd, &error)) { - D("failure: %s *\n", error.c_str()); - fprintf(stderr,"error: %s\n", error.c_str()); + if (!wait_for_device(service, ttype, serial)) { return 1; } - /* Allow a command to be run after wait-for-device, - * e.g. 'adb wait-for-device shell'. - */ + // Allow a command to be run after wait-for-device, + // e.g. 'adb wait-for-device shell'. if (argc == 1) { return 0; } @@ -1157,11 +1162,12 @@ int adb_commandline(int argc, const char **argv) { } std::string cmd = "shell:"; - cmd += argv[1]; - argc -= 2; - argv += 2; + --argc; + ++argv; while (argc-- > 0) { - cmd += " " + escape_arg(*argv++); + // We don't escape here, just like ssh(1). http://b/20564385. + cmd += *argv++; + if (*argv) cmd += " "; } while (true) { @@ -1183,7 +1189,7 @@ int adb_commandline(int argc, const char **argv) { if (persist) { fprintf(stderr,"\n- waiting for device -\n"); adb_sleep_ms(1000); - do_cmd(ttype, serial, "wait-for-device", 0); + wait_for_device("wait-for-device", ttype, serial); } else { if (h) { printf("\x1b[0m"); @@ -1259,8 +1265,7 @@ int adb_commandline(int argc, const char **argv) { } else if (!strcmp(argv[0], "bugreport")) { if (argc != 1) return usage(); - do_cmd(ttype, serial, "shell", "bugreport", 0); - return 0; + return send_shell_command(ttype, serial, "shell:bugreport"); } /* adb_command() wrapper commands */ else if (!strcmp(argv[0], "forward") || !strcmp(argv[0], "reverse")) { @@ -1480,42 +1485,6 @@ int adb_commandline(int argc, const char **argv) { return 1; } -#define MAX_ARGV_LENGTH 16 -static int do_cmd(transport_type ttype, const char* serial, const char *cmd, ...) -{ - const char *argv[MAX_ARGV_LENGTH]; - int argc; - va_list ap; - - va_start(ap, cmd); - argc = 0; - - if (serial) { - argv[argc++] = "-s"; - argv[argc++] = serial; - } else if (ttype == kTransportUsb) { - argv[argc++] = "-d"; - } else if (ttype == kTransportLocal) { - argv[argc++] = "-e"; - } - - argv[argc++] = cmd; - while(argc < MAX_ARGV_LENGTH && - (argv[argc] = va_arg(ap, char*)) != 0) argc++; - assert(argc < MAX_ARGV_LENGTH); - va_end(ap); - -#if 0 - int n; - fprintf(stderr,"argc = %d\n",argc); - for(n = 0; n < argc; n++) { - fprintf(stderr,"argv[%d] = \"%s\"\n", n, argv[n]); - } -#endif - - return adb_commandline(argc, argv); -} - static int pm_command(transport_type transport, const char* serial, int argc, const char** argv) { diff --git a/adb/tests/test_adb.py b/adb/tests/test_adb.py index 52d805687..237ef47ef 100755 --- a/adb/tests/test_adb.py +++ b/adb/tests/test_adb.py @@ -272,13 +272,24 @@ class AdbBasic(unittest.TestCase): adb = AdbWrapper() # http://b/19734868 + # Note that this actually matches ssh(1)'s behavior --- it's + # converted to "sh -c echo hello; echo world" which sh interprets + # as "sh -c echo" (with an argument to that shell of "hello"), + # and then "echo world" back in the first shell. result = adb.shell("sh -c 'echo hello; echo world'").splitlines() + self.assertEqual(["", "world"], result) + # If you really wanted "hello" and "world", here's what you'd do: + result = adb.shell("echo hello\;echo world").splitlines() self.assertEqual(["hello", "world"], result) # http://b/15479704 self.assertEqual('t', adb.shell("'true && echo t'").strip()) self.assertEqual('t', adb.shell("sh -c 'true && echo t'").strip()) + # http://b/20564385 + self.assertEqual('t', adb.shell("FOO=a BAR=b echo t").strip()) + self.assertEqual('123Linux', adb.shell("echo -n 123\;uname").strip()) + class AdbFile(unittest.TestCase): SCRATCH_DIR = "/data/local/tmp"