# Variant Root Cause Analysis — CVE-2026-58166

## Summary

No fixed-version bypass was confirmed. The variant analysis found one same-sink, vulnerable-only alternate filename form: an absolute multipart `filename` reaches the same `AttachmentService.save_upload_file()` path-join flaw as the parent `../` traversal on vulnerable commit `a6a5cda5560053136897aa44301eacc6c48d8168`. However, the fixed commit `4fd4da603801766b14ad8788649cfc1ad21f99a6` reduces client-controlled filenames to a basename before joining with the temporary upload directory, so both the parent `../` traversal and the absolute-filename alternate trigger are blocked on the fixed/latest tested source. The stage therefore records a negative bypass verdict with a bounded candidate matrix.

## Fix Coverage / Assumptions

The original fix relies on the invariant that `AttachmentService.save_upload_file()` never joins attacker-controlled directory components into `temp_dir / filename`. Commit `4fd4da603801766b14ad8788649cfc1ad21f99a6` adds `AttachmentService._safe_upload_filename(raw)`, which:

- replaces Windows backslashes with POSIX separators,
- applies `os.path.basename(...)`,
- trims whitespace,
- maps empty, `.`, and `..` names to `upload.bin`, and
- uses the sanitized basename for the temporary write and attachment display name.

This explicitly covers the vulnerable upload route `POST /api/uploads/{session_id}` because `server/routes/uploads.py::upload_attachment()` calls only `manager.attachment_service.save_upload_file(session_id, file)` for multipart uploads. Source scanning found no other server-side multipart endpoint that calls `save_upload_file()`.

The fix does not attempt to change unrelated high-risk product behavior such as unauthenticated session creation over `/ws`, arbitrary absolute `yaml_file` handling in `POST /api/workflow/run`, or local-tool/workflow authoring APIs. Those behaviors may deserve separate hardening, but they are not alternate ways to reach the same upload filename path traversal sink.

Target threat-model review: no `SECURITY.md` or explicit repository threat model file was present in the tested checkout. The product documentation exposes Web UI and HTTP workflow execution surfaces, and no documentation was found that excludes unauthenticated upload path traversal from security scope.

## Variant / Alternate Trigger

Tested candidates:

1. **Parent baseline control:** POSIX `../` traversal filename through `AttachmentService.save_upload_file()`.
   - Vulnerable commit: escaped the temporary upload directory via a symlink target and deleted the supplied symlink path.
   - Fixed/latest commit: blocked; response/record basename was sanitized.

2. **Candidate alternate trigger: absolute multipart filename with symlink target.**
   - Entry point equivalence: same production route as parent, `POST /api/uploads/{session_id}`, because the route passes multipart `UploadFile.filename` to the same sink. The runtime test uses a direct `UploadFile`-compatible object against the service sink for deterministic side-by-side source checks; the parent RCA already validated the remote API route to this sink.
   - Code path: `server/routes/uploads.py::upload_attachment()` → `server/services/attachment_service.py::AttachmentService.save_upload_file()` → `temp_path = temp_dir / filename` → `temp_path.open("wb")`.
   - Vulnerable behavior: because `pathlib.Path` discards the left-hand side when the right-hand operand is absolute, `temp_dir / "/attacker/path"` writes to the attacker-specified absolute path. With a symlink at that path, the write follows the symlink and persists data outside the temp directory while cleanup removes only the symlink.
   - Fixed/latest behavior: `_safe_upload_filename()` converts the absolute path to a basename, so the write stays under `temp_dir`; the symlink target is not created.

3. **Candidate alternate trigger: absolute multipart filename pointing at an existing file.**
   - Same sink and same absolute-path semantics as candidate 2.
   - Vulnerable behavior: the existing target is overwritten and then removed by the cleanup `unlink()`.
   - Fixed/latest behavior: the existing target remains unchanged; sanitized basename is stored under the temp/attachment workspace.

Bounded source scan results:

- `server/routes/uploads.py` is the only route that accepts a multipart upload and calls `AttachmentService.save_upload_file()`.
- `server/routes/batch.py` accepts `UploadFile`, but `server/services/batch_parser.py` only uses the upload filename to select/label CSV/XLS parsing (`Path(filename).suffix` and `.stem`), and `BatchRunService._sanitize_label()` constrains derived task directories. It does not join the raw multipart filename into an arbitrary write path.
- Workflow CRUD endpoints in `server/routes/workflows.py` route through `validate_workflow_filename()` and reject traversal/absolute filenames.
- `server/routes/tools.py` restricts local tool filenames with an alphanumeric/underscore/dash regex plus a post-resolution `relative_to()` check.
- `server/routes/vuegraphs.py` stores `filename` as a SQLite key rather than a filesystem path.

## Impact

- **Package/component affected:** OpenBMB/ChatDev workflow server upload attachment handling, primarily `server/services/attachment_service.py` and `server/routes/uploads.py`.
- **Affected version tested:** vulnerable parent commit `a6a5cda5560053136897aa44301eacc6c48d8168` (`4fd4da603801766b14ad8788649cfc1ad21f99a6^`).
- **Fixed/latest version tested:** `4fd4da603801766b14ad8788649cfc1ad21f99a6`; in this run `origin/main` resolved to the same commit.
- **Risk level and consequences:** On vulnerable versions, the absolute-filename form has the same arbitrary file write/delete primitive as the parent bug and can support the same RCE chain if delivered through the unauthenticated upload route. On the fixed/latest commit, the tested candidates do not escape the upload temp directory.

## Impact Parity

