T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/download_file.py:23
- Finding
- Remote Drive Filename Can Escape the Destination Directory and Overwrite Local Files<![CDATA[ ## Vulnerability Details **File Location**: `scripts/download_file.py:23-41` **Vulnerability Type**: Untrusted path traversal and arbitrary local file overwrite **Risk Level**: High ### Vulnerable Code ```python def download_file(drive, file_id: str, dest_path: str) -> None: # Get filename if dest_path is a directory if os.path.isdir(dest_path): meta = ( drive.files() .get(fileId=file_id, fields="name", supportsAllDrives=True) .execute() ) dest_path = os.path.join(dest_path, meta["name"]) request = drive.files().get_media(fileId=file_id, supportsAllDrives=True) with open(dest_path, "wb") as fh: downloader = MediaIoBaseDownload(fh, request) done = False while not done: status, done = downloader.next_chunk() pct = int(status.progress() * 100) print(f"\rDownloading... {pct}%", end="", flush=True) print(f"\nSaved to: {dest_path}") ``` ### Technical Analysis When the destination supplied through `--dest` is a directory, the script retrieves the file name from Google Drive metadata and passes it directly to `os.path.join`. The resulting path is then opened in `wb` mode without normalization, containment validation, or overwrite protection. The Drive filename is remote data and must therefore be treated as untrusted. A name containing path traversal components can cause the resolved destination to escape the selected download directory. On platforms where an attacker-controlled name is interpreted as an absolute path, `os.path.join` can also discard the intended directory entirely. Opening the resulting path with `open(dest_path, "wb")` creates a new file or truncates an existing file. The script does not check whether the target already exists, whether it is a symbolic link, or whether its resolved path remains under the requested directory. ### Attack Path 1. An attacker creates or controls a Google Drive fil ...[truncated 1558 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Treat the remote filename as untrusted and enforce destination containment: 1. Reject absolute filenames and names containing path separators. 2. Reduce the remote value to a safe filename using `os.path.basename`, while also accounting for separators used by other operating systems. 3. Resolve both the destination directory and candidate output path with `pathlib.Path.resolve()`. 4. Verify that the candidate is a child of the resolved destination directory. 5. Refuse to follow symbolic links or overwrite an existing file by default. 6. Require an explicit `--overwrite` option before truncating an existing destination. 7. Consider generating a local safe filename and retaining the Drive name only as display metadata. Example hardening pattern: ```python from pathlib import Path, PurePath dest_dir = Path(dest_path).resolve() remote_name = meta["name"] if ( not remote_name or PurePath(remote_name).is_absolute() or "/" in remote_name or "\\" in remote_name ): raise ValueError("Unsafe Drive filename") safe_name = Path(remote_name).name target = (dest_dir / safe_name).resolve() if dest_dir not in target.parents: raise ValueError("Download path escapes destination directory") if target.exists(): raise FileExistsError(f"Refusing to overwrite existing file: {target}") with target.open("xb") as fh: downloader = MediaIoBaseDownload(fh, request) ``` For stronger protection against symbolic-link races, use platform-supported descriptor-relative file creation with exclusive and no-follow flags where available. ]]>