|
| 1 | +I’m going to scan the sai package for common security risk patterns (YAML loading, subprocess usage, tar extraction, eval/exec, pickle, insecure HTTP, etc.) to identify hotspots and then drill into the key files. Expect a quick risk map and targeted fix suggestions. |
| 2 | + |
| 3 | +Searched text for `yaml\.load|safe_load|FullLoader|UnsafeLoader` (`**/sai/**`), 7 results |
| 4 | + |
| 5 | +Searched text for `subprocess|Popen\(|check_output\(|run\(` (`**/sai/**`), 77 results |
| 6 | + |
| 7 | +Searched text for `shell\s*=\s*True` (`**/sai/**`), 3 results |
| 8 | + |
| 9 | +Searched text for `tarfile\.|extractall|extract\(` (`**/sai/**`), 19 results |
| 10 | + |
| 11 | +Searched text for `requests\..*verify\s*=\s*False` (`**/sai/**`), no results |
| 12 | + |
| 13 | +Searched text for `eval\(|exec\(` (`**/sai/**`), 1 result |
| 14 | + |
| 15 | +Searched text for `pickle\.|dill\.` (`**/sai/**`), no results |
| 16 | + |
| 17 | +Searched text for `tempfile\.mktemp` (`**/sai/**`), no results |
| 18 | + |
| 19 | +Searched text for `http://` (`**/sai/**`), 5 results |
| 20 | + |
| 21 | +Searched text for `random\.(rand|random|randint|randbytes)` (`**/sai/**`), no results |
| 22 | + |
| 23 | +Here’s the security review with prioritized findings and concrete improvement suggestions. |
| 24 | + |
| 25 | +## High-Risk / Priority Hotspots |
| 26 | + |
| 27 | +1. Shell invocation (`shell=True`) |
| 28 | + File: base.py line ~103. |
| 29 | + Risk: Command injection if any part of the constructed command involves user-controlled or remote-provided data. |
| 30 | + Fix: |
| 31 | + - Remove `shell=True`; pass a list of args. |
| 32 | + - If shell features are truly needed (globbing, pipes), explicitly implement them via Python (`subprocess.run` with multiple calls, `shlex.split`, or modules like `glob`) and sanitize inputs. |
| 33 | + - Add an allowlist validator for executable name before running. |
| 34 | + |
| 35 | +2. Tar / Zip extraction path traversal |
| 36 | + File: tarball_repository_handler.py multiple `extractall`, `extract`. |
| 37 | + Risk: Malicious archive with `../` or absolute paths overwriting arbitrary files. |
| 38 | + Fix: |
| 39 | + - Implement a safe extraction wrapper verifying each member’s final path stays within `target_dir`. |
| 40 | + Example pattern: |
| 41 | + ``` |
| 42 | + def _safe_extract(tar, target_dir): |
| 43 | + for m in tar.getmembers(): |
| 44 | + dest = os.path.abspath(os.path.join(target_dir, m.name)) |
| 45 | + if not dest.startswith(os.path.abspath(target_dir) + os.sep): |
| 46 | + raise SecurityError(f"Blocked path traversal: {m.name}") |
| 47 | + tar.extract(m, target_dir) |
| 48 | + ``` |
| 49 | + - Replace direct `tar.extractall(target_dir)` with `_safe_extract`. |
| 50 | + - Same approach for zip: iterate `zip_file.infolist()` and validate. |
| 51 | +
|
| 52 | +3. Unencrypted / insecure protocol allowance |
| 53 | + Files: `sai/models/config.py` valid prefixes includes `http://`; usage in examples. |
| 54 | + Risk: Potential MITM when fetching remote resources / saidata. If integrity is critical, require HTTPS or enforce hash checking on HTTP resources. |
| 55 | + Fix: |
| 56 | + - Require `https://` by default; allow `http://` only if a mandatory checksum (SHA256) is provided in config. |
| 57 | + - Add warning log when `http://` used without integrity data. |
| 58 | + - Optionally implement automatic upgrade attempt (try HTTPS first). |
| 59 | +
|
| 60 | +4. Subprocess usage patterns |
| 61 | + Files: spread across `utils/system.py`, `core/execution_engine.py`, `core/git_repository_handler.py`, `utils/security.py`. |
| 62 | + Observations: |
| 63 | + - Generally using `shell=False` (good) except one hotspot above. |
| 64 | + - Need uniform timeouts; ensure all `subprocess.run` calls specify `timeout=`. |
| 65 | + - Some runs may capture stdout/stderr; confirm `text=True` or `encoding` to prevent potential decoding surprises; add `errors="replace"` if needed. |
| 66 | + Recommended improvements: |
| 67 | + - Centralize command execution in a hardened helper (`run_command(cmd: list[str], timeout: int = DEFAULT_TIMEOUT, cwd: Optional[str]=None, env: Optional[dict]=None)`). |
| 68 | + - Validate binary path: `shutil.which(cmd[0])` and reject if not found in allowed locations (optional). |
| 69 | + - Drop ambient environment variables: construct a minimal env whitelist. |
| 70 | +
|
| 71 | +5. YAML loading |
| 72 | + Files: multiple use of `yaml.safe_load` (good). |
| 73 | + Residual risk: Large YAML may cause resource usage issues; add size limits to input before loading. |
| 74 | + Fix: |
| 75 | + - Enforce max file size (e.g., 5MB) or max document complexity (limit number of top-level keys). |
| 76 | + - If file may be remote, verify Content-Type and enforce a timeout. |
| 77 | +
|
| 78 | +6. Path handling / repository operations |
| 79 | + File: `core/git_repository_handler.py`. |
| 80 | + Potential risks: |
| 81 | + - Interpolated user-provided repo URLs; ensure only expected protocols. Currently a prefix allowlist exists—extend with domain allowlist or block embedded credentials (`user:pass@host`). |
| 82 | + - Ensure `git` commands use `--` separator before user-supplied refs or paths to prevent option injection. |
| 83 | + Fix: |
| 84 | + - When checking out refs: `["git", "checkout", "--", ref]`. |
| 85 | + - When adding paths: `["git", "add", "--", path]`. |
| 86 | +
|
| 87 | +## Moderate Risk / Hardening Opportunities |
| 88 | +
|
| 89 | +1. Process preexec (`preexec()` in `execution_engine.py`) |
| 90 | + - Ensure it doesn’t perform unsafe operations; consider resource limits (RLIMIT_CPU, RLIMIT_AS). |
| 91 | + - Add `os.setsid()` already? If not, use it to isolate process group, then drop privileges (if running as root). |
| 92 | +
|
| 93 | +2. Logging of command arguments |
| 94 | + - Review logging for secrets; mask tokens, passwords, or private paths. |
| 95 | +
|
| 96 | +3. HTTP URL Handling (`utils/system.py` line ~297) |
| 97 | + - Add explicit TLS verification when using `requests` (didn’t see any `verify=False`, good). |
| 98 | + - Add optional pinned hashes (SRI style) for downloaded artifacts. |
| 99 | +
|
| 100 | +4. Provider execution (`providers/base.py`) |
| 101 | + - Input validation on dynamic provider parameters; ensure numeric bounds, whitelist strings. |
| 102 | +
|
| 103 | +5. Config models (`models/config.py`) |
| 104 | + - Add explicit validation of repository config: limit number of repositories loaded to prevent memory spikes. |
| 105 | +
|
| 106 | +## Low Risk / Best Practices |
| 107 | +
|
| 108 | +1. Random usage: none found for cryptographic purpose (OK). If generating tokens in future, switch to `secrets` module. |
| 109 | +2. Absence of pickle/eval/exec (good). |
| 110 | +3. Add SecurityError custom exception (already have `errors.py`—extend there). |
| 111 | +4. Supply type annotations for security helpers to improve static analysis. |
| 112 | +
|
| 113 | +## Suggested Code Changes (Targets) |
| 114 | +
|
| 115 | +Change 1: Remove shell=True in `providers/base.py`. |
| 116 | +Before: |
| 117 | +``` |
| 118 | +result = subprocess.run(cmd, shell=True, ...) |
| 119 | +``` |
| 120 | +After: |
| 121 | +``` |
| 122 | +import shlex |
| 123 | +args = cmd if isinstance(cmd, list) else shlex.split(cmd) |
| 124 | +result = subprocess.run(args, shell=False, ...) |
| 125 | +``` |
| 126 | +Add validation: |
| 127 | +``` |
| 128 | +if not args: raise SecurityError("Empty command") |
| 129 | +if os.path.basename(args[0]) not in ALLOWED_BINARIES: raise SecurityError(...) |
| 130 | +``` |
| 131 | +
|
| 132 | +Change 2: Introduce safe tar/zip extraction in `tarball_repository_handler.py`. |
| 133 | +Add helper near top: |
| 134 | +``` |
| 135 | +def _is_within_directory(base: str, target: str) -> bool: |
| 136 | + base = os.path.abspath(base) |
| 137 | + target = os.path.abspath(target) |
| 138 | + return os.path.commonpath([base]) == os.path.commonpath([base, target]) |
| 139 | + |
| 140 | +def safe_extract_tar(tar, target_dir): |
| 141 | + for m in tar.getmembers(): |
| 142 | + dest = os.path.join(target_dir, m.name) |
| 143 | + if not _is_within_directory(target_dir, dest): |
| 144 | + raise SecurityError(f"Blocked path traversal: {m.name}") |
| 145 | + tar.extract(m, target_dir) |
| 146 | +``` |
| 147 | +Replace `tar.extractall(target_dir)` with `safe_extract_tar(tar, target_dir)`. |
| 148 | +
|
| 149 | +Do same for zip: |
| 150 | +``` |
| 151 | +for zinfo in zip_file.infolist(): |
| 152 | + dest = os.path.join(target_dir, zinfo.filename) |
| 153 | + if not _is_within_directory(target_dir, dest): |
| 154 | + raise SecurityError(...) |
| 155 | + zip_file.extract(zinfo, target_dir) |
| 156 | +``` |
| 157 | +
|
| 158 | +Change 3: Add integrity requirement for `http://` resources. |
| 159 | +In `models/config.py` or wherever resources are parsed: |
| 160 | +``` |
| 161 | +if url.startswith("http://") and not resource.get("sha256"): |
| 162 | + raise SecurityError("Plain HTTP requires sha256 checksum.") |
| 163 | +``` |
| 164 | +Add to documentation a note about mandatory checksum. |
| 165 | +
|
| 166 | +Change 4: Central command runner (e.g., in `utils/system.py`): |
| 167 | +``` |
| 168 | +def secure_run(cmd: list[str], timeout: int = 60, env: Optional[dict]=None): |
| 169 | + if not isinstance(cmd, list): raise TypeError |
| 170 | + if any(a.startswith("--") and a == "--" for a in cmd[1:]): raise SecurityError("Suspicious standalone --") |
| 171 | + executable = shutil.which(cmd[0]) |
| 172 | + if executable is None: raise SecurityError("Executable not found in PATH") |
| 173 | + clean_env = {"PATH": os.environ.get("PATH", "")} |
| 174 | + if env: clean_env.update(env) |
| 175 | + return subprocess.run(cmd, timeout=timeout, shell=False, check=False, env=clean_env, capture_output=True, text=True) |
| 176 | +``` |
| 177 | +Refactor scattered `subprocess.run` calls to use `secure_run`. |
| 178 | +
|
| 179 | +Change 5: Git command safety. |
| 180 | +Wrap git commands: |
| 181 | +``` |
| 182 | +def git_cmd(*args): |
| 183 | + base = ["git"] |
| 184 | + safe = [] |
| 185 | + for a in args: |
| 186 | + if a.startswith("-") and a not in SAFE_GIT_OPTIONS: |
| 187 | + raise SecurityError(f"Disallowed git option: {a}") |
| 188 | + safe.append(a) |
| 189 | + return secure_run(base + safe) |
| 190 | +``` |
| 191 | +
|
| 192 | +Change 6: Size limit for YAML loader (`utils/config.py`). |
| 193 | +Before reading: |
| 194 | +``` |
| 195 | +if os.path.getsize(path) > MAX_YAML_SIZE: |
| 196 | + raise SecurityError("YAML file too large") |
| 197 | +``` |
| 198 | +
|
| 199 | +## Testing Additions |
| 200 | +
|
| 201 | +Add tests: |
| 202 | +- `tests/sai/test_security_tar_extraction.py`: create an archive with `../evil` entry and assert `SecurityError`. |
| 203 | +- `tests/sai/test_providers_shell_injection.py`: attempt a command with `; echo HACK` (should fail or be parsed into tokens). |
| 204 | +- `tests/sai/test_http_checksum_requirement.py`: resource with `http://` no checksum -> raises. |
| 205 | +
|
| 206 | +## Documentation Updates |
| 207 | +
|
| 208 | +In `docs/README.md` or `docs/template-engine.md`: |
| 209 | +Add a “Security Hardening” section summarizing: |
| 210 | +- No shell=True |
| 211 | +- Safe extraction |
| 212 | +- Mandatory checksum on HTTP |
| 213 | +- Limited YAML size |
| 214 | +
|
| 215 | +## Next Steps If You Want More |
| 216 | +
|
| 217 | +- Privilege dropping: If runs happen under elevated user, integrate `os.setuid` to unprivileged user before execution (platform constraints). |
| 218 | +- Sandboxing: Use `subprocess.run` with `seccomp` (Linux) or containerization for untrusted builds. |
| 219 | +- Supply chain: Add signature verification for tarballs (e.g., `.asc` + GPG keyring). |
| 220 | +
|
| 221 | +Let me know which changes you’d like me to implement now, and I’ll apply patches and add tests. Or I can start with the most critical (shell=True removal + safe extraction). Just say the word.## Testing Additions |
| 222 | +
|
| 223 | +Add tests: |
| 224 | +- `tests/sai/test_security_tar_extraction.py`: create an archive with `../evil` entry and assert `SecurityError`. |
| 225 | +- `tests/sai/test_providers_shell_injection.py`: attempt a command with `; echo HACK` (should fail or be parsed into tokens). |
| 226 | +- `tests/sai/test_http_checksum_requirement.py`: resource with `http://` no checksum -> raises. |
| 227 | +
|
| 228 | +## Documentation Updates |
| 229 | +
|
| 230 | +In `docs/README.md` or `docs/template-engine.md`: |
| 231 | +Add a “Security Hardening” section summarizing: |
| 232 | +- No shell=True |
| 233 | +- Safe extraction |
| 234 | +- Mandatory checksum on HTTP |
| 235 | +- Limited YAML size |
| 236 | +
|
| 237 | +## Next Steps If You Want More |
| 238 | +
|
| 239 | +- Privilege dropping: If runs happen under elevated user, integrate `os.setuid` to unprivileged user before execution (platform constraints). |
| 240 | +- Sandboxing: Use `subprocess.run` with `seccomp` (Linux) or containerization for untrusted builds. |
| 241 | +- Supply chain: Add signature verification for tarballs (e.g., `.asc` + GPG keyring). |
| 242 | +
|
| 243 | +Let me know which changes you’d like me to implement now, and I’ll apply patches and add tests. Or I can start with the most critical (shell=True removal + safe extraction). Just say the word. |
0 commit comments