Prevent path traversal in DescriptorIndexCollector

[JuliusHenke]

Motivation

• The remote descriptor index exposed file names that could contain .., absolute paths, or separators and be used to write files outside the configured descriptor download directory, enabling a path traversal overwrite attack.
• The change aims to ensure downloaded files and their temporary files always resolve beneath the configured local descriptor directory and to skip malicious or malformed entries instead of writing them.

Description

• Added normalization of the configured localDir to Path localDirPath = localDir.toPath().toAbsolutePath().normalize() and used it as the canonical containment root.
• Rejected remote file names that are null, empty, . or .., or contain path separators (/ or \) and skip those entries with a warning.
• Resolved and normalized both the destination and temporary file paths and verified each startsWith(localDirPath) before performing the download and rename, skipping and logging any entries that resolve outside the local directory.

Testing

• Ran ./gradlew compileJava from the backend/ directory, which failed in this environment with Gradle reporting What went wrong: 25.0.1.
• Attempting ./gradlew :backend:compileJava from the repo root failed because a wrapper was not present at the project root in this environment.
• No additional automated tests were available or executed here; behavior was validated by inspecting the modified control-flow and canonical path checks in DescriptorIndexCollector.fetchRemoteFiles.

---

Codex Task

[Copilot]

Pull request overview

This pull request hardens DescriptorIndexCollector.fetchRemoteFiles against path traversal by validating remote file names and ensuring computed destination paths remain under the configured local descriptor directory before writing.

Changes:

• Normalizes the configured local descriptor directory into a canonical containment root (localDirPath).
• Rejects remote entries with invalid file names (empty, ./.., or containing path separators) and skips them with warnings.
• Normalizes destination and temp file paths and skips any entries that resolve outside the local directory.

Comments suppressed due to low confidence (1)

backend/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java:117

• filepathname.replace(filename, "") is unsafe for deriving the parent directory: it replaces all occurrences of filename, which can yield an unintended (or even absolute) path. Because mkdirs() is executed before the startsWith(localDirPath) containment check, a malicious filename that matches a segment in the configured remote path (e.g., recent) could cause directory creation attempts outside localDir or abort collection unexpectedly. Prefer deriving the parent path using path operations (e.g., take Paths.get(filepathname).getParent()), then resolve against localDirPath and perform containment validation before creating directories.

      File filepath = new File(localDir,
          filepathname.replace(filename, ""));
      long lastModifiedMillis = entry.getValue().lastModifiedMillis();
      if (lastModifiedMillis < minLastModified
          || (locals.containsKey(filepathname)
              && locals.get(filepathname) >= lastModifiedMillis)) {
        continue;
      }
      if (!filepath.exists() && !filepath.mkdirs()) {
        logger.warn("Cannot create local directory {} to store remote file {}. "
            + "Aborting descriptor collection.", filepath, filename);
        return false;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[JuliusHenke]

@copilot Add test cases for the changed functionality ensuring that genuine activity still works as intended while path traversal is prevented.

[Copilot]

@copilot Add test cases for the changed functionality ensuring that genuine activity still works as intended while path traversal is prevented.

Added targeted regression coverage in DescriptorIndexCollectorTest (commit 4b4b331). The tests verify both normal descriptor download behavior and that traversal attempts via symlinked paths are blocked.