Add low integrity sandbox behind --enable-sandbox flag #8351

deepanjanroy · 2025-05-30T23:28:52Z

Design

You can turn on the sandbox using --enable-sandbox, only on windows.
It checks for correct directory permissions at startup
If using the default directory structure, it offers to set up the permissions automatically. Users get an elevation request (see pics below). The elevated privileges only apply to the setup_sandbox_permissions.bat file. This should be a one time thing.
The directories user, custom_nodes, and output are lowered in integrity level, along with a new directory write-permitted.
- Making custom_nodes low integrity means any custom nodes that want to write to its own directory continues functioning. Tested with comfyui-manager. It also means if there is a binary inside a custom_node it will always run with low integrity.
- The temp directory is moved inside the new write-permitted folder because we have code that deletes and recreated the entire directory.
- If we ever want to create new places we want to write to, we can just put it inside write-permitted to keep things simple.

Manual tests performed:

The default comfy workflow can generate outputs
ComfyUI manager seems to work fine although I haven't tested it too hard
User settings persist across update
Works even if ComfyUI is in D: drive
If --enable-sandbox is not passed, it does not require pywin32 (so it also should not break non-windows platforms)

Work to do in follow up PRs:

Support overriding directories (e.g. --output-directory)
Enable it by default / add the --enable-sandbox to bat file for portable

