From 1541f3a78b3adccb32ab2848a0958571eb6760ae Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Tue, 23 Jun 2026 13:30:09 +0000 Subject: [PATCH] GP-6934: Implement OS/shell-dependent escaping. --- .../ghidra/dbg/jdi/rmi/jpda/JdiArguments.java | 4 +- .../launcher/AbstractTraceRmiLaunchOffer.java | 19 +- .../core/terminal/TerminalProvider.java | 4 +- .../src/main/java/ghidra/pty/ShellUtils.java | 217 +++++++++++++++++- .../java/ghidra/pty/windows/ConPtyChild.java | 47 +++- 5 files changed, 271 insertions(+), 20 deletions(-) diff --git a/Ghidra/Debug/Debugger-jpda/src/main/java/ghidra/dbg/jdi/rmi/jpda/JdiArguments.java b/Ghidra/Debug/Debugger-jpda/src/main/java/ghidra/dbg/jdi/rmi/jpda/JdiArguments.java index 64eceb37ae..964faf7ae7 100644 --- a/Ghidra/Debug/Debugger-jpda/src/main/java/ghidra/dbg/jdi/rmi/jpda/JdiArguments.java +++ b/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)); } } } diff --git a/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/launcher/AbstractTraceRmiLaunchOffer.java b/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/launcher/AbstractTraceRmiLaunchOffer.java index d002477d46..4cb6b0da88 100644 --- a/Ghidra/Debug/Debugger-rmi-trace/src/main/java/ghidra/app/plugin/core/debug/gui/tracermi/launcher/AbstractTraceRmiLaunchOffer.java +++ b/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 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(); diff --git a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/terminal/TerminalProvider.java b/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/terminal/TerminalProvider.java index b5e923dd86..0c4082445d 100644 --- a/Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/terminal/TerminalProvider.java +++ b/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 -> { }); diff --git a/Ghidra/Framework/Pty/src/main/java/ghidra/pty/ShellUtils.java b/Ghidra/Framework/Pty/src/main/java/ghidra/pty/ShellUtils.java index 9d853cc528..e3a2e6b3b8 100644 --- a/Ghidra/Framework/Pty/src/main/java/ghidra/pty/ShellUtils.java +++ b/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 + *

+ * LATER: This is meant to mimic UNIX- / C-style arguments, but that's probably not appropriate + * on all systems. We should either: + * + *

    + *
  1. Let the offer specify how @args ought to be treated.
    2. + *
  2. Let the header annotations for @args include some extra specifier.
    3. + *
+ * + * @param args the arguments as a single string + * @return the list of split arguments + */ public static List parseArgs(String args) { List argsList = new ArrayList<>(); StringBuilder curArg = new StringBuilder(); @@ -123,29 +140,207 @@ public class ShellUtils { return List.copyOf(copy); } - public static String generateLine(List args) { + public static String generateLine(List 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 + *

+ * 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 + *

+ * See CommandLineToArgvW + * function + */ + WINDOWS { + /** + * Derived from Everyone + * quotes command line arguments the wrong way + *

+ * 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. + *

+ * NOTE: 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., + * %1. 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 full command line is + * passed through the batch file, it should work as intended. However, if grabbing + * individual arguments, they cannot be reliably controlled. + *

+ * 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: + * Everyone + * quotes command line arguments the wrong way + */ + 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 env) { diff --git a/Ghidra/Framework/Pty/src/main/java/ghidra/pty/windows/ConPtyChild.java b/Ghidra/Framework/Pty/src/main/java/ghidra/pty/windows/ConPtyChild.java index cbccbc6a7d..3ab7de73a3 100644 --- a/Ghidra/Framework/Pty/src/main/java/ghidra/pty/windows/ConPtyChild.java +++ b/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 cmd /c, ought to escape the metacharacters, I'm not + * certain about when a user explicitly invokes cmd /c. 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 cmd /c + * 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} + *

+ * WARNING: If arg[0], i.e., the application name, is a batch file, Windows will + * automatically invoke it using cmd /c. This method is aware of this implicit + * invocation and, upon detecting it, will appropriately escape cmd's metacharacters. + * However, if a client explicitly invokes cmd /c 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 env, File workingDirectory, Collection 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*/,