You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@plc4x.apache.org by sr...@apache.org on 2026/08/24 15:42:32 UTC
(plc4x) 01/04: fix(plc4j/knxnetip): bound how far a knxproj entry may expand
This is an automated email from the ASF dual-hosted git repository.
sruehl pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/plc4x.git
commit 8c7770e91b73421752b54925aa39669ab458bf5a
Author: Sebastian Rühl <sr...@apache.org>
AuthorDate: Mon Aug 24 17:21:04 2026 +0200
fix(plc4j/knxnetip): bound how far a knxproj entry may expand
A knxproj is a zip, and a zip entry says how large it will be once expanded. Nothing
checked either the claim or the outcome: seven places read an entry straight into the
XML parser or onto the disk. Compression ratios of a thousand to one are ordinary for
repetitive data, and the XML here is not only inflated but parsed into a document,
which costs several times the text again - so a small file could name a large amount
of memory and get it.
Both halves are checked now, because they fail differently. The declared size is
compared with the budget before anything is read, which turns away the honest case
cheaply. The bytes are then counted as they arrive, because the declaration is
written by whoever made the file and a header that lies would otherwise get exactly
what it asked for. The nested project zip that password-protected files carry is
read the same way.
Writing an entry out no longer goes through zip4j's extractFile, which would have
written whatever the entry expanded to before anybody could object; it streams under
the same budget.
128 MiB per entry. The largest knx_master seen in practice is a few tens of
megabytes, so there is room to spare, and a crafted file no longer decides.
---
.../apache/plc4x/java/knxnetip/ets/EtsParser.java | 99 ++++++++++++++++++++--
.../knxnetip/ets/EtsParserExpansionBoundTest.java | 82 ++++++++++++++++++
2 files changed, 174 insertions(+), 7 deletions(-)
diff --git a/plc4j/drivers/knxnetip/src/main/java/org/apache/plc4x/java/knxnetip/ets/EtsParser.java b/plc4j/drivers/knxnetip/src/main/java/org/apache/plc4x/java/knxnetip/ets/EtsParser.java
index 3f442eafe4..6fc2da1261 100644
--- a/plc4j/drivers/knxnetip/src/main/java/org/apache/plc4x/java/knxnetip/ets/EtsParser.java
+++ b/plc4j/drivers/knxnetip/src/main/java/org/apache/plc4x/java/knxnetip/ets/EtsParser.java
@@ -49,6 +49,9 @@ import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.stream.StreamResult;
import javax.xml.xpath.*;
import java.io.File;
+import java.io.FilterInputStream;
+import java.io.InputStream;
+import java.nio.file.StandardCopyOption;
import java.io.IOException;
import java.io.StringWriter;
import java.nio.file.Files;
@@ -66,6 +69,21 @@ public class EtsParser {
private static final Logger LOGGER = LoggerFactory.getLogger(EtsParser.class);
+ /**
+ * How much a single entry of a knxproj may expand to.
+ *
+ * <p>A zip entry says how big it will be once inflated, and it can say anything. Compression
+ * ratios of a thousand to one are ordinary for repetitive data, so a small file can name a
+ * large one - and the XML here is not merely inflated but parsed into a document, which costs
+ * several times the text again.</p>
+ *
+ * <p>The declared size is checked before anything is read, and the bytes are counted as they
+ * arrive, because the declaration is written by whoever made the file. The largest knx_master
+ * seen in practice is a few tens of megabytes, so this leaves room to spare while keeping a
+ * crafted file from deciding how much memory we use.</p>
+ */
+ private static final long MAX_UNCOMPRESSED_ENTRY_SIZE = 128L * 1024 * 1024;
+
private static boolean isEts6Schema(String schemaVersion) {
if (schemaVersion == null) {
return false;
@@ -77,6 +95,73 @@ public class EtsParser {
}
}
+ /**
+ * Reads an entry, refusing one that says it is larger than the budget and stopping if it turns
+ * out to be larger than it said.
+ */
+ private static InputStream boundedEntryStream(ZipFile zipFile, FileHeader header)
+ throws IOException {
+ long declared = header.getUncompressedSize();
+ if (declared > MAX_UNCOMPRESSED_ENTRY_SIZE) {
+ throw new PlcRuntimeException(String.format(
+ "Refusing to read '%s': it declares %d bytes once expanded, over the %d byte limit",
+ header.getFileName(), declared, MAX_UNCOMPRESSED_ENTRY_SIZE));
+ }
+ return new BoundedInputStream(zipFile.getInputStream(header), MAX_UNCOMPRESSED_ENTRY_SIZE,
+ header.getFileName());
+ }
+
+ /**
+ * Writes an entry out under the same budget. Not zip4j's extractFile, which would have written
+ * whatever the entry expanded to before anybody could object.
+ */
+ private static void extractBounded(ZipFile zipFile, FileHeader header, Path target)
+ throws IOException {
+ try (InputStream in = boundedEntryStream(zipFile, header)) {
+ Files.copy(in, target, StandardCopyOption.REPLACE_EXISTING);
+ }
+ }
+
+ /** Counts what passes through and stops once it is more than was allowed. */
+ static final class BoundedInputStream extends FilterInputStream {
+
+ private final long limit;
+ private final String entryName;
+ private long read;
+
+ BoundedInputStream(InputStream in, long limit, String entryName) {
+ super(in);
+ this.limit = limit;
+ this.entryName = entryName;
+ }
+
+ private void count(long justRead) throws IOException {
+ if (justRead < 0) {
+ return;
+ }
+ read += justRead;
+ if (read > limit) {
+ throw new IOException(String.format(
+ "'%s' expanded past the %d byte limit, whatever its header claimed",
+ entryName, limit));
+ }
+ }
+
+ @Override
+ public int read() throws IOException {
+ int b = super.read();
+ count(b < 0 ? 0 : 1);
+ return b;
+ }
+
+ @Override
+ public int read(byte[] b, int off, int len) throws IOException {
+ int n = super.read(b, off, len);
+ count(n);
+ return n;
+ }
+ }
+
public EtsModel parse(File knxprojFile, String password) {
try {
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
@@ -97,8 +182,8 @@ public class EtsParser {
if(knxMasterFileHeader == null) {
throw new PlcRuntimeException("Error accessing knx_master.xml file.");
}
- zipFile.extractFile("knx_master.xml", tempDir.toFile().getAbsolutePath());
File knxMasterFile = new File(tempDir.toFile(), "knx_master.xml");
+ extractBounded(zipFile, knxMasterFileHeader, knxMasterFile.toPath());
// If the file contains: <KNX xmlns="http://knx.org/xml/project/21"> it's an ETS6 file
// In all other cases, we'll treat it as ETS5
String etsSchemaVersion = null;
@@ -136,8 +221,8 @@ public class EtsParser {
throw new PlcRuntimeException(String.format("Error accessing project header file. Project file '%s/project.xml' and '%s.zip' don't exist.", projectNumber, projectNumber));
}
// Dump the encrypted zip to a temp file.
- zipFile.extractFile(projectNumber + ".zip", tempDir.toFile().getAbsolutePath());
File tempFile = new File(tempDir.toFile(), projectNumber + ".zip");
+ extractBounded(zipFile, encryptedProjectFileHeader, tempFile.toPath());
// Unzip the archive inside the archive.
//noinspection CommentedOutCode
@@ -146,7 +231,7 @@ public class EtsParser {
if (compressedProjectFileHeader == null) {
throw new PlcRuntimeException(String.format("Error accessing project header file: Project file 'project.xml' inside '%s.zip'.", projectNumber));
}
- projectHeaderDoc = builder.parse(projectZipFile.getInputStream(compressedProjectFileHeader));
+ projectHeaderDoc = builder.parse(boundedEntryStream(projectZipFile, compressedProjectFileHeader));
// TODO: Leave this in, as it helps debug problems, wen a new ETS version comes out.
// Print the document content to the console.
@@ -157,19 +242,19 @@ public class EtsParser {
if (projectFileFileHeader == null) {
throw new PlcRuntimeException("Error accessing project file.");
}
- projectDoc = builder.parse(projectZipFile.getInputStream(projectFileFileHeader));
+ projectDoc = builder.parse(boundedEntryStream(projectZipFile, projectFileFileHeader));
// TODO: Leave this in, as it helps debug problems, wen a new ETS version comes out.
//System.out.println("Project Doc:");
//printDocument(projectDoc);
}
} else {
- projectHeaderDoc = builder.parse(zipFile.getInputStream(projectFileHeader));
+ projectHeaderDoc = builder.parse(boundedEntryStream(zipFile, projectFileHeader));
FileHeader projectFileFileHeader = zipFile.getFileHeader(projectNumber + "/0.xml");
if (projectFileFileHeader == null) {
throw new PlcRuntimeException("Error accessing project file.");
}
- projectDoc = builder.parse(zipFile.getInputStream(projectFileFileHeader));
+ projectDoc = builder.parse(boundedEntryStream(zipFile, projectFileFileHeader));
}
final XPathExpression xpathGroupAddressStyle = xPath.compile("/KNX/Project/ProjectInformation/@GroupAddressStyle");
Attr groupAddressStyle = (Attr) xpathGroupAddressStyle.evaluate(projectHeaderDoc, XPathConstants.NODE);
@@ -182,7 +267,7 @@ public class EtsParser {
if (knxMasterDataFileFileHeader == null) {
throw new PlcRuntimeException("Error accessing KNX master file.");
}
- Document knxMasterDoc = builder.parse(zipFile.getInputStream(knxMasterDataFileFileHeader));
+ Document knxMasterDoc = builder.parse(boundedEntryStream(zipFile, knxMasterDataFileFileHeader));
final XPathExpression xpathDatapointSubtype = xPath.compile("//DatapointSubtype");
NodeList datapointSubtypeNodes = (NodeList) xpathDatapointSubtype.evaluate(knxMasterDoc, XPathConstants.NODESET);
diff --git a/plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/ets/EtsParserExpansionBoundTest.java b/plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/ets/EtsParserExpansionBoundTest.java
new file mode 100644
index 0000000000..ee74257ab7
--- /dev/null
+++ b/plc4j/drivers/knxnetip/src/test/java/org/apache/plc4x/java/knxnetip/ets/EtsParserExpansionBoundTest.java
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.knxnetip.ets;
+
+import org.apache.plc4x.java.knxnetip.ets.EtsParser.BoundedInputStream;
+import org.junit.jupiter.api.Test;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * A zip entry declares how large it will be once expanded, and that declaration is written by
+ * whoever made the file. So the bytes are counted as they arrive as well.
+ */
+class EtsParserExpansionBoundTest {
+
+ private static InputStream of(int bytes) {
+ return new ByteArrayInputStream(new byte[bytes]);
+ }
+
+ @Test
+ void anEntryWithinItsBudgetIsReadWhole() throws IOException {
+ try (BoundedInputStream in = new BoundedInputStream(of(1000), 4096, "knx_master.xml")) {
+ assertEquals(1000, in.readAllBytes().length);
+ }
+ }
+
+ @Test
+ void anEntryThatExpandsPastTheBudgetStops() {
+ IOException thrown = assertThrows(IOException.class, () -> {
+ try (BoundedInputStream in = new BoundedInputStream(of(8192), 4096, "0.xml")) {
+ in.readAllBytes();
+ }
+ });
+ assertTrue(thrown.getMessage().contains("0.xml"),
+ "the failure should name the entry, but was: " + thrown.getMessage());
+ assertTrue(thrown.getMessage().contains("whatever its header claimed"),
+ "and should say why it is not trusting the header");
+ }
+
+ @Test
+ void theBudgetIsEnforcedByteByByteToo() {
+ assertThrows(IOException.class, () -> {
+ try (BoundedInputStream in = new BoundedInputStream(of(64), 8, "single-byte-reads")) {
+ for (int i = 0; i < 64; i++) {
+ in.read();
+ }
+ }
+ });
+ }
+
+ @Test
+ void readingExactlyTheBudgetIsAllowed() {
+ assertDoesNotThrow(() -> {
+ try (BoundedInputStream in = new BoundedInputStream(of(4096), 4096, "exact")) {
+ assertEquals(4096, in.readAllBytes().length);
+ }
+ });
+ }
+}