Skip to content

Confine ExecutableManager file access to its managed directories - #18328

Open
arpitjain099 wants to merge 1 commit into
apache:masterfrom
arpitjain099:hardening/executable-manager-path-containment
Open

Confine ExecutableManager file access to its managed directories#18328
arpitjain099 wants to merge 1 commit into
apache:masterfrom
arpitjain099:hardening/executable-manager-path-containment

Conversation

@arpitjain099

Copy link
Copy Markdown
Contributor

Description

ExecutableManager addresses files under libRoot and temporaryLibRoot by concatenating the root with a caller-supplied name:

Path path = Paths.get(this.libRoot + File.separator + fileName);

A name containing parent-directory segments, or an absolute path, therefore resolves outside the directory the manager is responsible for. The accessors have no check that the result stayed inside its root.

This adds resolveUnderRoot(), which resolves the name against the root, normalizes it, and rejects anything that escapes:

public static Path resolveUnderRoot(String root, String fileName) throws IOException {
  Path rootPath = Paths.get(root).toAbsolutePath().normalize();
  Path resolved = rootPath.resolve(fileName).normalize();
  if (!resolved.startsWith(rootPath)) {
    throw new IOException(
        String.format("The resolved path %s is outside of the directory %s", resolved, rootPath));
  }
  return resolved;
}

and routes the by-name accessors through it: removeFileUnderLibRoot, hasFileUnderLibRoot, hasFileUnderInstallDir, hasFileUnderTemporaryRoot, saveTextAsFileUnderTemporaryRoot, removeFileUnderTemporaryRoot and readTextFromFileUnderTemporaryRoot.

Putting it in the shared accessor means the trigger, UDF and pipe-plugin paths that all go through ExecutableManager get the same behaviour from one place, rather than each growing its own check and drifting apart later.

The has* methods return false instead of propagating the exception, since their callers use them as plain existence checks and would not expect one.

Normal names are unaffected, including relative ones that stay inside the root such as install/udf.jar or sub/../udf.jar.

Tests

ExecutableManagerTest covers:

  • names inside the root resolve as before, including one that walks out and back in
  • ../name, ../../name, sub/../../name and an absolute path are all rejected
  • an escaping name passed to saveTextAsFileUnderTemporaryRoot writes nothing to disk
  • an escaping name passed to removeFileUnderLibRoot leaves the outside file in place
  • the has* predicates return false for an escaping name rather than throwing

Verified with mvn -pl iotdb-core/node-commons -am -Dtest=ExecutableManagerTest test: 7 tests pass with the change, and 5 of the 7 fail without it.

This PR has:

  • been self-reviewed.
  • added unit tests

ExecutableManager addresses files under libRoot and temporaryLibRoot by
building the path with string concatenation, so a name containing parent
directory segments or an absolute path resolves outside the directory the
manager is responsible for.

Add resolveUnderRoot(), which resolves the name against the root, normalizes
the result and rejects anything that does not stay inside it, and route the
by-name accessors through it. The has* predicates return false rather than
throwing, since their callers treat them as simple existence checks.

This is a robustness improvement in the shared accessor, so the trigger, UDF
and pipe plugin paths that all use it get consistent behaviour from one place
rather than each needing its own check.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant