DanielLeens commented on PR #10618:
URL: https://github.com/apache/seatunnel/pull/10618#issuecomment-5412551189
Hi @SEZ9, thanks for the detailed follow-up pass. I independently re-read
`seatunnel-common/src/main/java/org/apache/seatunnel/common/utils/FileUtils.java`
on the current head (`77afe1b29dee`) before replying, rather than taking the
findings at face value.
**Issue 2 (nested-symlink escape via `FOLLOW_LINKS`) is correct, and it's a
real gap in my own prior review.** `isContainedDirectory()`
(`FileUtils.java:89-104`) only resolves and checks the top-level
`common/`/`<type>/` directory against `zetaRealPath`. Once that check passes,
`searchJarFiles(commonDir)` / `searchJarFiles(storageDir)` walk with
`Files.walk(directory, maxDepth, FileVisitOption.FOLLOW_LINKS)`
(`FileUtils.java:59`) at unbounded depth, and nothing re-checks containment
per-entry during that walk. A symlink nested inside an already-contained
directory (e.g. `common/deps -> /opt/attacker`) or a symlinked `.jar` file
anywhere under it would still be picked up. My August 24 review only re-derived
the three vectors from the original July 26 finding (string traversal, absolute
path, top-level symlink) and didn't check whether containment survives the
recursive walk — it doesn't. Conceding this one; it should be a blocking item,
not non-blocking, since it defeats the cont
ainment guarantee the Javadoc at lines 84-87 claims. Fix direction I'd
suggest: check `path.toRealPath().startsWith(zetaRealPath)` per candidate
inside the walk (or run the storage-layout walk without `FOLLOW_LINKS`, since
legitimate storage JARs won't be symlinked).
**Issues 3/4/5 (empty or illegal `storage.type` fall back to a full
recursive scan) are also confirmed in the code** (`FileUtils.java:127-128`,
`136-142`) — that path does merge `common/` + `s3/` + `oss/` into one
classloader, which is the isolation problem this PR sets out to fix, and the
"Fail closed" comment on line 135 is misleading since it's actually the widest
possible scan. I'd treat these as legitimate non-blocking-but-should-fix items
(not the same severity as Issue 2, since this doesn't cross a filesystem trust
boundary the way the symlink escape does — it's a design gap in the
default/misconfigured case, not a path-containment bypass).
**Issue 6 (unguarded `toRealPath()` IOException) and Issue 7 (missing
storage dir silently degrades to common+root with a success-shaped log)** both
check out against the current source too — no `catch` around either
`toRealPath()` call, and the `splitLayoutDetected` branch doesn't distinguish
"storage dir found" from "storage dir missing, only common/root loaded."
On the CI-blocking compile break I flagged in my own August 24 review
(`JobStateEventTest.java:165`, `FAILED_JOB_EVENT_TIMEOUT_SECONDS` undefined): I
re-checked `dev` just now and it's already fixed there — `dev` commit
`43fe63b1fc` ("[Fix][Zeta] Fix undefined job event timeout constant (#11954)")
is on the current `dev` HEAD. So the concrete next step for @corgy-w is to
rebase/sync this branch onto the latest `dev` and rerun `Build`; that specific
compile break should be gone, and this PR's own new tests
(`CheckpointServiceStorageClassLoaderTest`, `FileMapStoreTest`) will finally
get to run in CI, which they haven't yet in any run so far.
Given the above, I'm revising my prior "no High/blocking items in this PR's
own diff" conclusion: Issue 2 should be treated as a blocking item alongside
getting a clean, fully-executed CI run. Issues 1/3/4/5/6/7/8 remain valid
non-blocking follow-ups worth addressing in the same pass. Thanks again for the
thorough re-check, @SEZ9 — this is exactly the kind of independent verification
that catches what a single reviewer misses.
--
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]
