T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/trade_logger.py:33
- Finding
- Path Traversal Through Unvalidated Journal Dates<![CDATA[ ## Vulnerability Details **File Location**: `scripts/trade_logger.py:33-36`, with attacker-controlled data reaching the vulnerable function at `scripts/trade_logger.py:92-101` **Vulnerability Type**: Path traversal and arbitrary JSON file overwrite **Risk Level**: High ### Vulnerable Code ```python def _journal_path(target_date: str = None) -> str: """Get path for a date's journal file.""" if target_date is None: target_date = date.today().isoformat() return os.path.join(JOURNALS_DIR, target_date + ".json") ``` The `date` field is passed into this path construction without validation: ```python def add_trade(trade_data: dict) -> dict: """Add a new trade entry and calculate P&L.""" target_date = trade_data.get("date", date.today().isoformat()) # Auto-calculate P&L if not trade_data.get("open", False) and trade_data.get("exit_price") is not None: trade_data["pnl"] = _calculate_pnl(trade_data) trades = _load_trades(target_date) trades.append(trade_data) _save_trades(trades, target_date) ``` ### Technical Analysis `_journal_path()` concatenates an untrusted date value with `.json` and passes it directly to `os.path.join()`. It does not require the value to be a canonical ISO date, reject path separators, normalize the result, or verify that the resolved path remains beneath `JOURNALS_DIR`. A value containing parent-directory components, such as `../backup`, produces a path outside the intended journal directory. A value beginning with an absolute path can also cause `os.path.join()` to discard the intended base directory. The vulnerable function is used by both `_load_trades()` and `_save_trades()`. Consequently, the issue affects the `add`, `update`, and `delete` workflows. The target is constrained to a filename ending in `.json`, and existing files must contain the expected JSON object structure for mutation operations to complete successfully. ### Attack Path 1. An attacker supplies a ...[truncated 1248 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Parse every journal date with `date.fromisoformat()` before using it in a filename. 2. Require canonical `YYYY-MM-DD` formatting by comparing the parsed date's `isoformat()` result with the original input. 3. Reject values containing path separators, parent-directory components, null bytes, or absolute paths. 4. Resolve the final path and verify containment beneath the resolved journal directory. 5. Apply the same centralized validation to `add`, `list`, `update`, and `delete`. 6. Consider opening files through a directory file descriptor or another platform-appropriate safe-path mechanism to reduce symlink race risks. 7. Add tests covering `../target`, absolute paths, malformed dates, encoded separators, and symlinks. Example hardening: ```python def _journal_path(target_date: str = None) -> str: value = target_date or date.today().isoformat() try: parsed = date.fromisoformat(value) except (TypeError, ValueError): raise ValueError("Journal date must use YYYY-MM-DD format") if parsed.isoformat() != value: raise ValueError("Journal date must be canonical YYYY-MM-DD") base = os.path.realpath(JOURNALS_DIR) candidate = os.path.realpath(os.path.join(base, value + ".json")) if os.path.commonpath([base, candidate]) != base: raise ValueError("Journal path escapes the journal directory") return candidate ``` ]]>