Network Blocking for Sandbox Code Execution#1071
Conversation
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
📝 WalkthroughWalkthroughThis pull request implements a two-layer defense-in-depth network blocking mechanism for sandboxed execution environments. Layer 1 uses a preloadable C shared library to intercept socket system calls at the OS level, while Layer 2 patches Python socket modules to block network operations at the application level, allowing only Unix domain sockets and local operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
nemo_skills/code_execution/local_sandbox/local_sandbox_server.py (1)
86-99: Consider addingAF_LOCALcheck for consistency with the C implementation.The C layer explicitly checks both
AF_UNIXandAF_LOCAL, but the Python patch only checksAF_UNIX. WhileAF_LOCALis typically an alias forAF_UNIXon Linux, adding it explicitly ensures consistency and guards against edge cases.class BlockedSocket(_socket_module.socket): def __init__(self, family=-1, type=-1, proto=-1, fileno=None): # Allow Unix domain sockets (needed for IPC) - if family in (_socket_module.AF_UNIX,): + if family in (_socket_module.AF_UNIX, getattr(_socket_module, 'AF_LOCAL', _socket_module.AF_UNIX)): super().__init__(family, type, proto, fileno)tests/test_sandbox_network_blocking.py (4)
34-34: Hardcoded image name may cause issues in different environments.Consider making the image name configurable via environment variable for flexibility in CI/CD pipelines.
+import os + -SANDBOX_IMAGE = "locally-built-sandbox:latest" +SANDBOX_IMAGE = os.getenv("SANDBOX_TEST_IMAGE", "locally-built-sandbox:latest")
52-63: Consider adding more specific error handling for startup diagnostics.While the bare
exceptis acceptable for polling, capturing the exception type during the final failure could aid debugging. Also, thesandboxinstance created during health checking should be closed to avoid resource leaks.for _ in range(60): try: sandbox = LocalSandbox(host="127.0.0.1", port=str(port)) if sandbox._check_ready(timeout=5): + await sandbox.close() # Clean up http session break - except Exception: - pass + except Exception as e: + last_error = e time.sleep(2) else: + logs = container.logs().decode('utf-8', errors='replace')[-2000:] container.remove(force=True) - pytest.fail("Sandbox failed to start") + pytest.fail(f"Sandbox failed to start. Last error: {last_error}\nLogs: {logs}")
107-118: Error message assertion may be fragile.The assertion on line 118 checks for "Network is unreachable" in
stdout, but the error traceback fromrequeststypically appears instderror within the traceback output. Consider checking both outputs or removing this secondary assertion.result, _ = await sandbox.execute_code(code, language="ipython") assert "STATUS:" not in result.get("stdout", ""), "requests library should be blocked" - assert "Network is unreachable" in result.get("stdout", ""), "Should show blocking error" + # Error may appear in stdout (traceback) or stderr + combined = result.get("stdout", "") + result.get("stderr", "") + assert "Network is unreachable" in combined or "OSError" in combined, "Should show blocking error"
83-94: Consider cleaning up sandbox resources after each test.Each test creates a
LocalSandboxinstance that holds an HTTP session. While not critical for tests, explicitly closing these would be cleaner.async def test_direct_socket_blocked(self, blocked_sandbox): """LLM tries: socket.socket(AF_INET, SOCK_STREAM)""" sandbox = LocalSandbox(host="127.0.0.1", port=str(blocked_sandbox)) - code = """ + try: + code = """ import socket s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) print("NETWORK_ALLOWED") """ - result, _ = await sandbox.execute_code(code, language="ipython") - assert "NETWORK_ALLOWED" not in result.get("stdout", ""), "Direct socket should be blocked" + result, _ = await sandbox.execute_code(code, language="ipython") + assert "NETWORK_ALLOWED" not in result.get("stdout", ""), "Direct socket should be blocked" + finally: + await sandbox.close()Alternatively, consider using a pytest fixture to manage
LocalSandboxlifecycle.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dockerfiles/Dockerfile.sandbox(1 hunks)dockerfiles/sandbox/block_network.c(1 hunks)dockerfiles/sandbox/start-with-nginx.sh(1 hunks)nemo_skills/code_execution/local_sandbox/local_sandbox_server.py(1 hunks)tests/test_sandbox_network_blocking.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
nemo_skills/code_execution/local_sandbox/local_sandbox_server.py (1)
dockerfiles/sandbox/block_network.c (1)
socket(41-61)
tests/test_sandbox_network_blocking.py (2)
nemo_skills/code_execution/sandbox.py (2)
LocalSandbox(279-353)_check_ready(257-272)nemo_skills/pipeline/utils/server.py (1)
get_free_port(43-59)
🪛 Clang (14.0.6)
dockerfiles/sandbox/block_network.c
[error] 34-34: 'stddef.h' file not found
(clang-diagnostic-error)
🪛 Ruff (0.14.7)
tests/test_sandbox_network_blocking.py
58-59: try-except-pass detected, consider logging the exception
(S110)
58-58: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pre-commit
- GitHub Check: unit-tests
🔇 Additional comments (6)
dockerfiles/sandbox/block_network.c (2)
1-31: Well-documented header with clear usage instructions.The header documentation clearly explains the purpose, defense-in-depth rationale, and build instructions. The use of
/etc/ld.so.preloadinstead ofLD_PRELOADis correctly justified as it prevents bypass via environment variable clearing.
40-61: Socket interception logic is correct.The implementation properly blocks IPv4/IPv6 internet sockets while allowing Unix domain sockets for local IPC. The socket() override successfully prevents the documented attack vectors (direct socket creation, requests/urllib, curl/wget subprocess calls). The static pointer initialization has a benign race condition in multi-threaded contexts, but since
dlsymreturns the same pointer each time, this only results in redundant calls—no functional issue.The permissive fallthrough allowing
AF_NETLINK,AF_PACKET, and other socket types poses no practical risk; user code executes without elevated privileges needed to create these kernel-level sockets, and the threat model focuses on Internet exfiltration (which is effectively blocked).dockerfiles/sandbox/start-with-nginx.sh (1)
289-301: Correct timing for enabling network blocking.The network blocking is correctly enabled after nginx/uwsgi start, ensuring the API server's sockets are created before the preload takes effect. The warning for a missing library is appropriate.
nemo_skills/code_execution/local_sandbox/local_sandbox_server.py (1)
43-74: Excellent documentation of defense-in-depth strategy.The table clearly illustrates why both layers are necessary—the Python patch handles forked IPython processes while the ld.so.preload layer handles exec'd subprocesses. This is well-thought-out documentation that will help maintainers understand the security architecture.
dockerfiles/Dockerfile.sandbox (1)
107-110: Build step is correct.The compilation flags (
-shared -fPIC -ldl) are appropriate for creating a preloadable shared library. Installing to/usr/libensures the library is in a standard location accessible to the dynamic linker.tests/test_sandbox_network_blocking.py (1)
196-228: Excellent coverage of allowed operations.This test properly validates that the network blocking doesn't break legitimate functionality—math operations, file I/O, and Unix domain sockets (critical for IPC) all work correctly. This is essential for ensuring the blocking is surgical rather than overly broad.
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com> Signed-off-by: mmkrtchyan <mmkrtchyan@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com>
Signed-off-by: George Armstrong <georgea@nvidia.com> Signed-off-by: dgitman <dgitman@nvidia.com>
Summary
This PR adds the ability to block outbound network access for code executed in the sandbox, preventing LLM-generated code from making unauthorized network requests (e.g., calling external APIs).
Usage
Enable network blocking by setting the environment variable when starting the sandbox container:
Implementation
Uses an approach with two complementary layers:
Layer 1: C Library Interception (
libblock_network.so)socket()syscalls at the C library level via/etc/ld.so.preloadLayer 2: Python Socket Patch
socket.socketand_socket.socketin the IPython shell workerBoth layers are required because neither alone covers all network access.
What's Blocked
socket.socket(),_socket.socket())requests,urllib)curl,wget)env={}to clear environment)What Still Works