- **Disclosed/claimed maximum impact for parent:** remote code execution after unauthenticated upload path traversal.
- **Reproduced impact from this variant run:** vulnerable-only arbitrary file write/delete at the same sink using an absolute filename. The fixed/latest commit blocked this behavior.
- **Parity:** `partial` for the vulnerable-only alternate trigger; `none` as a fixed-version bypass.
- **Not demonstrated:** No code execution was demonstrated for the absolute-filename candidate in the variant stage because the purpose was to test fix coverage. The parent reproduction already demonstrated the RCE chain for the same sink on vulnerable code. No fixed-version write/delete/RCE bypass was observed.

## Root Cause

The underlying vulnerable root cause is the same as the parent issue: pre-fix `AttachmentService.save_upload_file()` trusted `UploadFile.filename` and joined it into a server-side temporary path before opening it for writing. In Python `pathlib`, both `../` components and absolute right-hand operands can escape a base directory when joined naively. The parent bug used `../` traversal; the variant candidate used an absolute filename. Both exploit the same missing basename/path-confinement invariant before `temp_path.open("wb")` and cleanup `temp_path.unlink()`.

The fixed commit closes this class for upload filenames by reducing all slash/backslash-containing input to a final basename before any path join. Therefore, the alternate absolute-filename candidate is not a bypass of commit `4fd4da603801766b14ad8788649cfc1ad21f99a6`.

Fix commit: https://github.com/OpenBMB/ChatDev/commit/4fd4da603801766b14ad8788649cfc1ad21f99a6

## Reproduction Steps

1. Run `bundle/vuln_variant/reproduction_steps.sh` with `bash`.
2. The script:
   - resolves the vulnerable commit (`4fd4da603801766b14ad8788649cfc1ad21f99a6^`), fixed commit (`4fd4da603801766b14ad8788649cfc1ad21f99a6`), and latest `origin/main` when available;
   - creates isolated source clones under `bundle/vuln_variant/` and does not mutate the repro checkout;
   - runs an `UploadFile`-compatible driver against `AttachmentService.save_upload_file()` for the parent baseline and two absolute-filename candidates;
   - writes structured results for vulnerable, fixed, and latest targets; and
   - exits `0` only if a fixed/latest bypass is reproduced, otherwise exits `1` after successful evaluation.
3. Expected evidence for this run:
   - `bundle/vuln_variant/eval_summary.json` shows `bypass_confirmed=false`, `vulnerable_only_alternate_trigger_observed=true`, and `fixed_blocks_parent_baseline=true`.
   - `bundle/vuln_variant/result_vulnerable.json` shows the absolute filename candidates escape/delete on the vulnerable commit.
   - `bundle/vuln_variant/result_fixed.json` shows the same candidates do not escape/delete on the fixed commit.

## Evidence

Primary logs and outputs:

- Main run log: `bundle/logs/vuln_variant_reproduction_steps.log`.
- Vulnerable candidate log: `bundle/logs/vuln_variant_driver_vulnerable.log`.
- Fixed candidate log: `bundle/logs/vuln_variant_driver_fixed.log`.
- Structured summary: `bundle/vuln_variant/eval_summary.json`.
- Structured vulnerable evidence: `bundle/vuln_variant/result_vulnerable.json`.
- Structured fixed evidence: `bundle/vuln_variant/result_fixed.json`.
- Tested revision records: `bundle/logs/vuln_variant_fixed_version.txt` and `bundle/logs/vuln_variant_latest_version.txt`.

Key final-run summary:

```json
{
  "vulnerable_commit": "a6a5cda5560053136897aa44301eacc6c48d8168",
  "fixed_commit": "4fd4da603801766b14ad8788649cfc1ad21f99a6",
  "latest_commit": "4fd4da603801766b14ad8788649cfc1ad21f99a6",
  "bypass_confirmed": false,
  "vulnerable_only_alternate_trigger_observed": true,
  "fixed_blocks_parent_baseline": true
}
```

For `candidate_absolute_filename_symlink`, the vulnerable result has `escaped_temp_dir_via_write=true` and `deleted_supplied_path=true`. The fixed result has `escaped_temp_dir_via_write=false`, `deleted_supplied_path=false`, and a sanitized record name `fixed_candidate_absolute_filename_symlink_link.txt`.

For `candidate_absolute_filename_delete`, the vulnerable result has `deleted_supplied_path=true`. The fixed result has `deleted_supplied_path=false` and `target_preserved_original=true`.

The script was run twice consecutively. Both runs completed deterministically and exited `1`, which is the expected negative-bypass result for this variant stage.

## Recommendations / Next Steps

- Keep `_safe_upload_filename()` as a shared invariant for every path that accepts multipart upload filenames.
- Add regression tests for absolute multipart filenames in addition to `../`, Windows backslashes, blank, `.`, and `..` names. The current fix logic should pass these tests.
- Consider defense-in-depth in `save_upload_file()` by resolving `temp_path` and verifying it is inside `temp_dir` before opening, even after basename sanitization.
- Consider opening temp files with symlink-resistant APIs where practical.
- Separately review unauthenticated workflow execution and absolute `yaml_file` handling in `POST /api/workflow/run`; these are not bypasses of this upload filename fix, but the parent RCE chain shows they materially increase impact when combined with any file-write primitive.

## Additional Notes

- The variant script uses isolated source clones under `bundle/vuln_variant/` and does not leave the project cache/repro checkout in a different state.
- The direct service-sink test is appropriate for fix-coverage analysis because the parent reproduction already proved the remote unauthenticated API path reaches `AttachmentService.save_upload_file()` unchanged.
- No repository `SECURITY.md` or explicit threat model document was found in the tested source tree.