diff --git a/cdm/core/src/main/java/ucar/nc2/ft/remote/CdmrFeatureDataset.java b/cdm/core/src/main/java/ucar/nc2/ft/remote/CdmrFeatureDataset.java index e8b52b2310..782703cb85 100644 --- a/cdm/core/src/main/java/ucar/nc2/ft/remote/CdmrFeatureDataset.java +++ b/cdm/core/src/main/java/ucar/nc2/ft/remote/CdmrFeatureDataset.java @@ -118,7 +118,10 @@ private static org.jdom2.Document getCapabilities(String endpoint) throws IOExce org.jdom2.Document doc; try (InputStream in = CdmRemote.sendQuery(null, endpoint, "req=capabilities")) { SAXBuilder builder = new SAXBuilder(); + // this is the same as builder.setFeature("http://xml.org/sax/features/external-general-entities", false); builder.setExpandEntities(false); + builder.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + builder.setFeature("http://xml.org/sax/features/external-parameter-entities", false); doc = builder.build(in); // LOOK closes in when done ?? } catch (Throwable t) { diff --git a/cdm/zarr/src/main/java/thredds/filesystem/zarr/ControllerZip.java b/cdm/zarr/src/main/java/thredds/filesystem/zarr/ControllerZip.java index 0269645230..8bb4cf724c 100644 --- a/cdm/zarr/src/main/java/thredds/filesystem/zarr/ControllerZip.java +++ b/cdm/zarr/src/main/java/thredds/filesystem/zarr/ControllerZip.java @@ -123,11 +123,17 @@ protected static class FilteredIterator extends MFileIterator implements Iterato Path relativePath = file.getRelativePath(); for (ZipEntry entry : entries) { - Path entryPath = Paths.get(File.separator + entry.getName()); - if (!entryPath.startsWith(relativePath)) { - logger.warn(entryPath.toString() + " is not an entry in " + relativePath.toString()); + // entry path (relative to zip file), normalized to remove + // segments like '.' and '..' + Path normEntryPath = Paths.get(entry.getName()).normalize(); + // if normEntryPath starts with '..', it has tried to escape + // above its original starting level + if (normEntryPath.startsWith("..")) { + logger.warn(normEntryPath.toString() + " is not an entry in " + relativePath.toString()); continue; } + // anchor entry path to root of zip file + Path entryPath = Paths.get(File.separator + entry.getName()); // truncate path to one level below current path (i.e. direct child) Path childPath = entryPath.subpath(0, relativePath.getNameCount() + 1); fileNames.add(childPath); @@ -155,7 +161,14 @@ protected static class MFileIteratorLeaves extends MFileIterator implements Iter List entries = file.getLeafEntries(); for (ZipEntry entry : entries) { try { - this.files.add(new MFileZip(file.getRootPath() + File.separator + entry.getName())); + File entryFile = new File(file.getRootPath() + File.separator + entry.getName()); + if (entryFile.toPath().normalize().startsWith(file.getRootPath())) { + this.files.add(new MFileZip(entryFile.toString())); + } else { + // don't allow external references to escape the zip file + // (e.g., skip entries like ../path/outside/of/zip) + logger.warn("Zip entry references external entity in {}: {}. Skipping.", file.getPath(), entryFile); + } } catch (IOException ioe) { logger.error(ioe.getMessage(), ioe); } diff --git a/cdm/zarr/src/test/java/thredds/filesystem/zarr/TestControllerZip.java b/cdm/zarr/src/test/java/thredds/filesystem/zarr/TestControllerZip.java new file mode 100644 index 0000000000..8fa14edd9b --- /dev/null +++ b/cdm/zarr/src/test/java/thredds/filesystem/zarr/TestControllerZip.java @@ -0,0 +1,136 @@ +/* + * Copyright (c) 2026 University Corporation for Atmospheric Research/Unidata + * See LICENSE for license information. + */ + +package thredds.filesystem.zarr; + +import static com.google.common.truth.Truth.assertThat; + +import java.nio.file.Files; +import java.util.Arrays; +import java.util.Collections; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import thredds.inventory.CollectionConfig; +import thredds.inventory.MFile; + +import java.io.File; +import java.io.IOException; +import java.nio.file.DirectoryStream; +import java.util.ArrayList; +import java.util.List; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; + +public class TestControllerZip { + + @ClassRule + public static final TemporaryFolder tempFolder = new TemporaryFolder(); + + private static File zipFile, zipFileBad; + + @BeforeClass + public static void setUp() throws IOException { + zipFile = tempFolder.newFile("test.zip"); + try (ZipOutputStream zos = new ZipOutputStream(Files.newOutputStream(zipFile.toPath()))) { + // file1 (top level) + zos.putNextEntry(new ZipEntry("file1")); + zos.write("content1".getBytes()); + zos.closeEntry(); + + // dir1/file2 + zos.putNextEntry(new ZipEntry("dir1/file2")); + zos.write("content2".getBytes()); + zos.closeEntry(); + + // dir1/file3 + zos.putNextEntry(new ZipEntry("dir1/file3")); + zos.write("content3".getBytes()); + zos.closeEntry(); + } + + zipFileBad = tempFolder.newFile("test_bad.zip"); + try (ZipOutputStream zos = new ZipOutputStream(Files.newOutputStream(zipFileBad.toPath()))) { + // good entries + zos.putNextEntry(new ZipEntry("dir1/file_good1")); + zos.write("content1".getBytes()); + zos.closeEntry(); + + zos.putNextEntry(new ZipEntry("dir1/file_good2")); + zos.write("content2".getBytes()); + zos.closeEntry(); + + // bad entries + // reference outside of zip + zos.putNextEntry(new ZipEntry("../../file_bad")); + zos.write("content3".getBytes()); + zos.closeEntry(); + + // reference outside of zip + zos.putNextEntry(new ZipEntry("dir3/../../file_bad2")); + zos.write("content4".getBytes()); + zos.closeEntry(); + } + } + + @Test + public void testFilteredIteratorFiles() throws IOException { + ControllerZip controller = new ControllerZip(); + CollectionConfig mc = new CollectionConfig("test", zipFile.getAbsolutePath(), false, null, null); + try (DirectoryStream stream = controller.getInventoryTop(mc, false)) { + assertThat(stream).isNotNull(); + List names = new ArrayList<>(); + for (MFile mfile : stream) { + names.add(mfile.getName()); + } + // only one item in the top level of the zip + assertThat(names).containsExactly(File.separator + "file1"); + } + } + + @Test + public void testFilteredIteratorDirs() throws IOException { + ControllerZip controller = new ControllerZip(); + CollectionConfig mc = new CollectionConfig("test", zipFile.getAbsolutePath(), false, null, null); + try (DirectoryStream stream = controller.getSubdirs(mc, false)) { + assertThat(stream).isNotNull(); + List names = new ArrayList<>(); + for (MFile mfile : stream) { + names.add(mfile.getName()); + } + assertThat(names).containsExactly(File.separator + "dir1"); + } + } + + @Test + public void testFilteredFilesBad() throws IOException { + ControllerZip controller = new ControllerZip(); + CollectionConfig mc = new CollectionConfig("test", zipFileBad.getAbsolutePath(), false, null, null); + try (DirectoryStream stream = controller.getInventoryAll(mc, false)) { + assertThat(stream).isNotNull(); + List names = new ArrayList<>(); + for (MFile mfile : stream) { + names.add(mfile.getName()); + } + assertThat(names).containsExactlyElementsIn( + Arrays.asList(File.separator + "dir1/file_good1", File.separator + "dir1/file_good2")); + } + } + + @Test + public void testFilteredIteratorDirsBad() throws IOException { + ControllerZip controller = new ControllerZip(); + CollectionConfig mc = new CollectionConfig("test", zipFileBad.getAbsolutePath(), false, null, null); + try (DirectoryStream stream = controller.getSubdirs(mc, false)) { + assertThat(stream).isNotNull(); + List names = new ArrayList<>(); + for (MFile mfile : stream) { + names.add(mfile.getName()); + } + assertThat(names).containsExactlyElementsIn(Collections.singletonList(File.separator + "dir1")); + } + } +} diff --git a/opendap/src/main/java/opendap/dap/parsers/DDSXMLParser.java b/opendap/src/main/java/opendap/dap/parsers/DDSXMLParser.java index 0306a9bebf..7e166e1088 100644 --- a/opendap/src/main/java/opendap/dap/parsers/DDSXMLParser.java +++ b/opendap/src/main/java/opendap/dap/parsers/DDSXMLParser.java @@ -219,7 +219,10 @@ public void parse(InputStream is, DDS targetDDS, BaseTypeFactory fac, boolean va // get a jdom parser to parse and validate the XML document. SAXBuilder parser = new SAXBuilder(); + // this is the same as builder.setFeature("http://xml.org/sax/features/external-general-entities", false); parser.setExpandEntities(false); + parser.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false); // optionally turn on validation parser.setFeature("http://apache.org/xml/features/validation/schema", validation); diff --git a/uicdm/src/main/java/ucar/nc2/ui/op/WmsViewer.java b/uicdm/src/main/java/ucar/nc2/ui/op/WmsViewer.java index 9e01ad259e..0095239584 100644 --- a/uicdm/src/main/java/ucar/nc2/ui/op/WmsViewer.java +++ b/uicdm/src/main/java/ucar/nc2/ui/op/WmsViewer.java @@ -190,7 +190,10 @@ private boolean getCapabilities() { } SAXBuilder builder = new SAXBuilder(); + // this is the same as builder.setFeature("http://xml.org/sax/features/external-general-entities", false); builder.setExpandEntities(false); + builder.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + builder.setFeature("http://xml.org/sax/features/external-parameter-entities", false); Document tdoc = builder.build(method.getResponseAsStream()); Element root = tdoc.getRootElement(); parseGetCapabilities(root);