T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/download_pmc_pdf.py:115
- Finding
- Unrestricted Output Filename Allows Path Traversal and Arbitrary PDF File Overwrite<![CDATA[ ## Vulnerability Details **File Location**: `scripts/download_pmc_pdf.py`, lines 115–138 **Vulnerability Type**: Path traversal and unrestricted file overwrite **Risk Level**: Medium ### Vulnerable Code ```python # Construct the output filename if not filename: filename = f"PMC{pmc_id}.pdf" if not filename.endswith(".pdf"): filename += ".pdf" output_path = self.output_dir / filename print(f"Processing: {identifier}") print(f"PMC ID: PMC{pmc_id}") print(f"Saving to: {output_path}") # Try Europe PMC europe_pmc_url = f"https://europepmc.org/backend/ptpmcrender.fcgi?accid=PMC{pmc_id}&blobtype=pdf" print(f"\nTrying [Europe PMC]: {europe_pmc_url}") try: response = self.session.get(europe_pmc_url, timeout=120, stream=True) if response.status_code == 200: content_type = response.headers.get('Content-Type', '').lower() with open(output_path, "wb") as f: ``` ### Technical Analysis The `filename` argument is derived from the user-controlled `--filename` command-line option and is joined directly to `self.output_dir`. The code does not reject absolute paths, parent-directory components such as `..`, or symbolic-link destinations. With `pathlib`, joining a base path to an absolute second path causes the base path to be discarded. A relative filename containing traversal components can similarly resolve outside the intended output directory. The destination is then opened in `wb` mode, which truncates an existing file before writing the downloaded response. Appending `.pdf` is not a sufficient path security control. It only limits the final suffix and still permits overwriting any writable destination ending in `.pdf`. Symbolic links can also redirect the operation to another writable PDF path. ### Attack Path 1. An attacker influences the filename passed to the Skill, for example: ```bash python3 scripts/download_pmc_pdf.py PMC12345678 \ -o ./downloads \ -f ../../important-document.pdf ``` 2. The sc ...[truncated 1184 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Restrict custom filenames to a basename: ```python supplied_name = Path(filename) if supplied_name.is_absolute() or supplied_name.name != filename: raise ValueError("Filename must be a simple file name without directory components") ``` 2. Resolve the output directory and destination, then verify containment: ```python output_root = self.output_dir.resolve() output_path = (output_root / supplied_name.name).resolve() if output_root not in output_path.parents: raise ValueError("Output path escapes the configured output directory") ``` 3. Reject `.` and `..` path components explicitly. 4. Prevent unintended overwrites by opening the file in exclusive creation mode: ```python with open(output_path, "xb") as f: ... ``` Alternatively, require an explicit `--overwrite` option. 5. Check for symbolic links before writing and use platform-appropriate no-follow protections where available. 6. Download to a securely created temporary file inside the output directory, validate the completed file, and atomically rename it to the final destination only after all checks succeed. 7. Apply a maximum download size to prevent excessive disk consumption. ]]>