Skip to content

Fix java/zipslip CodeQL alerts by validating archive entry paths#761

Open
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-java-zipslip
Open

Fix java/zipslip CodeQL alerts by validating archive entry paths#761
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:fix-codeql-java-zipslip

Conversation

@vharseko

Copy link
Copy Markdown
Member

Fixes the four open java/zipslip code-scanning alerts (#105, #106, #107, #108) by validating archive entry paths before any file system operation, using the normalize-and-startsWith barrier recommended for this rule:

  • StaticUtils.extractZipArchive — rejects zip entries whose resolved path escapes the target directory before creating directories or extracting files.
  • BackupManager.restoreZipEntry — normalizes restoreDir.resolve(zipEntryName) and rejects entries outside the restore directory before creating parent directories and writing the restored file.
  • ZipExtractor.extract — rejects zip entries whose resolved destination escapes the destination directory before copying the entry.
  • GenerateConfigMojo — new safeOutputPath() helper validates the generated-file paths built from jar entry names before they are used to create output files.

@vharseko
vharseko requested a review from maximthomas July 24, 2026 15:31
@vharseko vharseko added security Security fixes / CodeQL code-scanning alerts java Pull requests that update java code labels Jul 24, 2026

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The normalize-and-startsWith barrier is placed correctly in all four sinks — before any mkdirs() / createDirectories() / FileOutputStream — and Path.startsWith is component-based, so /tmp/restore-evil vs /tmp/restore is handled right. BackupManager is exactly right: it reassigns fileToRestore to the normalized path and writes that. The other three validate one path and write a different one, which leaves a hole and, in one case, breaks a legitimate call. Two one-line fixes below.

Symlink traversal still escapes (Medium)

StaticUtils, ZipExtractor and GenerateConfigMojo check normalize() but then write to the un-normalized File. normalize() is purely lexical, so an entry like link/../evil.txt passes the guard and the OS then resolves link before ... Verified locally:

guard allows = true (norm=sym/target/evil.txt)
actually written to: .../scratchpad/sym/evil.txt   <-- outside sym/target

Requires a pre-existing symlink under the destination (a prior install, an upgrade target) — but that is precisely the extraction scenario here. The normalized path is already computed, so just use it:

// opendj-server-legacy/src/main/java/org/opends/server/util/StaticUtils.java
Path targetDir = targetDirectory.toPath().toAbsolutePath().normalize();
Path targetPath = targetDir.resolve(fileEntry.getName()).normalize();
if (!targetPath.startsWith(targetDir))
{
  throw new IOException("Zip entry '" + fileEntry.getName() + "' is outside of the target directory");
}
File targetFile = targetPath.toFile();

Same shape for ZipExtractor.extract and GenerateConfigMojo.safeOutputPath (return outputFile.toPath().normalize().toString()). BackupManager already does this and needs no change.

Relative target directory rejects every entry (Medium)

StaticUtils.extractZipArchive never absolutizes targetDirectory. Paths.get(".").normalize() is the empty path, and nothing startsWith it:

dir=.  name=opendj/bin/start-ds  ->  norm=opendj/bin/start-ds  base=(empty)  allowed=false

This is reachable through the public embedded API — EmbeddedDirectoryServer.java:455 does File deployDirectory = serverRoot.getParentFile();, and new File("./opendj").getParentFile() is .. So serverRootDirectory("./opendj") + extractArchiveForSetup() worked before this PR and now throws "Zip entry 'opendj/…' is outside of the target directory". Fixed by the toAbsolutePath() in the snippet above. ZipExtractor is unaffected — Utils.getPath() canonicalizes first (Utils.java:294).

No regression test (Low)

opendj-server-legacy/src/test/java/org/opends/server/util/BackupManagerTestCase.java already builds and restores archives, so a negative case is cheap: archive with a ../evil.txt entry, assert the restore fails and nothing lands outside the restore dir. Without it the guard can be dropped in a refactor silently.

Nits

  • GenerateConfigMojo guard is defensive-only: keys come from name.substring(name.lastIndexOf('/') + 1, endPos) (line 486) or File.getName() (line 464) and always get a CfgDefn.java-style suffix — they can never contain a separator. Fine to keep for the alert, but note javaDir + "/" + entry.getKey() + "/package-info.java" and the manifest path are left unwrapped, which reads as inconsistent.
  • ZipExtractor error is mislabelled: the new throw is inside the try whose catch (IOException) rewrites it as INFO_ERROR_COPYING, so a zip-slip rejection surfaces to the user as a file-copy error. Hoist the check above the try.
  • Hoist the base path out of the loop: targetDirectory.toPath().normalize() / new File(destDir).toPath().normalize() are recomputed for every zip entry.
  • Hardcoded English messages: these classes otherwise use LocalizableMessage. Acceptable here since BackupManager wraps into ERR_BACKUP_CANNOT_RESTORE upstream — just a conscious call.
  • Copyright wording: Portions Copyrighted 2026 (12 files repo-wide) vs Portions Copyright 2026 (180, and what the CDDL header template says).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

java Pull requests that update java code security Security fixes / CodeQL code-scanning alerts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants