mirror of
https://github.com/spring-projects/spring-boot.git
synced 2026-09-17 12:09:16 +00:00
Protect against corrupt buildpack archives
Update zip and tar handling in buildpack code to ensure that archive entries cannot be written outside of the expected destination. Although we consider buildpacks to be trusted, this update will help protect against corrupt archives. Closes gh-50141
This commit is contained in:
+5
@@ -37,6 +37,7 @@ import org.springframework.boot.buildpack.platform.docker.type.Layer;
|
||||
import org.springframework.boot.buildpack.platform.docker.type.LayerId;
|
||||
import org.springframework.boot.buildpack.platform.io.IOConsumer;
|
||||
import org.springframework.boot.buildpack.platform.io.TarArchive;
|
||||
import org.springframework.util.Assert;
|
||||
import org.springframework.util.StreamUtils;
|
||||
|
||||
/**
|
||||
@@ -132,6 +133,10 @@ final class ImageBuildpack implements Buildpack {
|
||||
out.setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX);
|
||||
TarArchiveEntry entry = in.getNextEntry();
|
||||
while (entry != null) {
|
||||
String entryName = entry.getName();
|
||||
Path entryPath = Path.of(entryName);
|
||||
Assert.state(entryPath.toAbsolutePath().equals(entryPath.toAbsolutePath().normalize()),
|
||||
() -> "Malformed zip entry name '%s'".formatted(entryName));
|
||||
out.putArchiveEntry(entry);
|
||||
StreamUtils.copy(in, out);
|
||||
out.closeArchiveEntry();
|
||||
|
||||
+15
-10
@@ -30,6 +30,7 @@ import org.apache.commons.compress.compressors.gzip.GzipCompressorInputStream;
|
||||
|
||||
import org.springframework.boot.buildpack.platform.docker.type.Layer;
|
||||
import org.springframework.boot.buildpack.platform.io.IOConsumer;
|
||||
import org.springframework.util.Assert;
|
||||
import org.springframework.util.StreamUtils;
|
||||
|
||||
/**
|
||||
@@ -86,19 +87,23 @@ final class TarGzipBuildpack implements Buildpack {
|
||||
private void copyAndRebaseEntries(OutputStream outputStream) throws IOException {
|
||||
String id = this.coordinates.getSanitizedId();
|
||||
Path basePath = Paths.get("/cnb/buildpacks/", id, this.coordinates.getVersion());
|
||||
try (TarArchiveInputStream tar = new TarArchiveInputStream(
|
||||
try (TarArchiveInputStream tarInputStream = new TarArchiveInputStream(
|
||||
new GzipCompressorInputStream(Files.newInputStream(this.path)));
|
||||
TarArchiveOutputStream output = new TarArchiveOutputStream(outputStream)) {
|
||||
writeBasePathEntries(output, basePath);
|
||||
TarArchiveEntry entry = tar.getNextEntry();
|
||||
TarArchiveOutputStream tarOutputStream = new TarArchiveOutputStream(outputStream)) {
|
||||
writeBasePathEntries(tarOutputStream, basePath);
|
||||
TarArchiveEntry entry = tarInputStream.getNextEntry();
|
||||
while (entry != null) {
|
||||
entry.setName(basePath + "/" + entry.getName());
|
||||
output.putArchiveEntry(entry);
|
||||
StreamUtils.copy(tar, output);
|
||||
output.closeArchiveEntry();
|
||||
entry = tar.getNextEntry();
|
||||
String entryName = entry.getName();
|
||||
Path entryPath = basePath.resolve(entryName);
|
||||
Assert.state(entryPath.toAbsolutePath().normalize().startsWith(basePath.toAbsolutePath()),
|
||||
() -> "Entry '%s' cannot be written outside of '%s'".formatted(entryName, basePath));
|
||||
entry.setName(basePath + "/" + entryName);
|
||||
tarOutputStream.putArchiveEntry(entry);
|
||||
StreamUtils.copy(tarInputStream, tarOutputStream);
|
||||
tarOutputStream.closeArchiveEntry();
|
||||
entry = tarInputStream.getNextEntry();
|
||||
}
|
||||
output.finish();
|
||||
tarOutputStream.finish();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+6
-1
@@ -20,6 +20,7 @@ import java.io.File;
|
||||
import java.io.IOException;
|
||||
import java.io.InputStream;
|
||||
import java.io.OutputStream;
|
||||
import java.nio.file.Path;
|
||||
import java.util.Enumeration;
|
||||
|
||||
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
|
||||
@@ -94,7 +95,11 @@ public class ZipFileTarArchive implements TarArchive {
|
||||
|
||||
private TarArchiveEntry convert(ZipArchiveEntry zipEntry) {
|
||||
byte linkFlag = (zipEntry.isDirectory()) ? TarConstants.LF_DIR : TarConstants.LF_NORMAL;
|
||||
TarArchiveEntry tarEntry = new TarArchiveEntry(zipEntry.getName(), linkFlag, true);
|
||||
String entryName = zipEntry.getName();
|
||||
Path entryPath = Path.of(entryName);
|
||||
Assert.state(entryPath.toAbsolutePath().equals(entryPath.toAbsolutePath().normalize()),
|
||||
() -> "Malformed zip entry name '%s'".formatted(entryName));
|
||||
TarArchiveEntry tarEntry = new TarArchiveEntry(entryName, linkFlag, true);
|
||||
tarEntry.setUserId(this.owner.getUid());
|
||||
tarEntry.setGroupId(this.owner.getGid());
|
||||
tarEntry.setModTime(NORMALIZED_MOD_TIME);
|
||||
|
||||
+24
-6
@@ -177,18 +177,36 @@ class ImageBuildpackTests extends AbstractJsonTests {
|
||||
assertThat(buildpack).isNull();
|
||||
}
|
||||
|
||||
@Test
|
||||
void resolveWhenEntryWouldWriteOutsideOfDestinationThrowsException() throws Exception {
|
||||
Image image = Image.of(getContent("buildpack-image.json"));
|
||||
ImageReference imageReference = ImageReference.of("example/buildpack1:latest");
|
||||
BuildpackResolverContext resolverContext = mock(BuildpackResolverContext.class);
|
||||
given(resolverContext.getBuildpackLayersMetadata()).willReturn(BuildpackLayersMetadata.fromJson("{}"));
|
||||
given(resolverContext.fetchImage(eq(imageReference), eq(ImageType.BUILDPACK))).willReturn(image);
|
||||
willAnswer((invocation) -> withMockLayers(invocation, "..")).given(resolverContext)
|
||||
.exportImageLayers(eq(imageReference), any());
|
||||
BuildpackReference reference = BuildpackReference.of("example/buildpack1");
|
||||
assertThatIllegalStateException().isThrownBy(() -> ImageBuildpack.resolve(resolverContext, reference))
|
||||
.withMessage("Malformed zip entry name '../cnb/'");
|
||||
}
|
||||
|
||||
private Object withMockLayers(InvocationOnMock invocation) {
|
||||
return withMockLayers(invocation, "");
|
||||
}
|
||||
|
||||
private Object withMockLayers(InvocationOnMock invocation, String entryPrefix) {
|
||||
try {
|
||||
IOBiConsumer<String, TarArchive> consumer = invocation.getArgument(1);
|
||||
File tarFile = File.createTempFile("create-builder-test-", null);
|
||||
try (TarArchiveOutputStream tarOut = new TarArchiveOutputStream(new FileOutputStream(tarFile))) {
|
||||
tarOut.setLongFileMode(TarArchiveOutputStream.LONGFILE_POSIX);
|
||||
writeTarEntry(tarOut, "/cnb/");
|
||||
writeTarEntry(tarOut, "/cnb/buildpacks/");
|
||||
writeTarEntry(tarOut, "/cnb/buildpacks/example_buildpack/");
|
||||
writeTarEntry(tarOut, "/cnb/buildpacks/example_buildpack/0.0.1/");
|
||||
writeTarEntry(tarOut, "/cnb/buildpacks/example_buildpack/0.0.1/buildpack.toml");
|
||||
writeTarEntry(tarOut, "/cnb/buildpacks/example_buildpack/0.0.1/" + this.longFilePath);
|
||||
writeTarEntry(tarOut, entryPrefix + "/cnb/");
|
||||
writeTarEntry(tarOut, entryPrefix + "/cnb/buildpacks/");
|
||||
writeTarEntry(tarOut, entryPrefix + "/cnb/buildpacks/example_buildpack/");
|
||||
writeTarEntry(tarOut, entryPrefix + "/cnb/buildpacks/example_buildpack/0.0.1/");
|
||||
writeTarEntry(tarOut, entryPrefix + "/cnb/buildpacks/example_buildpack/0.0.1/buildpack.toml");
|
||||
writeTarEntry(tarOut, entryPrefix + "/cnb/buildpacks/example_buildpack/0.0.1/" + this.longFilePath);
|
||||
tarOut.finish();
|
||||
}
|
||||
try (FileInputStream tarFileStream = new FileInputStream(tarFile)) {
|
||||
|
||||
+11
@@ -25,6 +25,7 @@ import org.junit.jupiter.api.io.TempDir;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
|
||||
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
|
||||
import static org.mockito.Mockito.mock;
|
||||
|
||||
/**
|
||||
@@ -91,4 +92,14 @@ class TarGzipBuildpackTests {
|
||||
assertThat(buildpack).isNull();
|
||||
}
|
||||
|
||||
@Test
|
||||
void resolveWillNotIApplyEntriesOutsideOfOutputLocation() throws Exception {
|
||||
Path compressedArchive = this.testTarGzip
|
||||
.createArchive((entryName) -> entryName.endsWith(".toml") ? entryName : "../" + entryName);
|
||||
BuildpackReference reference = BuildpackReference.of(compressedArchive.toUri().toString());
|
||||
Buildpack buildpack = TarGzipBuildpack.resolve(this.resolverContext, reference);
|
||||
assertThatIllegalStateException().isThrownBy(() -> buildpack.apply((layers) -> {
|
||||
})).withMessage("Entry '../bin/' cannot be written outside of '/cnb/buildpacks/example_buildpack1/0.0.1'");
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
+15
-9
@@ -26,6 +26,7 @@ import java.nio.file.Path;
|
||||
import java.nio.file.Paths;
|
||||
import java.util.ArrayList;
|
||||
import java.util.List;
|
||||
import java.util.function.UnaryOperator;
|
||||
|
||||
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
|
||||
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
|
||||
@@ -51,18 +52,22 @@ class TestTarGzip {
|
||||
}
|
||||
|
||||
Path createArchive() throws Exception {
|
||||
return createArchive(true);
|
||||
return createArchive(true, UnaryOperator.identity());
|
||||
}
|
||||
|
||||
Path createArchive(UnaryOperator<String> entryNameProcessor) throws Exception {
|
||||
return createArchive(true, entryNameProcessor);
|
||||
}
|
||||
|
||||
Path createEmptyArchive() throws Exception {
|
||||
return createArchive(false);
|
||||
return createArchive(false, UnaryOperator.identity());
|
||||
}
|
||||
|
||||
private Path createArchive(boolean addContent) throws Exception {
|
||||
private Path createArchive(boolean addContent, UnaryOperator<String> entryNameProcessor) throws Exception {
|
||||
Path path = Paths.get(this.buildpackDir.getAbsolutePath(), "buildpack.tar");
|
||||
Path archive = Files.createFile(path);
|
||||
if (addContent) {
|
||||
writeBuildpackContentToArchive(archive);
|
||||
writeBuildpackContentToArchive(archive, entryNameProcessor);
|
||||
}
|
||||
return compressBuildpackArchive(archive);
|
||||
}
|
||||
@@ -74,7 +79,8 @@ class TestTarGzip {
|
||||
return tgzPath;
|
||||
}
|
||||
|
||||
private void writeBuildpackContentToArchive(Path archive) throws Exception {
|
||||
private void writeBuildpackContentToArchive(Path archive, UnaryOperator<String> entryNameProcessor)
|
||||
throws Exception {
|
||||
StringBuilder buildpackToml = new StringBuilder();
|
||||
buildpackToml.append("[buildpack]\n");
|
||||
buildpackToml.append("id = \"example/buildpack1\"\n");
|
||||
@@ -92,10 +98,10 @@ class TestTarGzip {
|
||||
echo "---> build"
|
||||
""";
|
||||
try (TarArchiveOutputStream tar = new TarArchiveOutputStream(Files.newOutputStream(archive))) {
|
||||
writeEntry(tar, "buildpack.toml", buildpackToml.toString());
|
||||
writeEntry(tar, "bin/");
|
||||
writeEntry(tar, "bin/detect", detectScript);
|
||||
writeEntry(tar, "bin/build", buildScript);
|
||||
writeEntry(tar, entryNameProcessor.apply("buildpack.toml"), buildpackToml.toString());
|
||||
writeEntry(tar, entryNameProcessor.apply("bin/"));
|
||||
writeEntry(tar, entryNameProcessor.apply("bin/detect"), detectScript);
|
||||
writeEntry(tar, entryNameProcessor.apply("bin/build"), buildScript);
|
||||
tar.finish();
|
||||
}
|
||||
}
|
||||
|
||||
+17
-5
@@ -31,6 +31,7 @@ import org.junit.jupiter.api.io.TempDir;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
|
||||
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
|
||||
|
||||
/**
|
||||
* Tests for {@link ZipFileTarArchive}.
|
||||
@@ -52,7 +53,7 @@ class ZipFileTarArchiveTests {
|
||||
@Test
|
||||
void createWhenOwnerIsNullThrowsException() throws Exception {
|
||||
File file = new File(this.tempDir, "test.zip");
|
||||
writeTestZip(file);
|
||||
writeTestZip(file, "");
|
||||
assertThatIllegalArgumentException().isThrownBy(() -> new ZipFileTarArchive(file, null))
|
||||
.withMessage("'owner' must not be null");
|
||||
}
|
||||
@@ -61,7 +62,7 @@ class ZipFileTarArchiveTests {
|
||||
void writeToAdaptsContent() throws Exception {
|
||||
Owner owner = Owner.of(123, 456);
|
||||
File file = new File(this.tempDir, "test.zip");
|
||||
writeTestZip(file);
|
||||
writeTestZip(file, "");
|
||||
TarArchive tarArchive = TarArchive.fromZip(file, owner);
|
||||
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
|
||||
tarArchive.writeTo(outputStream);
|
||||
@@ -81,12 +82,23 @@ class ZipFileTarArchiveTests {
|
||||
}
|
||||
}
|
||||
|
||||
private void writeTestZip(File file) throws IOException {
|
||||
@Test
|
||||
void writeToDoesNotIncludeEntriesThatWouldBeWrittenOutsideOfDestination() throws Exception {
|
||||
Owner owner = Owner.of(123, 456);
|
||||
File file = new File(this.tempDir, "test.zip");
|
||||
writeTestZip(file, "../");
|
||||
TarArchive tarArchive = TarArchive.fromZip(file, owner);
|
||||
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
|
||||
assertThatIllegalStateException().isThrownBy(() -> tarArchive.writeTo(outputStream))
|
||||
.withMessage("Malformed zip entry name '../spring/'");
|
||||
}
|
||||
|
||||
private void writeTestZip(File file, String prefix) throws IOException {
|
||||
try (ZipArchiveOutputStream zip = new ZipArchiveOutputStream(file)) {
|
||||
ZipArchiveEntry dirEntry = new ZipArchiveEntry("spring/");
|
||||
ZipArchiveEntry dirEntry = new ZipArchiveEntry(prefix + "spring/");
|
||||
zip.putArchiveEntry(dirEntry);
|
||||
zip.closeArchiveEntry();
|
||||
ZipArchiveEntry fileEntry = new ZipArchiveEntry("spring/boot");
|
||||
ZipArchiveEntry fileEntry = new ZipArchiveEntry(prefix + "spring/boot");
|
||||
fileEntry.setUnixMode(0755);
|
||||
zip.putArchiveEntry(fileEntry);
|
||||
zip.write("test".getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
Reference in New Issue
Block a user