T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/backup_and_send.py:91
- Finding
- Sanitization Replaces an Extracted File Instead of the Original Archive<![CDATA[ ## Vulnerability Details **File Location**: `scripts/backup_and_send.py`, lines 91–106; transmission continues at lines 261–268 **Vulnerability Type**: Incorrect variable reuse causing unredacted sensitive-data transmission **Risk Level**: High ### Vulnerable Code ```python # Clean sensitive information cleaned_count = 0 for root, dirs, files in os.walk(temp_dir): for file in files: file_path = os.path.join(root, file) if clean_file(file_path): cleaned_count += 1 # Repackage the cleaned files with tarfile.open(temp_path, 'w:gz') as tar: tar.add(temp_dir, arcname='.') # Replace the original file shutil.move(temp_path, file_path) ``` The caller subsequently continues with email transmission: ```python # Clean sensitive information if args.clean: if not clean_sensitive_info(output_path): print("⚠️ Sensitive-information cleaning failed, but email transmission will continue") # Send email if not args.no_send: if not send_backup_email(output_path, args.to, args.subject, args.body): sys.exit(1) ``` ### Technical Analysis The `file_path` parameter initially identifies the original archive. Inside the nested directory traversal, however, it is reassigned to each extracted file: ```python file_path = os.path.join(root, file) ``` After traversal completes, `file_path` refers to the last encountered extracted file rather than the original archive. As a result: ```python shutil.move(temp_path, file_path) ``` moves the newly sanitized archive over that extracted file. It does not replace the original archive represented by `output_path`. The original archive therefore remains unchanged and still contains its unredacted contents. The main workflow then passes that original archive to `send_backup_email()`. The function may also report that cleaning completed successfully, creating a false security assurance. The exact filesystem result can vary depending on traversal order and the last extra ...[truncated 1658 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Preserve the original archive path in a separate variable that is never reused: ```python def clean_sensitive_info(archive_path): original_archive_path = os.path.abspath(archive_path) ``` 2. Use a distinct variable for each extracted file: ```python for root, dirs, files in os.walk(temp_dir): for filename in files: extracted_path = os.path.join(root, filename) if clean_file(extracted_path): cleaned_count += 1 ``` 3. Atomically replace the original archive only after cleaning and repackaging succeed: ```python os.replace(temp_path, original_archive_path) ``` The temporary archive should be created on the same filesystem as the destination if atomic replacement is required. 4. Abort transmission when sanitization fails. Do not continue sending when the user explicitly requested `--clean`: ```python if args.clean and not clean_sensitive_info(output_path): print("Sensitive-information cleaning failed; refusing to send.") sys.exit(1) ``` 5. Use `try`/`finally` or `tempfile.TemporaryDirectory()` to ensure temporary files and directories are securely removed on both success and failure. 6. Add automated tests that: - Create an archive containing representative secrets. - Run the cleaning workflow. - Reopen the archive at `output_path`. - Verify that secrets are absent. - Verify that the exact archive passed to the email function is the sanitized archive. ]]>