diff --git a/src/strands_tools/use_computer.py b/src/strands_tools/use_computer.py index 45db9b42..52d6e900 100644 --- a/src/strands_tools/use_computer.py +++ b/src/strands_tools/use_computer.py @@ -24,6 +24,7 @@ import logging import os import platform +import re import subprocess import time from datetime import datetime @@ -642,6 +643,19 @@ def extract_text_from_image(image_path: str, min_confidence: float = 0.5) -> Lis return results +# The application name is passed to launch/focus mechanisms as a separate +# argument (never interpolated into a shell command or script body), so normal +# printable names cannot be parsed as code. As light defense-in-depth we still +# reject control characters and newlines, which have no place in an app name and +# could otherwise confuse logging or downstream tools. +_CONTROL_CHARS = re.compile(r"[\x00-\x1f\x7f]") + + +def _is_valid_app_name(app_name: str) -> bool: + """Return True if app_name is non-empty and contains no control characters.""" + return bool(app_name) and _CONTROL_CHARS.search(app_name) is None + + def open_application(app_name: str) -> str: """ Launch an application cross-platform. @@ -658,7 +672,8 @@ def open_application(app_name: str) -> str: str: Success or error message detailing the result of the operation. Platform Support: - - Windows: Uses the 'start' command + - Windows: Uses os.startfile, which launches via the shell association + without going through cmd.exe - macOS: Uses the 'open -a' command - Linux: Attempts to run app_name directly as a command """ @@ -682,10 +697,24 @@ def open_application(app_name: str) -> str: # Use mapped name if available, otherwise use original actual_app_name = app_mappings.get(app_name.lower(), app_name) + # Names not covered by the known mapping must be plain, printable names. + if app_name.lower() not in app_mappings and not _is_valid_app_name(app_name): + return f"Invalid application name: '{app_name}'" + try: if system == "windows": - result = subprocess.run(f"start {actual_app_name}", shell=True, capture_output=True, text=True) - elif system == "darwin": # macOS + # Use os.startfile rather than 'cmd /c start'. Even with shell=False, + # subprocess joins the argv into a command line via list2cmdline, which + # only quotes items containing whitespace. A spaceless payload such as + # "notepad&whoami" would therefore be handed to cmd.exe unquoted and the + # '&' re-parsed as a command separator, allowing command injection. + # os.startfile launches the app/document through the shell association + # API directly and never invokes cmd.exe, so metacharacters like + # & | < > ^ ( ) % stay inert. + os.startfile(actual_app_name) + return f"Launched {actual_app_name}" + + if system == "darwin": # macOS result = subprocess.run(["open", "-a", actual_app_name], capture_output=True, text=True) elif system == "linux": result = subprocess.run([actual_app_name.lower()], capture_output=True, text=True) @@ -741,14 +770,22 @@ def focus_application(app_name: str, timeout: float = 2.0) -> bool: system = platform.system().lower() start_time = time.time() + if not _is_valid_app_name(app_name): + logger.warning(f"Invalid application name for focus: {app_name}") + return False + try: if system == "darwin": # macOS - # Use AppleScript to bring app to front with timeout - script = f'tell application "{app_name}" to activate' + # Pass the application name as a script argument (available via 'argv') + # instead of interpolating it into the AppleScript source, so the name + # is treated as data rather than code. + script = "on run argv\n tell application (item 1 of argv) to activate\nend run" # Set up a process with timeout try: - result = subprocess.run(["osascript", "-e", script], check=True, capture_output=True, timeout=timeout) + result = subprocess.run( + ["osascript", "-e", script, app_name], check=True, capture_output=True, timeout=timeout + ) if result.returncode != 0: logger.warning(f"Focus application returned non-zero exit code: {result.returncode}") return False @@ -763,14 +800,16 @@ def focus_application(app_name: str, timeout: float = 2.0) -> bool: return False elif system == "windows": - # Use PowerShell to focus window + # Pass the application name as a script argument (available via 'args') + # instead of interpolating it into the PowerShell source, so the name + # is treated as data rather than code. script = ( - f"Add-Type -AssemblyName Microsoft.VisualBasic; " - f"[Microsoft.VisualBasic.Interaction]::AppActivate('{app_name}')" + "Add-Type -AssemblyName Microsoft.VisualBasic; " + "[Microsoft.VisualBasic.Interaction]::AppActivate($args[0])" ) try: result = subprocess.run( - ["powershell", "-Command", script], check=True, capture_output=True, timeout=timeout + ["powershell", "-Command", script, app_name], check=True, capture_output=True, timeout=timeout ) if result.returncode != 0: return False diff --git a/tests/test_use_computer.py b/tests/test_use_computer.py index a8049e36..9d3b7073 100644 --- a/tests/test_use_computer.py +++ b/tests/test_use_computer.py @@ -332,10 +332,72 @@ class TestApplicationManagement: @pytest.mark.parametrize("system", ["windows", "darwin", "linux"]) def test_open_application(self, system): - with patch("platform.system", return_value=system), patch("subprocess.run") as mock_run: + with ( + patch("platform.system", return_value=system), + patch("subprocess.run") as mock_run, + patch("os.startfile", create=True) as mock_startfile, + ): mock_run.return_value = MagicMock(returncode=0) result = open_application("test_app") assert "Launched" in result + if system == "windows": + mock_startfile.assert_called_once() + + def test_open_application_windows_uses_startfile(self): + """On Windows the app is launched via os.startfile, never through cmd.exe.""" + with ( + patch("platform.system", return_value="windows"), + patch("subprocess.run") as mock_run, + patch("os.startfile", create=True) as mock_startfile, + ): + open_application("notepad") + mock_startfile.assert_called_once_with("notepad") + # cmd.exe is never invoked, so there is no shell command line to reparse. + mock_run.assert_not_called() + + def test_open_application_injection_payload_passed_as_data(self): + """A spaceless command-chaining payload is launched literally, never reparsed by cmd.exe. + + With the previous 'cmd /c start "" ' approach, list2cmdline only quotes + argv items containing whitespace, so a spaceless payload like "notepad&whoami" + produced the command line `cmd /c start "" notepad&whoami` and cmd.exe reparsed + '&' as a command separator (command injection). os.startfile receives the exact + literal string and never invokes a command interpreter. + """ + payload = "notepad&whoami" + with ( + patch("platform.system", return_value="windows"), + patch("subprocess.run") as mock_run, + patch("os.startfile", create=True) as mock_startfile, + ): + open_application(payload) + # The exact literal payload is passed to startfile with no shell involved. + mock_startfile.assert_called_once_with(payload) + # cmd.exe is never spawned, so the payload cannot be split on '&'. + mock_run.assert_not_called() + + def test_open_application_rejects_control_characters(self): + """A name containing control characters is rejected before any launch attempt.""" + with ( + patch("platform.system", return_value="windows"), + patch("subprocess.run") as mock_run, + patch("os.startfile", create=True) as mock_startfile, + ): + result = open_application("notepad\nwhoami") + assert "Invalid application name" in result + mock_run.assert_not_called() + mock_startfile.assert_not_called() + + def test_open_application_accepts_plus_in_name(self): + """A legitimate name like 'C++ Builder' is accepted and launched literally.""" + with ( + patch("platform.system", return_value="windows"), + patch("subprocess.run") as mock_run, + patch("os.startfile", create=True) as mock_startfile, + ): + open_application("C++ Builder") + mock_startfile.assert_called_once_with("C++ Builder") + mock_run.assert_not_called() def test_close_application(self): mock_process = MagicMock() @@ -627,7 +689,15 @@ class TestFocusApplication: @pytest.mark.parametrize( "system,expected_command", [ - ("darwin", ["osascript", "-e", 'tell application "TestApp" to activate']), + ( + "darwin", + [ + "osascript", + "-e", + "on run argv\n tell application (item 1 of argv) to activate\nend run", + "TestApp", + ], + ), ( "windows", [ @@ -635,8 +705,9 @@ class TestFocusApplication: "-Command", ( "Add-Type -AssemblyName Microsoft.VisualBasic; " - "[Microsoft.VisualBasic.Interaction]::AppActivate('TestApp')" + "[Microsoft.VisualBasic.Interaction]::AppActivate($args[0])" ), + "TestApp", ], ), ("linux", ["wmctrl", "-a", "TestApp"]), @@ -675,6 +746,54 @@ def test_focus_application_unknown_system(self): result = focus_application("TestApp") assert result is False + def test_focus_application_injection_payload_passed_as_data_macos(self): + """An AppleScript injection payload reaches osascript only as inert trailing argv data.""" + from src.strands_tools.use_computer import focus_application + + payload = 'TestApp" & do shell script "echo PWNED > /tmp/pwned' + with patch("platform.system", return_value="darwin"), patch("subprocess.run") as mock_run, patch("time.sleep"): + mock_run.return_value = MagicMock(returncode=0) + focus_application(payload) + args = mock_run.call_args[0][0] + # The script body is the constant 'on run argv' template; the payload + # is only the trailing argv item, so it is never parsed as AppleScript. + assert args[2] == "on run argv\n tell application (item 1 of argv) to activate\nend run" + assert args[-1] == payload + assert "do shell script" not in args[2] + + def test_focus_application_rejects_control_characters_macos(self): + """A name containing control characters is rejected before any subprocess call.""" + from src.strands_tools.use_computer import focus_application + + with patch("platform.system", return_value="darwin"), patch("subprocess.run") as mock_run, patch("time.sleep"): + mock_run.return_value = MagicMock(returncode=0) + assert focus_application("TestApp\nactivate") is False + mock_run.assert_not_called() + + def test_focus_application_accepts_plus_in_name_macos(self): + """A legitimate name like 'C++ Builder' is accepted and passed as a trailing argv item.""" + from src.strands_tools.use_computer import focus_application + + with patch("platform.system", return_value="darwin"), patch("subprocess.run") as mock_run, patch("time.sleep"): + mock_run.return_value = MagicMock(returncode=0) + focus_application("C++ Builder") + args = mock_run.call_args[0][0] + assert args[-1] == "C++ Builder" + assert args[2] == "on run argv\n tell application (item 1 of argv) to activate\nend run" + + def test_focus_application_script_is_static_macos(self): + """A benign name reaches osascript as data, with a constant script body.""" + from src.strands_tools.use_computer import focus_application + + with patch("platform.system", return_value="darwin"), patch("subprocess.run") as mock_run, patch("time.sleep"): + mock_run.return_value = MagicMock(returncode=0) + focus_application("Safari") + args = mock_run.call_args[0][0] + # The app name is the trailing argv item, not interpolated into the script body. + assert args[-1] == "Safari" + assert "Safari" not in args[2] + assert "do shell script" not in args[2] + class TestHandleAnalyzeScreenshotPytesseract: """Tests for screenshot analysis handling"""