-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Fixed remote code execution vulnerability in Composio server API #1589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
tuple: (is_valid: bool, error_message: str) | ||
""" | ||
for dep in dependencies: | ||
if not re.match(r'^[a-zA-Z0-9_\-\.]+[a-zA-Z0-9_\-\.=<>]*$', dep): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security: The regex pattern for validating dependencies allows potentially dangerous package names with special characters that could be used for command injection. The pattern should be more restrictive.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
if not re.match(r'^[a-zA-Z0-9_\-\.]+[a-zA-Z0-9_\-\.=<>]*$', dep): | |
if not re.match(r'^[a-zA-Z0-9_\-\.]+[a-zA-Z0-9_\-\.=<>]*$', dep) or '..' in dep or '//' in dep or '\\' in dep: |
process = subprocess.run( | ||
args=["pip", "install", "--no-cache-dir", "--disable-pip-version-check", *request.dependencies], | ||
stdout=subprocess.PIPE, | ||
stderr=subprocess.PIPE, | ||
timeout=120, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness: The subprocess.run()
call for installing dependencies lacks the check=True
parameter, which means it won't raise an exception for non-zero exit codes and relies on manual checking.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
process = subprocess.run( | |
args=["pip", "install", "--no-cache-dir", "--disable-pip-version-check", *request.dependencies], | |
stdout=subprocess.PIPE, | |
stderr=subprocess.PIPE, | |
timeout=120, | |
) | |
process = subprocess.run( | |
args=["pip", "install", "--no-cache-dir", "--disable-pip-version-check", *request.dependencies], | |
stdout=subprocess.PIPE, | |
stderr=subprocess.PIPE, | |
timeout=120, | |
check=True | |
) |
import_regex = re.compile(r'^(?:from|import)\s+([a-zA-Z0-9_.]+)', re.MULTILINE) | ||
imports = import_regex.findall(content) | ||
|
||
for imp in imports: | ||
module_parts = imp.split('.')[0] | ||
if module_parts in DANGEROUS_IMPORTS: | ||
return False, f"Dangerous import detected: {imp}" | ||
|
||
# Check for dangerous patterns | ||
for pattern in DANGEROUS_PATTERNS: | ||
matches = re.search(pattern, content) | ||
if matches: | ||
return False, f"Dangerous code pattern detected: {matches.group(0)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security: The validate_tool_content()
function doesn't check for indirect imports using __import__
or other dynamic import techniques that could bypass the import validation.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
import_regex = re.compile(r'^(?:from|import)\s+([a-zA-Z0-9_.]+)', re.MULTILINE) | |
imports = import_regex.findall(content) | |
for imp in imports: | |
module_parts = imp.split('.')[0] | |
if module_parts in DANGEROUS_IMPORTS: | |
return False, f"Dangerous import detected: {imp}" | |
# Check for dangerous patterns | |
for pattern in DANGEROUS_PATTERNS: | |
matches = re.search(pattern, content) | |
if matches: | |
return False, f"Dangerous code pattern detected: {matches.group(0)}" | |
import_regex = re.compile(r'^(?:from|import)\s+([a-zA-Z0-9_.]+)', re.MULTILINE) | |
imports = import_regex.findall(content) | |
for imp in imports: | |
module_parts = imp.split('.')[0] | |
if module_parts in DANGEROUS_IMPORTS: | |
return False, f"Dangerous import detected: {imp}" | |
# Check for dangerous patterns | |
for pattern in DANGEROUS_PATTERNS: | |
matches = re.search(pattern, content) | |
if matches: | |
return False, f"Dangerous code pattern detected: {matches.group(0)}" | |
# Check for any attempt to use __builtins__ to access restricted functionality | |
if re.search(r'__builtins__', content): | |
return False, "Dangerous code pattern detected: __builtins__ access" |
if access_token is None: | ||
logger.warning("Access token not set. Using default authentication.") | ||
# Instead of bypassing authentication, use a default token | ||
if "x-api-key" not in request.headers or not request.headers["x-api-key"]: | ||
return Response( | ||
content=APIResponse[None]( | ||
data=None, | ||
error="Authentication required. Set ACCESS_TOKEN environment variable.", | ||
).model_dump_json(), | ||
status_code=401, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security: The auth_middleware
function doesn't properly handle the case when access_token
is None
and no API key is provided, potentially allowing unauthenticated access.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
if access_token is None: | |
logger.warning("Access token not set. Using default authentication.") | |
# Instead of bypassing authentication, use a default token | |
if "x-api-key" not in request.headers or not request.headers["x-api-key"]: | |
return Response( | |
content=APIResponse[None]( | |
data=None, | |
error="Authentication required. Set ACCESS_TOKEN environment variable.", | |
).model_dump_json(), | |
status_code=401, | |
) | |
if access_token is None: | |
logger.warning("Access token not set. Requiring authentication with any non-empty token.") | |
# Instead of bypassing authentication, require any non-empty token | |
if "x-api-key" not in request.headers or not request.headers["x-api-key"]: | |
return Response( | |
content=APIResponse[None]( | |
data=None, | |
error="Authentication required. Set ACCESS_TOKEN environment variable.", | |
).model_dump_json(), | |
status_code=401, | |
) |
requested_path = Path(file) | ||
|
||
if requested_path.is_absolute(): | ||
requested_abs_path = requested_path.resolve() | ||
base_abs_path = base_download_dir | ||
|
||
if not str(requested_abs_path).startswith(str(base_abs_path)): | ||
logger.warning(f"Path traversal attempt: {requested_abs_path} is outside {base_abs_path}") | ||
raise HTTPException( | ||
status_code=403, | ||
detail="Access denied: Cannot access files outside the workspace directory" | ||
) | ||
|
||
safe_path = requested_abs_path | ||
else: | ||
safe_path = (base_download_dir / requested_path).resolve() | ||
|
||
if not str(safe_path).startswith(str(base_download_dir)): | ||
logger.warning(f"Path traversal attempt with relative path: {safe_path} is outside {base_download_dir}") | ||
raise HTTPException( | ||
status_code=403, | ||
detail="Access denied: Cannot access files outside the workspace directory" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security: The file path validation in the download endpoint doesn't handle symbolic links, which could allow path traversal attacks if a symbolic link points outside the base directory.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
requested_path = Path(file) | |
if requested_path.is_absolute(): | |
requested_abs_path = requested_path.resolve() | |
base_abs_path = base_download_dir | |
if not str(requested_abs_path).startswith(str(base_abs_path)): | |
logger.warning(f"Path traversal attempt: {requested_abs_path} is outside {base_abs_path}") | |
raise HTTPException( | |
status_code=403, | |
detail="Access denied: Cannot access files outside the workspace directory" | |
) | |
safe_path = requested_abs_path | |
else: | |
safe_path = (base_download_dir / requested_path).resolve() | |
if not str(safe_path).startswith(str(base_download_dir)): | |
logger.warning(f"Path traversal attempt with relative path: {safe_path} is outside {base_download_dir}") | |
raise HTTPException( | |
status_code=403, | |
detail="Access denied: Cannot access files outside the workspace directory" | |
) | |
requested_path = Path(file) | |
# Reject paths with suspicious patterns | |
if '..' in str(requested_path) or '//' in str(requested_path) or '\\\\' in str(requested_path): | |
logger.warning(f"Suspicious path pattern detected: {requested_path}") | |
raise HTTPException( | |
status_code=403, | |
detail="Access denied: Path contains suspicious patterns" | |
) | |
if requested_path.is_absolute(): | |
requested_abs_path = requested_path.resolve() | |
base_abs_path = base_download_dir | |
if not str(requested_abs_path).startswith(str(base_abs_path)): | |
logger.warning(f"Path traversal attempt: {requested_abs_path} is outside {base_abs_path}") | |
raise HTTPException( | |
status_code=403, | |
detail="Access denied: Cannot access files outside the workspace directory" | |
) | |
safe_path = requested_abs_path | |
else: | |
safe_path = (base_download_dir / requested_path).resolve() | |
if not str(safe_path).startswith(str(base_download_dir)): | |
logger.warning(f"Path traversal attempt with relative path: {safe_path} is outside {base_download_dir}") | |
raise HTTPException( | |
status_code=403, | |
detail="Access denied: Cannot access files outside the workspace directory" | |
) |
Hi @angrybayblade, @kaavee315 just wanted to check in on this too. |
Version: 0.7.16-rc1
Remote Code Execution in Composio Server API
Description
A remote code execution (RCE) vulnerability has been identified in Composio API Server version 0.7.16-rc1 and potentially earlier versions. The vulnerability exists in the
/api/tools
endpoint, which allows attackers to upload and execute arbitrary Python code on the server without proper validation or sandboxing.The vulnerability is particularly severe because:
ACCESS_TOKEN
environment variable is not setThis vulnerability allows remote attackers to execute arbitrary code with the privileges of the server process, potentially leading to complete system compromise.
Source-Sink Analysis
Source: User-controlled input via the
content
field in theToolUploadRequest
object sent to the/api/tools
endpoint.Sink: The code path where the user input is written to a file and executed via
importlib.import_module()
.Vulnerable Code Path:
The vulnerability is located in
python/composio/server/api.py
in the_upload_workspace_tools
function:The
ToolUploadRequest
class is defined as:Authentication Bypass:
The authentication middleware in
api.py
contains a critical flaw that bypasses authentication whenACCESS_TOKEN
is not set:Default Exposure:
The Docker deployment in
python/dockerfiles/entrypoint.sh
exposes the server to all network interfaces:Proof of Concept
This proof-of-concept demonstrates remote code execution via the vulnerable endpoint:
Step 1: Create a payload file
Step 2: Send the exploit request
Impact
ACCESS_TOKEN
is not set, the vulnerability can be exploited without any authentication.Contact
If you have any queries regarding this bug report, send an email to [email protected].