fix(security): 2 improvements across 2 files#1201
Conversation
- Security: Arbitrary Code Execution via Dynamic Task Import - Security: Unsafe YAML Loading in dirsearch Task Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: Arbitrary Code Execution via Dynamic Task Import - Security: Unsafe YAML Loading in dirsearch Task Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
WalkthroughTwo targeted fixes: ChangesTask Import Refactor and Dirsearch Path Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@secator/tasks/__init__.py`:
- Around line 10-14: The importlib.import_module() call in the loop only loads
the module into sys.modules but does not bind the task class to the package
namespace. To fix this, capture the return value of importlib.import_module()
and extract the class from the imported module object using getattr() with the
class name, then bind it to the current module's namespace using
globals()[cls.__name__] = imported_class so that direct imports like `from
secator.tasks import <classname>` will function correctly instead of raising
ImportError.
In `@secator/tasks/dirsearch.py`:
- Around line 79-83: Replace the string-based path containment check in the
validation block (where output_path and reports_folder are compared using
startswith) with a try-except pattern using the relative_to() method. Wrap the
relative_to() call in a try-except block to catch ValueError when output_path is
not actually within reports_folder, then yield the Error if the exception
occurs. This approach properly validates path containment and prevents directory
traversal attacks, regardless of similar path name prefixes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba8a5e17-b8b1-4d4f-bb81-1a2aab817c1f
📒 Files selected for processing (2)
secator/tasks/__init__.pysecator/tasks/dirsearch.py
| for cls in TASKS: | ||
| exec(f'from .{cls.__name__} import {cls.__name__}') | ||
| name = cls.__name__ | ||
| if not re.match(r'^[a-zA-Z_][a-zA-Z0-9_]*$', name): | ||
| raise ValueError(f'Invalid task class name: {name}') | ||
| importlib.import_module(f'.{name}', __package__) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that tests expect task classes to be directly importable from secator.tasks
rg -n -C2 'from secator\.tasks import' --type pyRepository: freelabz/secator
Length of output: 6970
🏁 Script executed:
cat -n secator/tasks/__init__.pyRepository: freelabz/secator
Length of output: 496
🏁 Script executed:
rg -n "def discover_tasks" --type py -A 20Repository: freelabz/secator
Length of output: 1230
🏁 Script executed:
rg -n "def discover_internal_tasks" --type py -A 30Repository: freelabz/secator
Length of output: 1746
🏁 Script executed:
rg -n "__getattr__" secator/tasks/__init__.pyRepository: freelabz/secator
Length of output: 42
🏁 Script executed:
git log --oneline -n 20 -- secator/tasks/__init__.pyRepository: freelabz/secator
Length of output: 113
🏁 Script executed:
git show 0274bd6 -- secator/tasks/__init__.pyRepository: freelabz/secator
Length of output: 968
🏁 Script executed:
python3 -c "from secator.tasks import nmap; print(nmap)" 2>&1Repository: freelabz/secator
Length of output: 515
🏁 Script executed:
git show HEAD~1:secator/tasks/__init__.py 2>/dev/null || echo "No previous version"Repository: freelabz/secator
Length of output: 80
🏁 Script executed:
git log --all --oneline -- secator/tasks/__init__.py | head -5Repository: freelabz/secator
Length of output: 397
🏁 Script executed:
git show df94657:secator/tasks/__init__.pyRepository: freelabz/secator
Length of output: 244
🏁 Script executed:
rg -n "import sys" secator/tasks/__init__.pyRepository: freelabz/secator
Length of output: 42
🏁 Script executed:
grep -r "from secator.tasks import" tests/ --include="*.py" | head -3 | cut -d: -f1 | uniq | xargs -I {} sh -c 'echo "File: {}"; head -20 "{}"'Repository: freelabz/secator
Length of output: 588
🏁 Script executed:
python3 << 'PY'
# Simulate what happens with the current __init__.py logic
import importlib
import re
from pathlib import Path
# Check what __all__ does without actual bindings
namespace = {}
namespace['__all__'] = ['nmap', 'httpx']
# Try to import something that's in __all__ but not in namespace
try:
nmap = namespace['nmap']
print("nmap is accessible:", nmap)
except KeyError:
print("KeyError: 'nmap' in __all__ but not in namespace - import would fail")
# This is what a direct import does
print("\nWhen you do 'from module import nmap', Python looks for 'nmap' in:")
print("1. module.__dict__ (module's globals)")
print("2. __all__ only affects 'from module import *'")
PYRepository: freelabz/secator
Length of output: 282
Task classes not exported to package namespace — breaks from secator.tasks import <task> imports.
The refactor from exec(f'from .{cls.__name__} import {cls.__name__}') to importlib.import_module() introduced a critical flaw. The old approach both imported the module AND bound the class to the package namespace. The new code only loads the module into sys.modules but discards the return value, leaving the class unbound.
Additionally, __all__ is defined but irrelevant—it only controls from secator.tasks import * behavior. Direct imports like from secator.tasks import nmap (as used throughout tests) require the class to exist in the module's __dict__, which it currently does not.
🐛 Proposed fix to bind the class to the package namespace
for cls in TASKS:
name = cls.__name__
if not re.match(r'^[a-zA-Z_][a-zA-Z0-9_]*$', name):
raise ValueError(f'Invalid task class name: {name}')
- importlib.import_module(f'.{name}', __package__)
+ module = importlib.import_module(f'.{name}', __package__)
+ globals()[name] = getattr(module, name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/tasks/__init__.py` around lines 10 - 14, The
importlib.import_module() call in the loop only loads the module into
sys.modules but does not bind the task class to the package namespace. To fix
this, capture the return value of importlib.import_module() and extract the
class from the imported module object using getattr() with the class name, then
bind it to the current module's namespace using globals()[cls.__name__] =
imported_class so that direct imports like `from secator.tasks import
<classname>` will function correctly instead of raising ImportError.
| output_path = Path(self.output_path).resolve() | ||
| reports_folder = Path(self.reports_folder).resolve() | ||
| if not str(output_path).startswith(str(reports_folder)): | ||
| yield Error(message=f'Output path {output_path} is not within reports folder {reports_folder}') | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Python version requirements to determine which fix to use
fd -t f -e toml -e cfg -e txt 'pyproject|setup|requirements' --exec cat {}
# Also check for existing usage of is_relative_to in the codebase
rg -n 'is_relative_to|relative_to' --type pyRepository: freelabz/secator
Length of output: 2347
🏁 Script executed:
# Check the actual code in dirsearch.py at lines 79-83
head -85 secator/tasks/dirsearch.py | tail -10Repository: freelabz/secator
Length of output: 416
🏁 Script executed:
# Search for how paths are validated elsewhere in the codebase
rg -n 'startswith.*/' --type py -A 2 -B 2 | head -50Repository: freelabz/secator
Length of output: 3135
🏁 Script executed:
# Check if dirsearch.py exists and verify its structure
wc -l secator/tasks/dirsearch.py
head -100 secator/tasks/dirsearch.py | tail -40Repository: freelabz/secator
Length of output: 1433
String-based startswith path check is bypassable—use path resolution exception handling instead.
The containment check using str(output_path).startswith(str(reports_folder)) can be bypassed. If reports_folder is /home/user/reports and output_path is /home/user/reports_evil/file.json, the check passes because the string starts with /home/user/reports, even though the path is not actually inside the reports folder.
Since the project requires Python 3.8+ support, use the relative_to() exception pattern for robust containment checking. This pattern is already used elsewhere in the codebase.
🔒 Proposed fix for Python 3.8+ compatibility
`@staticmethod`
def on_cmd_done(self):
output_path = Path(self.output_path).resolve()
reports_folder = Path(self.reports_folder).resolve()
- if not str(output_path).startswith(str(reports_folder)):
+ try:
+ output_path.relative_to(reports_folder)
+ except ValueError:
yield Error(message=f'Output path {output_path} is not within reports folder {reports_folder}')
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output_path = Path(self.output_path).resolve() | |
| reports_folder = Path(self.reports_folder).resolve() | |
| if not str(output_path).startswith(str(reports_folder)): | |
| yield Error(message=f'Output path {output_path} is not within reports folder {reports_folder}') | |
| return | |
| output_path = Path(self.output_path).resolve() | |
| reports_folder = Path(self.reports_folder).resolve() | |
| try: | |
| output_path.relative_to(reports_folder) | |
| except ValueError: | |
| yield Error(message=f'Output path {output_path} is not within reports folder {reports_folder}') | |
| return |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@secator/tasks/dirsearch.py` around lines 79 - 83, Replace the string-based
path containment check in the validation block (where output_path and
reports_folder are compared using startswith) with a try-except pattern using
the relative_to() method. Wrap the relative_to() call in a try-except block to
catch ValueError when output_path is not actually within reports_folder, then
yield the Error if the exception occurs. This approach properly validates path
containment and prevents directory traversal attacks, regardless of similar path
name prefixes.
|
@tomaioo this breaks a lot of things, I'm settings this as a draft. Please run unit tests, then fix your code so it passes them before marking it as ready: |
2 participants
Summary
fix(security): 2 improvements across 2 files
Problem
Severity:
High| File:secator/tasks/__init__.py:L1The
secator/tasks/__init__.pyfile usesexec()with string formatting to dynamically import task classes. Thediscover_tasks()function returns classes that are then imported usingexec(f'from .{cls.__name__} import {cls.__name__}'). While the current implementation appears to use discovered classes, this pattern is dangerous ifdiscover_tasks()can be influenced by external input or if the module loading mechanism can be tricked. More critically, theexecwith f-string formatting on class names, if any part of the class name can be controlled, could lead to arbitrary code execution. Additionally, thesecator/loader.py(not fully shown) likely contains thediscover_tasks()function which may use__import__orimportlibwith dynamic paths. The pattern of dynamic imports based on filesystem discovery without proper validation is a security risk.Solution
Replace
exec()withimportlib.import_module()and validate all module names against a strict allowlist usingre.match(r'^[a-zA-Z_][a-zA-Z0-9_]*$', name)before importing. Avoid dynamic imports based on filesystem scanning without cryptographic verification of the module contents.Changes
secator/tasks/__init__.py(modified)secator/tasks/dirsearch.py(modified)Summary by CodeRabbit
Release Notes