T09 · Insecure Skill Coding Practices
Error
- Location
- fa_advisor/advisor.py:131
- Finding
- User-Controlled Project Name Enables Path Traversal and Arbitrary PDF Overwrite<![CDATA[ ## Vulnerability Details **File Location**: `fa_advisor/advisor.py:131-143` **Related Validation Location**: `fa_advisor/types/project.py:193` **Vulnerability Type**: Path traversal and arbitrary file overwrite **Risk Level**: High ### Vulnerable Code ```python # fa_advisor/types/project.py:193 name: str = Field(min_length=1) ``` ```python # fa_advisor/advisor.py:131-143 output_dir = Path("output") output_dir.mkdir(exist_ok=True) result['pdf_reports'] = { 'assessment': await self.pdf_generator.generate_assessment_report( assessment, project.name, output_dir / f"{project.name}_assessment.pdf" ), 'valuation': await self.pdf_generator.generate_valuation_report( valuation, project.name, output_dir / f"{project.name}_valuation.pdf" ) } ``` The same unsafe construction is used for investment memos: ```python # fa_advisor/advisor.py:191-199 output_dir = Path("output") output_dir.mkdir(exist_ok=True) result['pdf_memo'] = await self.pdf_generator.generate_investment_memo( memo, project.name, output_dir / f"{project.name}_investment_memo.pdf" ) ``` The resulting path is passed directly to ReportLab: ```python # fa_advisor/pdf/generator.py:94-95 output_path = Path(output_path) doc = SimpleDocTemplate( str(output_path), pagesize=self.page_size, rightMargin=72, leftMargin=72, topMargin=72, bottomMargin=18 ) ``` ### Technical Analysis `Project.name` is only constrained to have a minimum length. It is not restricted to safe filename characters and may contain absolute paths, path separators, or traversal sequences such as `../`. The value is interpolated directly into output filenames. `pathlib` does not automatically guarantee that the resulting destination remains inside the intended `output` directory. Traversal components can escape that directory, while an absolute interpolated path can override the base path. The PDF generator subsequently opens th ...[truncated 1471 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Do not use the raw company name as a filesystem component. 2. Convert names to a strict filename slug containing only approved characters, such as ASCII letters, digits, hyphens, and underscores. 3. Reject absolute paths, `/`, `\`, `..`, null bytes, control characters, and platform-specific reserved names. 4. Resolve the destination and enforce containment beneath the approved output directory: ```python import re from pathlib import Path output_dir = Path("output").resolve() output_dir.mkdir(mode=0o700, parents=True, exist_ok=True) safe_name = re.sub(r"[^A-Za-z0-9_-]+", "_", project.name).strip("_") if not safe_name: raise ValueError("Project name cannot produce an empty filename") destination = (output_dir / f"{safe_name}_assessment.pdf").resolve() if output_dir not in destination.parents: raise ValueError("Invalid output path") ``` 5. Use collision-resistant filenames or exclusive file creation where overwriting is unnecessary. 6. Require explicit authorization before replacing an existing file. 7. Apply the same centralized path-validation routine to assessment, valuation, and investment-memo outputs. 8. Add tests covering traversal sequences, absolute paths, Windows separators, reserved names, and symbolic-link edge cases. ]]>