shahar1 commented on code in PR #67667:
URL: https://github.com/apache/airflow/pull/67667#discussion_r3423616284
##########
providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_sftp.py:
##########
@@ -176,7 +176,33 @@ def _resolve_destination_path(self, source_object: str,
prefix: str | None = Non
source_object = os.path.relpath(source_object, start=prefix)
else:
source_object = os.path.basename(source_object)
- return os.path.join(self.destination_path, source_object)
+ destination = os.path.join(self.destination_path, source_object)
+ # GCS object names are arbitrary UTF-8 strings and originate from
+ # whoever can write to the source bucket (frequently a lower-trust
+ # external producer). A name containing ``..`` segments or an
+ # absolute-path prefix would otherwise canonicalise outside the
+ # configured ``destination_path`` on the SFTP server: ``os.path.join``
+ # preserves ``..`` and an absolute ``source_object`` silently absorbs
+ # the destination prefix. Three escape patterns to refuse, while
+ # still allowing relative roots like ``"."`` or ``""`` (SFTP user's
+ # current/login directory) that depend on the configured base
+ # remaining the prefix of the resolved path.
+ resolved = os.path.normpath(destination)
+ escapes = resolved == ".." or resolved.startswith(".." + os.sep)
+ if not escapes and os.path.isabs(resolved) and not
os.path.isabs(self.destination_path):
+ # ``source_object`` was itself absolute and absorbed the relative
base.
+ escapes = True
+ if not escapes and os.path.isabs(self.destination_path):
Review Comment:
**Containment gap for a relative `destination_path`.** This containment arm
only runs when `destination_path` is absolute, so a relative base that
normalises to something other than `.`/`""` gets only the leading-`..` check
above — which a one-level escape clears once `normpath` drops the `..`.
Verified on this branch:
| `destination_path` | object name | result |
|---|---|---|
| `incoming` | `../.ssh/authorized_keys` | **ALLOWED →
`.ssh/authorized_keys`** |
| `uploads/in` | `../../etc/passwd` | **ALLOWED → `etc/passwd`** |
| `/srv/sftp/in` | `../.ssh/authorized_keys` | rejected ✅ |
| `.` / `""` | `../.ssh/authorized_keys` | rejected ✅ |
With `destination_path="incoming"` the upload lands at
`~/.ssh/authorized_keys` on the SFTP host — the exact vector this method's
comment cites. Suggestion: when `os.path.normpath(self.destination_path)` is
neither `.` nor `""`, apply the same `startswith(base + os.sep)` containment
the absolute branch uses.
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @shahar1 before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
