Merge remote-tracking branch 'origin/patch'

Ghidra/Debug/Debugger-jpda/src/main/java/ghidra/dbg/jdi/rmi/jpda/JdiArguments.java

@@ -22,7 +22,7 @@ import com.sun.jdi.connect.AttachingConnector;
 import com.sun.jdi.connect.Connector;
 import com.sun.jdi.connect.Connector.Argument;
 
-import ghidra.pty.ShellUtils;
+import ghidra.pty.ShellUtils.Shell;
 
 public class JdiArguments {
 	enum Mode {
@@ -101,7 +101,7 @@ public class JdiArguments {
 				}
 				String cp = env.get("OPT_TARGET_CLASSPATH");
 				if (!cp.isBlank()) {
-					args.get("options").setValue("-cp " + ShellUtils.generateArgument(cp));
+					args.get("options").setValue("-cp " + Shell.LOCAL.generateArgument(cp));
 				}
 			}
 		}

Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/launcher/AbstractTraceRmiLaunchOffer.java

@@ -50,6 +50,7 @@ import ghidra.program.model.address.Address;
 import ghidra.program.model.listing.Program;
 import ghidra.program.util.ProgramLocation;
 import ghidra.pty.*;
+import ghidra.pty.ShellUtils.Shell;
 import ghidra.trace.model.Trace;
 import ghidra.trace.model.TraceLocation;
 import ghidra.util.*;
@@ -475,7 +476,7 @@ public abstract class AbstractTraceRmiLaunchOffer implements TraceRmiLaunchOffer
 			parent.getInputStream(), parent.getOutputStream());
 
 		List<String> withoutPath = ShellUtils.removePath(commandLine);
