From 584de46aa84d0fe5891e51ce4bb6979e0e19788e Mon Sep 17 00:00:00 2001 From: Valentin Delaye Date: Tue, 12 May 2026 18:33:46 +0200 Subject: [PATCH] Normalize and validate path from org.opencontainers.image.title Signed-off-by: Valentin Delaye --- src/main/java/land/oras/OCILayout.java | 8 +- src/main/java/land/oras/Registry.java | 8 +- src/test/java/land/oras/OCILayoutTest.java | 60 +++++++++++++++ .../java/land/oras/RegistryWireMockTest.java | 74 +++++++++++++++++++ 4 files changed, 148 insertions(+), 2 deletions(-) diff --git a/src/main/java/land/oras/OCILayout.java b/src/main/java/land/oras/OCILayout.java index 8c429a06..5ec11705 100644 --- a/src/main/java/land/oras/OCILayout.java +++ b/src/main/java/land/oras/OCILayout.java @@ -166,7 +166,13 @@ public void pullArtifact(LayoutRef ref, Path path, boolean overwrite) { // Copy the blob to the target path try { - Files.copy(blobPath, path.resolve(layer.getAnnotations().get(Const.ANNOTATION_TITLE))); + Path targetPath = path.resolve(layer.getAnnotations().get(Const.ANNOTATION_TITLE)) + .normalize(); + if (!targetPath.startsWith(path.normalize())) { + throw new OrasException("Refusing to pull layer: title annotation is not withing folder '%s'" + .formatted(layer.getAnnotations().get(Const.ANNOTATION_TITLE))); + } + Files.copy(blobPath, targetPath); } catch (IOException e) { throw new OrasException("Failed to copy blob", e); } diff --git a/src/main/java/land/oras/Registry.java b/src/main/java/land/oras/Registry.java index ca6e2601..d43f68ed 100644 --- a/src/main/java/land/oras/Registry.java +++ b/src/main/java/land/oras/Registry.java @@ -1109,7 +1109,13 @@ private void pullLayer(ContainerRef ref, Layer layer, Path path, boolean overwri ArchiveUtils.untar(Files.newInputStream(tempArchive.getPath()), path); } else { - Path targetPath = path.resolve(layer.getAnnotations().get(Const.ANNOTATION_TITLE)); + Path targetPath = path.resolve(layer.getAnnotations().get(Const.ANNOTATION_TITLE)) + .normalize(); + if (!targetPath.startsWith(path.normalize())) { + throw new OrasException( + "Refusing to pull layer: path is not withing folder in title annotation '%s'" + .formatted(layer.getAnnotations().get(Const.ANNOTATION_TITLE))); + } if (Files.exists(targetPath) && !overwrite) { LOG.info("File already exists: {}", targetPath); return; diff --git a/src/test/java/land/oras/OCILayoutTest.java b/src/test/java/land/oras/OCILayoutTest.java index d2fc353d..8f1618ac 100644 --- a/src/test/java/land/oras/OCILayoutTest.java +++ b/src/test/java/land/oras/OCILayoutTest.java @@ -1149,4 +1149,64 @@ private void assertBlobContent(Path ociLayoutPath, String digest, String content .resolve(SupportedAlgorithm.getDigest(digest))), "Expect blob content to match"); } + + @Test + void pullArtifactShouldRejectInvalidTitleAnnotation() throws IOException { + + // Create a valid OCI layout via the normal push API + Path ociLayoutPath = layoutPath.resolve("traversal-test"); + Path artifactFile = blobDir.resolve("safe.txt"); + Files.writeString(artifactFile, "safe content"); + + LayoutRef layoutRef = LayoutRef.parse("%s:latest".formatted(ociLayoutPath.toString())); + OCILayout ociLayout = + OCILayout.Builder.builder().defaults(ociLayoutPath).build(); + ociLayout.pushArtifact( + layoutRef, ArtifactType.from("foo/bar"), Annotations.empty(), LocalPath.of(artifactFile, "text/plain")); + + // 2. Read the manifest stored on disk and replace the title annotation with a traversal path + Index index = Index.fromPath(ociLayoutPath.resolve(Const.OCI_LAYOUT_INDEX)); + String originalManifestDigest = index.getManifests().get(0).getDigest(); // e.g. sha256:abc... + Path manifestBlobPath = ociLayoutPath + .resolve("blobs") + .resolve("sha256") + .resolve(SupportedAlgorithm.getDigest(originalManifestDigest)); + + String originalManifestJson = Files.readString(manifestBlobPath); + + // Invalid path + String tamperedManifestJson = originalManifestJson.replace("\"safe.txt\"", "\"../traversed-file.txt\""); + + // Write tampered manifest as a new blob and update index.json + byte[] tamperedBytes = tamperedManifestJson.getBytes(StandardCharsets.UTF_8); + String tamperedDigest = SupportedAlgorithm.SHA256.digest(tamperedBytes); + Path tamperedBlobPath = + ociLayoutPath.resolve("blobs").resolve("sha256").resolve(SupportedAlgorithm.getDigest(tamperedDigest)); + Files.write(tamperedBlobPath, tamperedBytes); + + // Rewrite index.json to reference the tampered manifest digest + String updatedIndexJson = Files.readString(ociLayoutPath.resolve(Const.OCI_LAYOUT_INDEX)) + .replace( + SupportedAlgorithm.getDigest(originalManifestDigest), + SupportedAlgorithm.getDigest(tamperedDigest)); + Files.writeString(ociLayoutPath.resolve(Const.OCI_LAYOUT_INDEX), updatedIndexJson); + + // Attempt to pull — must be rejected because the title escapes the output directory + Path outputDir = extractDir.resolve("traversal-output"); + Files.createDirectories(outputDir); + + OCILayout tampered = OCILayout.Builder.builder().defaults(ociLayoutPath).build(); + OrasException exception = assertThrows( + OrasException.class, + () -> tampered.pullArtifact(layoutRef, outputDir, true), + "Expected OrasException for title annotation"); + assertTrue( + exception.getMessage().contains("is not withing folder"), + "Exception message should mention is not withing folder but was: " + exception.getMessage()); + + // 5. The file must NOT have been written outside the output directory + assertFalse( + Files.exists(outputDir.getParent().resolve("traversed-file.txt")), + "Blob must not be written outside the output directory"); + } } diff --git a/src/test/java/land/oras/RegistryWireMockTest.java b/src/test/java/land/oras/RegistryWireMockTest.java index 80322e00..23a244e8 100644 --- a/src/test/java/land/oras/RegistryWireMockTest.java +++ b/src/test/java/land/oras/RegistryWireMockTest.java @@ -23,8 +23,10 @@ import static com.github.tomakehurst.wiremock.client.WireMock.*; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.github.tomakehurst.wiremock.WireMockServer; import com.github.tomakehurst.wiremock.client.WireMock; @@ -1072,4 +1074,76 @@ private byte[] tokenScenario( ContainerRef.parse("localhost:%d/library/%s".formatted(wmRuntimeInfo.getHttpPort(), registryName)); return registry.getBlob(containerRef.withDigest(digest)); } + + @Test + void pullArtifactShouldRejectInvalidTitleAnnotation(WireMockRuntimeInfo wmRuntimeInfo) throws Exception { + WireMock wireMock = wmRuntimeInfo.getWireMock(); + String registryUrl = wmRuntimeInfo.getHttpBaseUrl().replace("http://", ""); + + // Craft a blob and build a manifest whose layer title contains a invalid sequence + byte[] blobContent = "malicious content".getBytes(java.nio.charset.StandardCharsets.UTF_8); + String blobDigest = SupportedAlgorithm.SHA256.digest(blobContent); + + Layer maliciousLayer = Layer.fromDigest(blobDigest, blobContent.length) + .withAnnotations(Map.of(Const.ANNOTATION_TITLE, "../traversed-file.txt")); + + Manifest manifest = Manifest.empty().withLayers(List.of(maliciousLayer)); + String manifestJson = JsonUtils.toJson(manifest); + String manifestDigest = + SupportedAlgorithm.SHA256.digest(manifestJson.getBytes(java.nio.charset.StandardCharsets.UTF_8)); + + // Stub HEAD manifest + wireMock.register(head(urlEqualTo("/v2/library/malicious-artifact/manifests/latest")) + .willReturn(aResponse() + .withStatus(200) + .withHeader(Const.CONTENT_TYPE_HEADER, Const.DEFAULT_MANIFEST_MEDIA_TYPE) + .withHeader(Const.DOCKER_CONTENT_DIGEST_HEADER, manifestDigest))); + + // Stub GET manifest + wireMock.register(get(urlEqualTo("/v2/library/malicious-artifact/manifests/latest")) + .willReturn(aResponse() + .withStatus(200) + .withHeader(Const.CONTENT_TYPE_HEADER, Const.DEFAULT_MANIFEST_MEDIA_TYPE) + .withHeader(Const.DOCKER_CONTENT_DIGEST_HEADER, manifestDigest) + .withBody(manifestJson))); + + // Stub GET blob + wireMock.register(get(urlEqualTo("/v2/library/malicious-artifact/blobs/%s".formatted(blobDigest))) + .willReturn(aResponse() + .withStatus(200) + .withBody(blobContent) + .withHeader(Const.DOCKER_CONTENT_DIGEST_HEADER, blobDigest))); + + Registry registry = Registry.Builder.builder() + .withAuthProvider(authProvider) + .withInsecure(true) + .build(); + + ContainerRef containerRef = ContainerRef.parse("%s/library/malicious-artifact:latest".formatted(registryUrl)); + + Path outputDir = configDir.resolve("pull-output"); + Files.createDirectories(outputDir); + + // Check exception + Throwable cause = assertThrows( + Exception.class, + () -> registry.pullArtifact(containerRef, outputDir, true), + "Expected an exception for title annotation"); + while (cause.getCause() != null && !(cause instanceof OrasException)) { + cause = cause.getCause(); + } + assertInstanceOf( + OrasException.class, + cause, + "Root cause should be OrasException but was: " + + cause.getClass().getName()); + assertTrue( + cause.getMessage().contains("is not withing folder"), + "Exception message should mention is not withing folder but was: " + cause.getMessage()); + + // The file must NOT have been written outside the output directory + assertFalse( + Files.exists(outputDir.getParent().resolve("traversed-file.txt")), + "Blob must not be written outside the output directory"); + } }