robertlazarski commented on PR #1227:
URL:
https://github.com/apache/axis-axis2-java-core/pull/1227#issuecomment-4857572573
Thanks for this @rootvector2, and thanks @cortlepp for taking a look! 🙌
I went through the containment logic carefully and it's correct — nicely
done. A couple of details I want to call out because they're easy to get
wrong and you got them right:
- The outPath + File.separator guard on the prefix check. A plain
startsWith(outPath) would let a sibling directory like .../svc-evil sneak
past a target of .../svc, and this closes that gap.
- The targetPath.equals(outPath) clause. The archive's root WWW/ entry
strips down to an empty name, so it resolves to out itself — without the
equals check, that legitimate entry (the one that creates the per-service
dir) would get skipped.
Using getCanonicalPath() instead of getAbsolutePath() is also the right
call: it collapses .. and resolves symlinks before the comparison, and
absolute-path entries get re-rooted by new File(parent, child) anyway. I
confirmed the test genuinely drives the vulnerable path via reflection and
that a WWW/../../ entry resolves outside the target and gets skipped as
expected. It also has the ASF license header and follows the module's JUnit
convention. 👍
I also checked the other zip-extraction spots — the one in
deployment/util/Utils.java routes entry names through
TempFileManager.createTempFile() with generated names, so it isn't exposed
to the same traversal. No companion fix needed; the scope here is spot on.
LGTM — merging. Thanks again for the contribution!
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