-		terminal.setSubTitle(ShellUtils.generateLine(withoutPath));
+		terminal.setSubTitle(ShellUtils.generateLine(withoutPath, Shell.DISPLAY));
 		TerminalListener resizeListener = new TerminalListener() {
 			@Override
 			public void resized(short cols, short rows) {
@@ -490,8 +491,19 @@ public abstract class AbstractTraceRmiLaunchOffer implements TraceRmiLaunchOffer
 		terminal.addTerminalListener(resizeListener);
 
 		env.put("TERM", "xterm-256color");
-		PtySession session =
-			pty.getChild().session(commandLine.toArray(String[]::new), env, workingDirectory);
+		PtySession session;
+		try {
+			session =
+				pty.getChild().session(commandLine.toArray(String[]::new), env, workingDirectory);
+		}
+		catch (Throwable t) {
+			terminal.terminated(-1);
+			pty.close();
+			for (TerminalSession ss : subordinates) {
+				ss.terminate();
+			}
+			throw t;
+		}
 
 		Thread waiter = new Thread(() -> {
 			try {
@@ -718,6 +730,7 @@ public abstract class AbstractTraceRmiLaunchOffer implements TraceRmiLaunchOffer
 				 * terminates early
 				 */
 				monitor.setMessage("Waiting for connection");
+				monitor.addCancelledListener(acceptor::cancel);
 				connection = acceptOrSessionEnds(acceptor, backEnd);
 				connection.registerTerminals(sessions.values());
 				monitor.increment();

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/terminal/TerminalProvider.java

@@ -442,7 +442,9 @@ public class TerminalProvider extends ComponentProviderAdapter {
 		Swing.runIfSwingOrRunLater(() -> {
 			terminated = true;
 			panel.terminalListeners.invoke().terminated(exitcode);
-			removeLocalAction(actionTerminate);
+			if (actionTerminate != null) {
+				removeLocalAction(actionTerminate);
+			}
 			panel.terminalListeners.clear();
 			panel.setOutputCallback(buf -> {
 			});

Ghidra/Framework/Pty/src/main/java/ghidra/pty/ShellUtils.java

@@ -19,11 +19,28 @@ import java.nio.file.Paths;
 import java.util.*;
 import java.util.stream.Collectors;
 
+import ghidra.framework.OperatingSystem;
+import ghidra.framework.Platform;
+
 public class ShellUtils {
 	enum State {
 		NORMAL, NORMAL_ESCAPE, DQUOTE, DQUOTE_ESCAPE, SQUOTE, SQUOTE_ESCAPE;
 	}
 
+	/**
+	 * Parse a command line into an argument list
+	 * <p>
+	 * LATER: This is meant to mimic UNIX- / C-style arguments, but that's probably not appropriate
+	 * on all systems. We should either:
+	 * 
+	 * <ol>
+	 * <li>Let the offer specify how <code>@args</code> ought to be treated.</li>
+	 * <li>Let the header annotations for <code>@args</code> include some extra specifier.</li>
+	 * </ol>
+	 * 
+	 * @param args the arguments as a single string
+	 * @return the list of split arguments
+	 */
 	public static List<String> parseArgs(String args) {
 		List<String> argsList = new ArrayList<>();
 		StringBuilder curArg = new StringBuilder();
@@ -123,29 +140,207 @@ public class ShellUtils {
 		return List.copyOf(copy);
 	}
 
-	public static String generateLine(List<String> args) {
+	public static String generateLine(List<String> args, Shell shell) {
 		if (args.isEmpty()) {
 			return "";
 		}
-		StringBuilder line = new StringBuilder(generateArgument(args.get(0)));
+		StringBuilder line = new StringBuilder(shell.generateArgument(args.get(0)));
 		for (int i = 1; i < args.size(); i++) {
 			String a = args.get(i);
-			line.append(" " + generateArgument(a));
+			line.append(" " + shell.generateArgument(a));
 		}
 		return line.toString();
 	}
 
-	public static String generateArgument(String a) {
-		if (a.contains(" ")) {
-			if (a.contains("\"")) {
-				if (a.contains("'")) {
-					return "\"" + a.replace("\"", "\\\"") + "\"";
+	/**
+	 * A target shell for command-line arguments
+	 * <p>
+	 * This determines how arguments are quoted and/or escaped. This should be set based on the
+	 * shell that is going to receive the actual commands, which may or may not be the local shell.
+	 * In many cases, it is the local shell, but please ensure for remote cases, the correct shell
+	 * is specified.
+	 */
+	public enum Shell {
+		/**
+		 * For display purposes only. DO NOT pass to any actual shell.
+		 */
+		DISPLAY {
+			@Override
+			public String generateArgument(String a) {
+				if (a.contains(" ")) {
+					if (a.contains("\"")) {
+						if (a.contains("'")) {
+							return '"' + a.replace("\"", "\\\"") + '"';
+						}
+						return "'" + a + "'";
+					}
+					return '"' + a + '"';
 				}
-				return "'" + a + "'";
+				return a;
 			}
-			return "\"" + a + "\"";
+		},
+		/**
+		 * Unix shells that follow the same conventions as "sh". This is most Unix shells.
+		 */
+		UNIX_SH {
+			@Override
+			public String generateArgument(String a) {
+				StringBuilder b = new StringBuilder();
+				for (int i = 0; i < a.length(); i++) {
+					char c = a.charAt(i);
+					boolean esc = switch (c) {
+						case '\t', ' ', // Whitespace
+								'&', '|', ';', '`', '(', ')', // Syntax, command separators
+								'<', '>', // Redirection
+								'$', // Variable substitution
+								'#', // Comments
+								'[', ']', '?', '*', // File globbing
+								'"', '\'', // Quotes
+								'\\' // The escape character itself
+								-> true;
+						default -> false;
+					};
+					if (esc) {
+						b.append('\\');
+					}
+					b.append(c);
+				}
+				return b.toString();
+			}
+		},
+		/**
+		 * Plain Windows command-line arguments for the C runtime
+		 * <p>
+		 * See <href a=
+		 * "https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw">CommandLineToArgvW
+		 * function</a>
+		 */
+		WINDOWS {
+			/**
+			 * Derived from <a href=
+			 * "https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way">Everyone
+			 * quotes command line arguments the wrong way</a>
+			 * <p>
+			 * The section titled "The Correct Solution" invites its readers to "translate it into
+			 * your language and coding style off choice."
+			 */
+			@Override
+			public String generateArgument(String a) {
+				if (!a.isEmpty() && a.indexOf(' ') == -1 && a.indexOf('\t') == -1 &&
+					a.indexOf('\n') == -1 && a.indexOf('"') == -1) {
+					return a;
+				}
+				StringBuilder b = new StringBuilder();
+				b.append('"');
+				for (int i = 0;; i++) {
+					int nBackSlash = 0;
+					while (i < a.length() && a.charAt(i) == '\\') {
+						i++;
+						nBackSlash++;
+					}
+
+					if (i == a.length()) {
+						/**
+						 * We reached the end of the argument while counting backslashes. Escape
+						 * them all. The terminating " we add at the end of this method will be
+						 * interpreted as a metacharacter.
+						 */
+						b.append("\\".repeat(nBackSlash * 2));
+						break;
+					}
+					else if (a.charAt(i) == '"') {
+						/**
+						 * Sequence of backslashes ends in a ". Escape them all, including the ".
+						 */
+						b.append("\\".repeat(nBackSlash * 2 + 1));
+						b.append(a.charAt(i));
+					}
+					else {
+						/**
+						 * They're just literal backslashes. Do not escape them. Be sure to add the
+						 * current character, too.
+						 */
+						b.append("\\".repeat(nBackSlash)); // No *2
+						b.append(a.charAt(i));
+					}
+				}
+				b.append('"');
+				return b.toString();
+			}
+		},
+		/**
+		 * The Windows cmd.exe shell.
+		 * <p>
+		 * <b>NOTE:</b> It seems to me using this with {@link ShellUtils#generateLine(List, Shell)}
+		 * is futile, if the intent is to use specific argument numbers in the batch file, e.g.,
+		 * <code>%1</code>. If you make clear certain constraints to the user, maybe it's suitable,
+		 * but especially involving quotes, it's not possible to encode any arbitrary string. It
+		 * seems the cmd shell is primarily concerned with just passing the arguments along to child
+		 * processes, as encoded, and then the child figures out the parsing. That said, if the
+		 * child process parses to argc/argv, then so long as the <em>full</em> command line is
+		 * passed through the batch file, it should work as intended. However, if grabbing
+		 * individual arguments, they cannot be reliably controlled.
+		 * <p>
+		 * LATER: There may be a way to factor the escaping part separately from the argument
+		 * catenation part, so that special logic can be applied here to better guarantee argument
+		 * numbering, but then there's still the issue if the final target is expected to parse to
+		 * argc/argv, if that can be encoded reliably.
+		 */
+		WINDOWS_CMD {
+			@Override
+			public String generateArgument(String a) {
+				String quoted = WINDOWS.generateArgument(a);
+				StringBuilder b = new StringBuilder();
+				for (int i = 0; i < quoted.length(); i++) {
+					char c = quoted.charAt(i);
+					/**
+					 * The list and rationale for each metacharacter comes from the same blogpost:
+					 * <a href=
+					 * "https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way">Everyone
+					 * quotes command line arguments the wrong way</a>
+					 */
+					boolean esc = switch (c) {
+						case '(', ')', '%', '!', '<', '>', '&', '|', // Metacharacters
+								'"', // Prevent cmd from interpreting quotes
+								'^' // The escape character itself
+								-> true;
+						default -> false;
+					};
+					if (esc) {
+						b.append('^');
+					}
+					b.append(c);
+				}
+				return b.toString();
+			}
+		};
+
+		/**
+		 * Get the probable shell for the given operating system
+		 * 
+		 * @param os the operating system
+		 * @return the shell, probably
+		 */
+		public static Shell forOs(OperatingSystem os) {
+			return switch (os) {
+				case OperatingSystem.WINDOWS -> Shell.WINDOWS;
+				default -> Shell.UNIX_SH;
+			};
 		}
-		return a;
+
+		/**
+		 * The local shell, probably
+		 */
+		public static final Shell LOCAL = forOs(Platform.CURRENT_PLATFORM.getOperatingSystem());
+
+		/**
+		 * Escape and/or quote a single command-line argument
+		 * 
+		 * @param a the argument
+		 * @return the argument formed in such a way that the shell will interpret it as the given
+		 *         string in one argument
+		 */
+		public abstract String generateArgument(String a);
 	}
 
 	public static String generateEnvBlock(Map<String, String> env) {

Ghidra/Framework/Pty/src/main/java/ghidra/pty/windows/ConPtyChild.java

@@ -28,6 +28,7 @@ import com.sun.jna.platform.win32.WinNT.HANDLE;
 
 import ghidra.pty.PtyChild;
 import ghidra.pty.ShellUtils;
+import ghidra.pty.ShellUtils.Shell;
 import ghidra.pty.local.LocalWindowsNativeProcessPtySession;
 import ghidra.pty.windows.jna.ConsoleApiNative;
 import ghidra.pty.windows.jna.ConsoleApiNative.STARTUPINFOEX;
@@ -75,12 +76,51 @@ public class ConPtyChild extends ConPtyEndpoint implements PtyChild {
 		return si;
 	}
 
+	private boolean isImplicitCmd(String[] args) {
+		if (args.length < 1) {
+			return false; // Really shouldn't, but let Windows decide how to fail
+		}
+		String lower0 = args[0].toLowerCase();
+		if (lower0.endsWith(".bat") || lower0.endsWith(".cmd")) {
+			return true;
+		}
+		/**
+		 * I'm on the fence about this. While it's pretty clear that invoking a .bat file, which
+		 * implicitly prefixes <code>cmd /c</code>, ought to escape the metacharacters, I'm not
+		 * certain about when a user explicitly invokes <code>cmd /c</code>. I think it should let
+		 * the metacharacters through, i.e., cmd should be permitted to do what the user probably
+		 * intended. Still, if someone using this API unwittingly puts the <code>cmd /c</code>
+		 * prefix on a user-supplied command line without sanitizing, they could create a
+		 * vulnerability.
+		 */
+		/*if (args.length < 2) {
+			return false;
+		}
+		if (!"/c".equals(args[1])) {
+			return false;
+		}
+		if ("cmd".equals(lower0) || "cmd.exe".equals(lower0) || lower0.endsWith("\\cmd") ||
+			lower0.endsWith("\\cmd.exe")) {
+			return true;
+		}*/
+		return false;
+	}
+
+	/**
+	 * {@inheritDoc}
+	 * <p>
+	 * <b>WARNING:</b> If arg[0], i.e., the application name, is a batch file, Windows will
+	 * automatically invoke it using <code>cmd /c</code>. This method is aware of this implicit
+	 * invocation and, upon detecting it, will appropriately escape cmd's metacharacters.
+	 * <em>However</em>, if a client explicitly invokes <code>cmd /c</code> with any part of the
+	 * command line formed from user-supplied arguments, IT MUST sanitize those arguments itself.
+	 * This can be achieved using {@link Shell#generateArgument(String)} of
+	 * {@link Shell#WINDOWS_CMD}.
+	 */
 	@Override
 	public LocalWindowsNativeProcessPtySession session(String[] args, Map<String, String> env,
 			File workingDirectory, Collection<TermMode> mode) throws IOException {
 		/**
-		 * TODO: How to incorporate environment into CreateProcess?
-		 * 
 		 * TODO: How to control local echo?
 		 */
 
@@ -92,7 +132,8 @@ public class ConPtyChild extends ConPtyEndpoint implements PtyChild {
 		STARTUPINFOEX si = prepareStartupInfo();
 		PROCESS_INFORMATION pi = new PROCESS_INFORMATION();
 
-		String commandLine = ShellUtils.generateLine(Arrays.asList(args));
+		Shell shell = isImplicitCmd(args) ? Shell.WINDOWS_CMD : Shell.WINDOWS;
+		String commandLine = ShellUtils.generateLine(Arrays.asList(args), shell);
 
 		if (!ConsoleApiNative.INSTANCE.CreateProcessW(
 			null /*lpApplicationName*/,