(^ this is a photo because it's hard to take a screenshot of the elevation screen)

github-actions · 2025-06-11T17:17:10Z

(Automated Bot Message) CI Tests are running, you can view the results at https://ci.comfy.org/?branch=8351%2Fmerge

comfyanonymous · 2025-06-11T23:49:50Z

.ci/update_windows/update.py

    pass
+
+
+print("Copying windows base files")


This block should go in a separate PR.

Done. Also note that I pulled the subprocess import to the top of the file - hope that's ok.

maludwig

I am unclear about the purpose of sandbox mode, but currently, it seems that if a custom node contained malicious code, it could overwrite the code of every other custom node by writing over the .py files. The permissions script will not work if there are spaces in the comfyui install path.

maludwig · 2025-06-14T22:52:16Z

sandbox/windows_sandbox.py

+LOW_INTEGRITY_SID_STRING = "S-1-16-4096"
+
+# Use absolute path to prevent command injection
+ICACLS_PATH = r"C:\Windows\System32\icacls.exe"


This assumes that Windows is installed there, recommend using:

ICACLS_PATH = os.path.join(win32api.GetSystemDirectory(), "icacls.exe")

maludwig · 2025-06-14T22:53:41Z

sandbox/windows_sandbox.py

+import folder_paths
+
+
+LOW_INTEGRITY_SID_STRING = "S-1-16-4096"


Consider adding a comment here explaining what "S-1-16-4096" means.

maludwig · 2025-06-14T23:00:26Z

sandbox/windows_sandbox.py

@@ -0,0 +1,172 @@
+import logging


Consider adding a docstring to the top of the file to explain the file, since it's pretty far off in the weeds of win32. Perhaps something like:

""" Sandboxing Utilities for Low-Integrity Process Execution on Windows =================================================================== This module enables a sandboxed execution environment by lowering the integrity level of the current process to "Low", which restricts file system and registry access for enhanced security. It ensures that specific directories are writable even under this constrained execution mode. """

maludwig · 2025-06-14T23:01:31Z

sandbox/windows_sandbox.py

+ICACLS_PATH = r"C:\Windows\System32\icacls.exe"
+
+
+def set_process_integrity_level_to_low():


Consider docstrings for each of these, since they're modestly advances win32, like:

""" Set the current process to run at a low integrity level. This restricts the process's permissions, creating a sandboxed environment to limit potential damage if compromised. """

maludwig · 2025-06-14T23:14:20Z

sandbox/windows_sandbox.py

+    Checks if an icacls output indicates that the path is writable by low
+    integrity processes.
+
+    Note that currently it is a bit of a crude check - it is possible for


I would elaborate on when this fails, if you know offhand.

maludwig · 2025-06-14T23:58:20Z

.gitignore

 openapi.yaml
 filtered-openapi.yaml
 uv.lock
+/write-permitted/


Presumably also remove /temp/, if you're moving it. I wouldn't personally remove temp, see my later comment in folder_paths.py

maludwig · 2025-06-14T23:59:59Z

comfy/cli_args.py

+    "--enable-sandbox",
+    default=False,
+    action="store_true",
+    help="Enable sandbox mode.",


I would elaborate in this help entry about what use case sandbox mode is intended for. Some people might assume that it would be a security feature that allows them to safely execute malicious code. Consider a more specific name, like: "--limit-write-access" or something, rather than "sandbox".

maludwig · 2025-06-15T00:01:45Z

main.py

+        # Note: If we ever support custom directories, we should warn users if
+        # the directories are in a senstive location (e.g. a high level
+        # directory like C:\ or the user's home directory).
+        raise Exception("Sandbox mode is not supported when using --output-directory, "


This is a really good note. Setting the user's entire home directory to Low would be horrendous.

maludwig · 2025-06-15T00:02:36Z

main.py

+        from sandbox import windows_sandbox
+        try_enable_sandbox()
+    else:
+        logging.warning("Sandbox mode is not supported on non-windows platforms."


Shouldn't this be an error?

maludwig · 2025-06-15T00:04:12Z

sandbox/windows_sandbox.py

+    proc_info = shell.ShellExecuteEx(**execute_info)
+    hProcess = proc_info["hProcess"]
+
+    # Setup script should less than a second. Time out at 10 seconds.


If the user hums and has at the UAC prompt for a moment, or the ACLs take a while to apply, then do you really want to quit halfway through? I would consider a much longer timeout.

Baughn · 2025-12-07T13:09:03Z

Related: #11013 kinda does this on Linux. I just spotted your PR.

deepanjanroy requested review from Kosinkadink, christian-byrne, comfyanonymous, ltdrdata, pythongosssss, robinjhuang, webfiltered and yoland68 as code owners May 30, 2025 23:28

deepanjanroy force-pushed the in-process-sandbox branch from 5a8067e to 7fdb4b7 Compare June 10, 2025 19:11

yoland68 added Core Core team dependency Run-CI-Test This is an administrative label to tell the CI to run full automatic testing on this PR now. labels Jun 11, 2025

comfyanonymous reviewed Jun 11, 2025

View reviewed changes

deepanjanroy added 13 commits June 13, 2025 17:31

Add sandbox behind --enable-sandbox flag

03aaf1b

Clean up

0f51c16

Use correct CloseHandle

15c8efa

Guard pywin32 imports

46af9a9

Make custom_nodes write-permitted

d5121f1

Move windows only requirements out to separate file

8207d00

Temporary updater changes for testing

96798c0

Copy windows base files during update

5692036

Clean up update script

7c6b045

Remove unused pytest

f6547ee

Remove unused line

ea53b74

Warn about no sandbox on non-windows

fcd53c4

Remove bat updating code

fc47bd2

deepanjanroy force-pushed the in-process-sandbox branch from 14d75bf to fc47bd2 Compare June 13, 2025 21:33

Remove blank line (and rebase)

67e3bee

maludwig suggested changes Jun 15, 2025

View reviewed changes

deepanjanroy mentioned this pull request Jun 18, 2025

Support pip environment markers Comfy-Org/ComfyUI-Manager#1943

Open

		ICACLS_PATH = r"C:\Windows\System32\icacls.exe"


		def set_process_integrity_level_to_low():

Add low integrity sandbox behind --enable-sandbox flag #8351

Are you sure you want to change the base?

Add low integrity sandbox behind --enable-sandbox flag #8351

Uh oh!

Conversation

deepanjanroy commented May 30, 2025

Uh oh!

github-actions bot commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maludwig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Baughn commented Dec 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants