fix(security): patch 3 critical vulnerabilities (Antiproof report)#24336
fix(security): patch 3 critical vulnerabilities (Antiproof report)#24336Harshit28j wants to merge 3 commits intoBerriAI:mainfrom
Conversation
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…/UC Berkeley 1. Custom Code Guardrail RCE: Add PROXY_ADMIN role check to /apply_guardrail endpoint and block frame traversal patterns (cr_frame, gi_frame, f_back, f_globals, etc.) in the code validator to prevent sandbox escape. 2. Skills Sandbox Path Traversal: Sanitize ZIP entry names in extract_all_files() to reject .. and absolute paths, and add realpath containment check in sandbox_executor before writing files. 3. OIDC Arbitrary File Read: Add path validation blocking sensitive directories (/etc, /root, /proc, /sys, /dev) in the OIDC file provider, and restrict /health/test_connection endpoint to admin users only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Greptile SummaryThis PR patches three critical/high security vulnerabilities reported by Antiproof (UC Berkeley): a custom code guardrail RCE via Python frame-traversal, a Skills sandbox ZIP path traversal, and an OIDC arbitrary file read. The intent is sound and the test coverage is strong, but several of the new defences contain their own correctness gaps that could undermine the fixes. Key changes:
Issues found:
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| litellm/secret_managers/main.py | Adds OIDC file-path validation (_validate_oidc_file_path) with a denylist of sensitive directories, but has two security issues: the LITELLM_ALLOWED_OIDC_DIRS allowlist is bypassable via startswith without a trailing separator, and relative paths are not blocked. |
| litellm/proxy/guardrails/guardrail_endpoints.py | Adds PROXY_ADMIN role check to /apply_guardrail; correctly fixes the RCE vector but is a breaking change for existing non-admin callers without a backwards-compatible feature flag. |
| litellm/proxy/health_endpoints/_health_endpoints.py | Restricts /health/test_connection to PROXY_ADMIN; fixes the OIDC arbitrary-read path but is backwards-incompatible without a feature flag for existing non-admin users. |
| litellm/llms/litellm_proxy/skills/sandbox_executor.py | Adds realpath containment check before writing ZIP-extracted files; the startswith comparison lacks a trailing path separator and is logically unsound, though practically safe due to random tmpdir names. |
| litellm/llms/litellm_proxy/skills/prompt_injection.py | ZIP entry path traversal validation is correctly implemented with both a split-component .. check and a normpath double-check; logic is sound. |
| litellm/proxy/guardrails/guardrail_hooks/custom_code/code_validator.py | 12 new frame-traversal regex patterns added to FORBIDDEN_PATTERNS; blocks major escape vectors but misses several other co_* code-object attributes (co_code, co_cellvars, co_freevars, co_filename). |
| tests/litellm/proxy/guardrails/test_custom_code_security.py | 7 new unit tests for frame-traversal patterns; all are pure mock/unit tests with no network calls, correctly extending existing coverage. |
| tests/llm_translation/test_skills_security.py | New test file covering ZIP path traversal via mocks; no real network calls, correctly validates the extraction logic. |
| tests/test_litellm/secret_managers/test_secret_managers_main.py | 8 new OIDC path validation tests covering blocked system directories and a valid tmp-path round-trip; no network calls; good coverage of the happy/sad paths. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B{Endpoint?}
B -->|POST /apply_guardrail| C{PROXY_ADMIN role?}
C -->|No| D[403 Forbidden]
C -->|Yes| E[validate_custom_code against FORBIDDEN_PATTERNS]
E --> F{Pattern match?}
F -->|Yes| G[CustomCodeValidationError]
F -->|No| H[Execute Guardrail]
B -->|POST /health/test_connection| I{PROXY_ADMIN role?}
I -->|No| J[403 Forbidden]
I -->|Yes| K[Test Model Connection]
B -->|OIDC file read via get_secret| L[_validate_oidc_file_path]
L --> M{Blocked prefix?}
M -->|Yes| N[ValueError: path blocked]
M -->|No| O{LITELLM_ALLOWED_OIDC_DIRS set?}
O -->|Yes| P{startswith allowed_dir?}
P -->|No| Q[ValueError: not in allowlist]
P -->|"Yes (bypass risk: no trailing sep)"| R[open and read file]
O -->|No| R
B -->|Skill ZIP via extract_all_files| S[Check for .. and abs path]
S --> T{Path traversal?}
T -->|Yes| U[Skip entry]
T -->|No| V[Stage file in tmpdir]
V --> W{realpath startswith tmpdir?}
W -->|"No"| X[ValueError: traversal detected]
W -->|"Yes (no trailing sep)"| Y[Write and copy to sandbox]
Last reviewed commit: "Merge branch 'main' ..."
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN: | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail="Admin access required to apply guardrails", | ||
| ) |
There was a problem hiding this comment.
Restricting /apply_guardrail to PROXY_ADMIN only is a backwards-incompatible change. Any non-admin users who were legitimately calling this endpoint (e.g. internal tooling, team members testing guardrails) will now get a 403 with no opt-out. Per the project's policy on backwards compatibility, breaking changes should be gated behind a user-controlled flag rather than applied unconditionally.
Consider guarding this behind an env/config flag, e.g.:
if litellm.require_admin_for_apply_guardrail and user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN:
raise HTTPException(
status_code=403,
detail="Admin access required to apply guardrails",
)This allows existing deployments to opt-in at their own pace while the default can be True for new installs.
Rule Used: What: avoid backwards-incompatible changes without... (source)
| if user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN: | ||
| raise HTTPException( | ||
| status_code=403, | ||
| detail="Admin access required to test model connections", | ||
| ) |
There was a problem hiding this comment.
Same backwards-compatibility concern as /apply_guardrail: /health/test_connection was previously accessible to any authenticated user. Restricting it unconditionally to PROXY_ADMIN will break non-admin scripts and CI pipelines that relied on it for health-checking credentials. This should be gated behind a flag per the project's backwards-compatibility policy.
Rule Used: What: avoid backwards-incompatible changes without... (source)
| allowed_dirs_env = os.getenv("LITELLM_ALLOWED_OIDC_DIRS") | ||
| if allowed_dirs_env: | ||
| allowed = [os.path.realpath(d.strip()) for d in allowed_dirs_env.split(",")] | ||
| if not any(resolved.startswith(d) for d in allowed): | ||
| raise ValueError( | ||
| f"OIDC file path {resolved} not in allowed directories" | ||
| ) |
There was a problem hiding this comment.
startswith allowlist bypass — missing path separator
resolved.startswith(d) can be fooled when an allowed directory name is a prefix of an attacker-controlled directory name. For example, if LITELLM_ALLOWED_OIDC_DIRS=/var/run/secrets, the path /var/run/secrets.evil/token passes the check because "/var/run/secrets.evil/token".startswith("/var/run/secrets") is True.
Each entry should be normalised to end with / before the comparison:
allowed = [
(os.path.realpath(d.strip()).rstrip("/") + "/")
for d in allowed_dirs_env.split(",")
]
if not any(resolved.startswith(d) for d in allowed):
raise ValueError(
f"OIDC file path {resolved} not in allowed directories"
)| real_path = os.path.realpath(local_path) | ||
| if not real_path.startswith(os.path.realpath(tmpdir)): | ||
| raise ValueError( | ||
| f"Path traversal detected in skill file: {path}" | ||
| ) |
There was a problem hiding this comment.
startswith containment check missing path separator
real_path.startswith(os.path.realpath(tmpdir)) is logically flawed in the same way as the OIDC allowlist: if tmpdir were /tmp/abc, a resolved path of /tmp/abcdef/evil would pass the check. In practice, tempfile.TemporaryDirectory() generates long random suffixes that make an accidental prefix collision very unlikely, but relying on that is fragile. Append a trailing separator to make the boundary check explicit:
safe_root = os.path.realpath(tmpdir) + os.sep
if not real_path.startswith(safe_root):
raise ValueError(
f"Path traversal detected in skill file: {path}"
)| def _validate_oidc_file_path(file_path: str) -> None: | ||
| """Validate that an OIDC token file path is safe to read.""" | ||
| # Normalize the path to resolve .. components | ||
| normalized = os.path.normpath(file_path) | ||
| resolved = os.path.realpath(file_path) | ||
|
|
||
| # Check both normalized and resolved paths against blocked prefixes | ||
| # (resolved handles symlinks, normalized handles cases where file doesn't exist yet) | ||
| for prefix in _OIDC_BLOCKED_PATH_PREFIXES: | ||
| if normalized.startswith(prefix) or resolved.startswith(prefix): | ||
| raise ValueError( | ||
| f"OIDC file path not allowed: reading from {prefix} is blocked for security" | ||
| ) | ||
| # Also check /private variants (macOS) | ||
| private_prefix = f"/private{prefix}" | ||
| if resolved.startswith(private_prefix): | ||
| raise ValueError( | ||
| f"OIDC file path not allowed: reading from {prefix} is blocked for security" | ||
| ) |
There was a problem hiding this comment.
_validate_oidc_file_path only blocks paths whose normpath/realpath starts with one of the hardcoded prefixes. All of those prefixes begin with /. A relative path such as etc/passwd produces:
os.path.normpath("etc/passwd")→etc/passwd(no leading/, passes every check)os.path.realpath("etc/passwd")→<CWD>/etc/passwd(only blocked ifCWD == /)
If the OIDC file path is ultimately sourced from user-supplied input that does not enforce an absolute path, relative paths can bypass the denylist entirely. At minimum, consider rejecting any path that does not start with / after normalization, or resolving the path against a known safe root before checking.
| (r"\.cr_frame", "coroutine frame access is not allowed"), | ||
| (r"\.gi_frame", "generator frame access is not allowed"), | ||
| (r"\.ag_frame", "async generator frame access is not allowed"), | ||
| (r"\.f_back", "frame traversal via f_back is not allowed"), | ||
| (r"\.f_globals", "frame globals access is not allowed"), | ||
| (r"\.f_locals", "frame locals access is not allowed"), | ||
| (r"\.f_code", "frame code access is not allowed"), | ||
| (r"\.f_builtins", "frame builtins access is not allowed"), | ||
| (r"\bcurrentframe\b", "currentframe() is not allowed"), | ||
| (r"\b_getframe\b", "sys._getframe() is not allowed"), | ||
| (r"\.co_consts", "code object constants access is not allowed"), | ||
| (r"\.co_names", "code object names access is not allowed"), |
There was a problem hiding this comment.
Several additional co_* attributes are useful for sandbox escape and are not yet blocked:
co_code— raw bytecode, can be used to reconstruct and re-execute codeco_cellvars/co_freevars— expose closure cell contentsco_filename— discloses server file-system paths
Consider extending the list:
(r"\.co_code\b", "code object bytecode access is not allowed"),
(r"\.co_cellvars\b", "code object cell variables access is not allowed"),
(r"\.co_freevars\b", "code object free variables access is not allowed"),
(r"\.co_filename\b", "code object filename access is not allowed"),
themavik
left a comment
themavik
left a comment
There was a problem hiding this comment.
Solid hardening across multiple vectors — ZIP traversal, frame access, OIDC path validation, and RBAC checks.
One gap in the code validator: string-level regex patterns like \.cr_frame can be bypassed with getattr(obj, 'cr_' + 'frame') or vars() tricks. The validator would need AST-level analysis to catch those. Not a blocker since this is defense-in-depth on top of the sandbox, but worth noting.
The LITELLM_ALLOWED_OIDC_DIRS escape hatch is a nice touch — avoids breaking Kubernetes token mounts in /etc/kubernetes/.
3 participants
Summary
Fixes three security vulnerabilities reported by Antiproof (UC Berkeley) on March 7, 2026:
PROXY_ADMINrole check to/apply_guardrailendpoint and blocked 12 frame traversal patterns (cr_frame,gi_frame,f_back,f_globals, etc.) in the code validator to prevent sandbox escape.extract_all_files()to reject..and absolute paths, and addedrealpathcontainment check insandbox_executorbefore writing files to disk._validate_oidc_file_path()blocking reads from sensitive directories (/etc/,/root/,/proc/,/sys/,/dev/) and restricted/health/test_connectionendpoint to admin users only.Test plan
🤖 Generated with Claude Code