Copilot commented on code in PR #537:
URL: https://github.com/apache/tez/pull/537#discussion_r4046790685
##########
tez-common/src/main/java/org/apache/tez/dag/utils/RelocalizationUtils.java:
##########
@@ -60,11 +60,37 @@ public static void addUrlsToClassPath(List<URL> urls) {
private static Path downloadResource(String destName, URI uri, Configuration
conf, String destDir)
throws IOException {
+ // destName is the map key from the client-supplied additionalAmResources
+ // and ends up as the file name we write under the AM working directory.
+ // Refuse anything that could redirect that write outside destDir — an
+ // absolute path, a path separator, or a ".." segment — so a submitter
+ // holding AM modify access cannot land a jar anywhere on the AM host and
+ // chain it into code execution via the classloader / am-hooks path.
+ validateDestName(destName);
FileSystem fs = FileSystem.get(uri, conf);
Path cwd = new Path(destDir);
Path dFile = new Path(cwd, destName);
Path srcPath = new Path(uri);
fs.copyToLocalFile(srcPath, dFile);
return dFile.makeQualified(FileSystem.getLocal(conf).getUri(), cwd);
}
+
+ static void validateDestName(String destName) {
+ if (destName == null || destName.isEmpty()) {
+ throw new IllegalArgumentException("Resource name must not be empty");
+ }
+ if (destName.indexOf('/') >= 0 || destName.indexOf('\\') >= 0
+ || destName.indexOf('\0') >= 0) {
+ throw new IllegalArgumentException(
+ "Resource name must not contain path separators: " + destName);
+ }
+ if (destName.equals(".") || destName.equals("..")) {
+ throw new IllegalArgumentException(
+ "Resource name must not be a parent-directory reference: " +
destName);
+ }
+ if (new Path(destName).isAbsolute()) {
Review Comment:
`Path.isAbsolute()` only checks whether the parsed path component starts
with `/`; it does not reject a URI-scheme form such as `file:..`. That value
passes these checks, but Hadoop's `new Path(cwd, destName)` preserves the
`file` scheme and merges the `..` path component, allowing the destination to
resolve outside `cwd` (and potentially causing an overwrite there). Reject
`:`/URI-scheme syntax for this basename-only input, and add `file:..` to the
regression cases.
##########
tez-common/src/main/java/org/apache/tez/dag/utils/RelocalizationUtils.java:
##########
@@ -60,11 +60,37 @@ public static void addUrlsToClassPath(List<URL> urls) {
private static Path downloadResource(String destName, URI uri, Configuration
conf, String destDir)
throws IOException {
+ // destName is the map key from the client-supplied additionalAmResources
+ // and ends up as the file name we write under the AM working directory.
+ // Refuse anything that could redirect that write outside destDir — an
+ // absolute path, a path separator, or a ".." segment — so a submitter
+ // holding AM modify access cannot land a jar anywhere on the AM host and
+ // chain it into code execution via the classloader / am-hooks path.
+ validateDestName(destName);
Review Comment:
This validation introduces an unchecked failure after
`DAGAppMaster.startDAGExecution` has assigned `currentDAG` and recorded the
resources/history for the submission. The exception is not caught there, so a
single malformed resource name leaves `currentDAG` incomplete and subsequent
session submissions fail with `App master already running a DAG`; validate the
keys before those mutations or roll back the submission state when rejecting
them.
--
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]
