SteNicholas commented on PR #3674:
URL: https://github.com/apache/celeborn/pull/3674#issuecomment-4627685842
## Review — Validate applicationId to prevent worker path traversal
Good catch on the underlying issue, and the fix targets the right sink. The
worker builds `<workingDir>/<appId>/<shuffleId>/<fileName>` in
`StorageManager.createDiskFile`, which is reached at `ReserveSlots` time — and
that path is now validated. A few things to address before merge.
### What works well
- **The canonical-path backstop is done correctly**: `dir.getCanonicalPath +
File.separator` with `startsWith` avoids the sibling-prefix false-negative
(`/work` vs `/work-evil`), and moving `shuffleDir.mkdirs()` to *after* the
check means a traversal directory is never created.
- **Allowlist charset is the right approach** — rejecting anything outside
`[A-Za-z0-9_-]` excludes `.`, `/`, `\`, and spaces, so `..` and path separators
can't appear at all.
- `DestroyWorkerSlots` correctly splits the shuffleKey first, then validates
the extracted appId.
- Solid positive/negative test coverage.
### Issues
1. **Description doesn't match the diff** (please reconcile). The PR text
says validateAppId is called "at `PushDataHandler.handleCore`, and the two
checkAuth sites in `FetchHandler`," but the diff changes only 4 files —
`Utils.scala`, `UtilsSuite.scala`, `Controller.scala`, `StorageManager.scala`.
There are no changes to `PushDataHandler` or `FetchHandler`. Either those calls
were dropped from the commit, or the description is stale.
For what it's worth, the impact is limited:
`FetchHandler`/`PushDataHandler` resolve files via
`StorageManager.getFileInfo(shuffleKey, fileName)`, which is a
`ConcurrentHashMap` lookup — it does **not** reconstruct a filesystem path from
`appId`. So those handlers aren't independent traversal sinks, and the missing
calls are defense-in-depth/consistency rather than a hole in the core fix. But
the description should reflect what actually ships.
2. **The regex accepts a trailing newline** (low severity, but a validator
should be airtight). `^[A-Za-z0-9_-]+$` evaluated with `findFirstIn` matches
`"validapp\n"`, because `$` (without `MULTILINE`) matches *before a final line
terminator*, not strictly end-of-input. I verified the anchor semantics —
Java's `java.util.regex` behaves the same:
```
"validapp\n" -> matches (not rejected)
```
A lone trailing `\n` in a directory name isn't itself a traversal
sequence, so real-world impact is small, but the clean fix is a full-region
match:
```scala
if (applicationId == null ||
!appIdPattern.pattern.matcher(applicationId).matches()) { ... }
```
(`matches()` anchors the whole input; equivalently use `\A...\z`.) Worth
adding `"app\n"` to the negative test cases.
3. **Backstop is local-disk-only** (defense-in-depth gap). The
canonical-path containment check is in the local-disk branch of
`createDiskFile`, but the HDFS/S3/OSS branches also build
`s"$appId/$shuffleId"` paths (e.g. `StorageManager.scala:1166`, `1171`) and
rely solely on the upstream `validateAppId`. Given the Controller guard is the
primary defense this is acceptable, but if the intent is layered defense, the
DFS branches currently have none — at least worth a comment noting the
backstop's scope.
4. **Consider centralizing the check** (maintainability). Validation is
currently sprinkled per-RPC-case, which is exactly why items 1 & 3 are easy to
get wrong — a future handler can forget the call. Since every guarded site
calls `checkAuth(context, applicationId)` right after, calling `validateAppId`
*inside* `checkAuth` (so it runs even when auth is disabled) would cover all
current and future entry points from one place.
Nice, well-scoped security fix overall — the main blocker is item 1 (make
the description and code agree), and item 2 is a quick hardening of the
validator itself.
--
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]
