From 9b1761f71f043e0a9b2030a9263bdc00a1971d2a Mon Sep 17 00:00:00 2001 From: skylot <118523+skylot@users.noreply.github.com> Date: Sun, 27 Sep 2020 21:10:30 +0300 Subject: [PATCH] fix: prevent zipbomb forged headers attacks (#980, PR #982) --- .../main/java/jadx/api/ResourcesLoader.java | 17 ++---- .../src/main/java/jadx/core/clsp/ClsSet.java | 26 ++++----- .../java/jadx/core/xmlgen/ResourcesSaver.java | 5 +- .../jadx/plugins/input/dex/DexFileLoader.java | 22 ++++---- .../api/plugins/utils/LimitedInputStream.java | 53 +++++++++++++++++++ .../jadx/api/plugins/utils/ZipSecurity.java | 42 +++++++++++++++ 6 files changed, 125 insertions(+), 40 deletions(-) create mode 100644 jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/utils/LimitedInputStream.java diff --git a/jadx-core/src/main/java/jadx/api/ResourcesLoader.java b/jadx-core/src/main/java/jadx/api/ResourcesLoader.java index 8c67cd64a..25d8cdab9 100644 --- a/jadx-core/src/main/java/jadx/api/ResourcesLoader.java +++ b/jadx-core/src/main/java/jadx/api/ResourcesLoader.java @@ -7,7 +7,6 @@ import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; -import java.util.Enumeration; import java.util.List; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; @@ -69,7 +68,7 @@ public final class ResourcesLoader { if (!ZipSecurity.isValidZipEntry(entry)) { return null; } - try (InputStream inputStream = new BufferedInputStream(zipFile.getInputStream(entry))) { + try (InputStream inputStream = ZipSecurity.getInputStreamForEntry(zipFile, entry)) { return decoder.decode(entry.getSize(), inputStream); } } @@ -129,17 +128,9 @@ public final class ResourcesLoader { return; } if (FileUtils.isZipFile(file)) { - try (ZipFile zip = new ZipFile(file)) { - Enumeration entries = zip.entries(); - while (entries.hasMoreElements()) { - ZipEntry entry = entries.nextElement(); - if (ZipSecurity.isValidZipEntry(entry)) { - addEntry(list, file, entry); - } - } - } catch (Exception e) { - LOG.warn("Failed to open zip file: {}", file.getAbsolutePath()); - } + ZipSecurity.visitZipEntries(file, (zipFile, entry) -> { + addEntry(list, file, entry); + }); } else { addResourceFile(list, file); } diff --git a/jadx-core/src/main/java/jadx/core/clsp/ClsSet.java b/jadx-core/src/main/java/jadx/core/clsp/ClsSet.java index dd7fdd927..ff0363349 100644 --- a/jadx-core/src/main/java/jadx/core/clsp/ClsSet.java +++ b/jadx-core/src/main/java/jadx/core/clsp/ClsSet.java @@ -325,22 +325,22 @@ public class ClsSet { private void load(File input) throws IOException, DecodeException { String name = input.getName(); - try (InputStream inputStream = new FileInputStream(input)) { - if (name.endsWith(CLST_EXTENSION)) { + if (name.endsWith(CLST_EXTENSION)) { + try (InputStream inputStream = new FileInputStream(input)) { load(inputStream); - } else if (name.endsWith(".jar")) { - try (ZipInputStream in = new ZipInputStream(inputStream)) { - ZipEntry entry = in.getNextEntry(); - while (entry != null) { - if (entry.getName().endsWith(CLST_EXTENSION) && ZipSecurity.isValidZipEntry(entry)) { - load(in); - } - entry = in.getNextEntry(); + } + } else if (name.endsWith(".jar")) { + ZipSecurity.readZipEntries(input, (entry, in) -> { + if (entry.getName().endsWith(CLST_EXTENSION)) { + try { + load(in); + } catch (Exception e) { + throw new JadxRuntimeException("Failed to load jadx class set"); } } - } else { - throw new JadxRuntimeException("Unknown file format: " + name); - } + }); + } else { + throw new JadxRuntimeException("Unknown file format: " + name); } } diff --git a/jadx-core/src/main/java/jadx/core/xmlgen/ResourcesSaver.java b/jadx-core/src/main/java/jadx/core/xmlgen/ResourcesSaver.java index 965284f78..1fe23b4dd 100644 --- a/jadx-core/src/main/java/jadx/core/xmlgen/ResourcesSaver.java +++ b/jadx-core/src/main/java/jadx/core/xmlgen/ResourcesSaver.java @@ -2,6 +2,7 @@ package jadx.core.xmlgen; import java.io.File; import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.StandardCopyOption; import org.slf4j.Logger; @@ -89,9 +90,11 @@ public class ResourcesSaver implements Runnable { private void saveResourceFile(ResourceFile resFile, File outFile) throws JadxException { ResourcesLoader.decodeStream(resFile, (size, is) -> { + Path target = outFile.toPath(); try { - Files.copy(is, outFile.toPath(), StandardCopyOption.REPLACE_EXISTING); + Files.copy(is, target, StandardCopyOption.REPLACE_EXISTING); } catch (Exception e) { + Files.deleteIfExists(target); // delete partially written file throw new JadxRuntimeException("Resource file save error", e); } return null; diff --git a/jadx-plugins/jadx-dex-input/src/main/java/jadx/plugins/input/dex/DexFileLoader.java b/jadx-plugins/jadx-dex-input/src/main/java/jadx/plugins/input/dex/DexFileLoader.java index 9bcd8ac75..65bfedba7 100644 --- a/jadx-plugins/jadx-dex-input/src/main/java/jadx/plugins/input/dex/DexFileLoader.java +++ b/jadx-plugins/jadx-dex-input/src/main/java/jadx/plugins/input/dex/DexFileLoader.java @@ -11,7 +11,6 @@ import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.stream.Collectors; -import java.util.zip.ZipFile; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -66,19 +65,16 @@ public class DexFileLoader { private static List collectDexFromZip(File file) { List result = new ArrayList<>(); - try (ZipFile zip = new ZipFile(file)) { - zip.stream() - .filter(entry -> !entry.isDirectory()) - .filter(ZipSecurity::isValidZipEntry) - .forEach(entry -> { - try (InputStream in = zip.getInputStream(entry)) { - result.addAll(checkFileMagic(null, in, entry.getName())); - } catch (Exception e) { - LOG.error("Failed to read zip entry: {}", entry, e); - } - }); + try { + ZipSecurity.readZipEntries(file, (entry, in) -> { + try { + result.addAll(checkFileMagic(null, in, entry.getName())); + } catch (Exception e) { + LOG.error("Failed to read zip entry: {}", entry, e); + } + }); } catch (Exception e) { - LOG.warn("Failed to open zip file: {}", file.getAbsolutePath()); + LOG.error("Failed to process zip file: {}", file.getAbsolutePath(), e); } return result; } diff --git a/jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/utils/LimitedInputStream.java b/jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/utils/LimitedInputStream.java new file mode 100644 index 000000000..dfa4ba5ee --- /dev/null +++ b/jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/utils/LimitedInputStream.java @@ -0,0 +1,53 @@ +package jadx.api.plugins.utils; + +import java.io.FilterInputStream; +import java.io.IOException; +import java.io.InputStream; + +public class LimitedInputStream extends FilterInputStream { + + private final long maxSize; + + private long currentPos; + + protected LimitedInputStream(InputStream in, long maxSize) { + super(in); + this.maxSize = maxSize; + } + + private void checkPos() { + if (currentPos > maxSize) { + throw new IllegalStateException("Read limit exceeded"); + } + } + + @Override + public int read() throws IOException { + int data = super.read(); + if (data != -1) { + currentPos++; + checkPos(); + } + return data; + } + + @Override + public int read(byte[] b, int off, int len) throws IOException { + int count = super.read(b, off, len); + if (count > 0) { + currentPos += count; + checkPos(); + } + return count; + } + + @Override + public long skip(long n) throws IOException { + long skipped = super.skip(n); + if (skipped != 0) { + currentPos += skipped; + checkPos(); + } + return skipped; + } +} diff --git a/jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/utils/ZipSecurity.java b/jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/utils/ZipSecurity.java index 965502ab2..983b0bcfe 100644 --- a/jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/utils/ZipSecurity.java +++ b/jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/utils/ZipSecurity.java @@ -1,8 +1,13 @@ package jadx.api.plugins.utils; +import java.io.BufferedInputStream; import java.io.File; import java.io.IOException; +import java.io.InputStream; +import java.util.Enumeration; +import java.util.function.BiConsumer; import java.util.zip.ZipEntry; +import java.util.zip.ZipFile; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -12,6 +17,7 @@ public class ZipSecurity { // size of uncompressed zip entry shouldn't be bigger of compressed in MAX_SIZE_DIFF times private static final int MAX_SIZE_DIFF = 100; + private static final int MAX_ENTRIES_COUNT = 100_000; private ZipSecurity() { } @@ -73,4 +79,40 @@ public class ZipSecurity { return isValidZipEntryName(entry.getName()) && !isZipBomb(entry); } + + public static InputStream getInputStreamForEntry(ZipFile zipFile, ZipEntry entry) throws IOException { + InputStream in = zipFile.getInputStream(entry); + LimitedInputStream limited = new LimitedInputStream(in, entry.getSize()); + return new BufferedInputStream(limited); + } + + public static void visitZipEntries(File file, BiConsumer visitor) { + try (ZipFile zip = new ZipFile(file)) { + Enumeration entries = zip.entries(); + int entriesProcessed = 0; + while (entries.hasMoreElements()) { + ZipEntry entry = entries.nextElement(); + if (!entry.isDirectory() && isValidZipEntry(entry)) { + visitor.accept(zip, entry); + entriesProcessed++; + if (entriesProcessed > MAX_ENTRIES_COUNT) { + throw new IllegalStateException("Zip entries count limit exceeded: " + MAX_ENTRIES_COUNT + + ", last entry: " + entry.getName()); + } + } + } + } catch (Exception e) { + throw new RuntimeException("Failed to process zip file: " + file.getAbsolutePath(), e); + } + } + + public static void readZipEntries(File file, BiConsumer visitor) { + visitZipEntries(file, (zip, entry) -> { + try (InputStream in = getInputStreamForEntry(zip, entry)) { + visitor.accept(entry, in); + } catch (Exception e) { + throw new RuntimeException("Error process zip entry: " + entry.getName()); + } + }); + } }