Fix Android ZIP path traversal (#8541)

Author: danoli3

addons/ofxAndroid/Java/cc/openframeworks/OFZipUtil.java

@@ -16,21 +16,49 @@ public class OFZipUtil
 {
   private static final int  BUFFER_SIZE = 4096;
 
+  private static File resolveEntry(File outdir, String name) throws IOException
+  {
+    File canonicalOutdir = outdir.getCanonicalFile();
+    File destination = new File(canonicalOutdir, name).getCanonicalFile();
+    String outdirPath = canonicalOutdir.getPath();
+    String destinationPath = destination.getPath();
+
+    if (!destinationPath.equals(outdirPath)
+        && !destinationPath.startsWith(outdirPath + File.separator))
+      throw new IOException("Zip entry is outside the target directory: " + name);
+
+    return destination;
+  }
+
   public static void extractFile(ZipInputStream in, File outdir, String name) throws IOException
   {
     byte[] buffer = new byte[BUFFER_SIZE];
-    BufferedOutputStream out = new BufferedOutputStream(new FileOutputStream(new File(outdir,name)));
-    int count = -1;
-    while ((count = in.read(buffer)) != -1)
-      out.write(buffer, 0, count);
-    out.close();
+    File destination = resolveEntry(outdir, name);
+    try (BufferedOutputStream out = new BufferedOutputStream(new FileOutputStream(destination)))
+    {
+      int count;
+      while ((count = in.read(buffer)) != -1)
+        out.write(buffer, 0, count);
+    }
+  }
+
+  private static void createDirectories(File outdir, String path) throws IOException
+  {
+    File directory = resolveEntry(outdir, path);
+    if (!directory.exists() && !directory.mkdirs() && !directory.isDirectory())
+      throw new IOException("Could not create directory: " + directory);
   }
 
   public static void mkdirs(File outdir,String path)
   {
-    File d = new File(outdir, path);
-    if( !d.exists() )
-      d.mkdirs();
+    try
+    {
+      createDirectories(outdir, path);
+    }
+    catch (IOException e)
+    {
+      throw new IllegalArgumentException(e);
+    }
   }
 
   public static String dirpart(String name)
@@ -55,9 +83,11 @@ public static void extract(InputStream zipfile, File outdir)
       while ((entry = zin.getNextEntry()) != null)
       {
         name = entry.getName();
+        // Validate every entry before creating directories or opening files.
+        resolveEntry(outdir, name);
         if( entry.isDirectory() )
         {
-          mkdirs(outdir,name);
+          createDirectories(outdir,name);
           continue;
         }
         /* this part is necessary because file entry can come before
@@ -68,7 +98,7 @@ public static void extract(InputStream zipfile, File outdir)
          */
         dir = dirpart(name);
         if( dir != null )
-          mkdirs(outdir,dir);
+          createDirectories(outdir,dir);
 
         extractFile(zin, outdir, name);
       }

tests/android/OFZipUtil/run.sh

@@ -0,0 +1,13 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+REPO_ROOT="$(git rev-parse --show-toplevel)"
+TEST_CLASSES="$(mktemp -d)"
+
+javac -d "$TEST_CLASSES" \
+  "$REPO_ROOT/tests/android/OFZipUtil/stubs/android/util/Log.java" \
+  "$REPO_ROOT/tests/android/OFZipUtil/stubs/androidx/annotation/Keep.java" \
+  "$REPO_ROOT/addons/ofxAndroid/Java/cc/openframeworks/OFZipUtil.java" \
+  "$REPO_ROOT/tests/android/OFZipUtil/src/OFZipUtilTest.java"
+
+java -cp "$TEST_CLASSES" OFZipUtilTest

tests/android/OFZipUtil/src/OFZipUtilTest.java

@@ -0,0 +1,74 @@
+import cc.openframeworks.OFZipUtil;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
+
+public class OFZipUtilTest
+{
+  private static byte[] zip(String name, String contents, boolean directory) throws Exception
+  {
+    ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+    try (ZipOutputStream zip = new ZipOutputStream(bytes))
+    {
+      ZipEntry entry = new ZipEntry(name);
+      zip.putNextEntry(entry);
+      if (!directory)
+        zip.write(contents.getBytes(StandardCharsets.UTF_8));
+      zip.closeEntry();
+    }
+    return bytes.toByteArray();
+  }
+
+  private static void extract(File output, String name, String contents) throws Exception
+  {
+    OFZipUtil.extract(new ByteArrayInputStream(zip(name, contents, false)), output);
+  }
+
+  private static void assertMissing(Path path, String message)
+  {
+    if (Files.exists(path))
+      throw new AssertionError(message + ": " + path);
+  }
+
+  public static void main(String[] args) throws Exception
+  {
+    Path parent = Files.createTempDirectory("ofziputil-test-");
+    File output = Files.createDirectory(parent.resolve("output")).toFile();
+
+    extract(output, "nested/good.txt", "good");
+    Path good = output.toPath().resolve("nested/good.txt");
+    if (!Files.isRegularFile(good)
+        || !"good".equals(Files.readString(good, StandardCharsets.UTF_8)))
+      throw new AssertionError("A safe nested entry was not extracted correctly");
+
+    Path escaped = parent.resolve("escaped.txt");
+    extract(output, "../escaped.txt", "bad");
+    assertMissing(escaped, "A parent traversal entry escaped the output directory");
+
+    extract(output, "nested/../../escaped.txt", "bad");
+    assertMissing(escaped, "A nested parent traversal entry escaped the output directory");
+
+    Path absolute = parent.resolve("absolute.txt");
+    extract(output, absolute.toString(), "bad");
+    assertMissing(absolute, "An absolute entry escaped the output directory");
+
+    Path escapedDirectory = parent.resolve("escaped-directory");
+    OFZipUtil.extract(
+        new ByteArrayInputStream(zip("../escaped-directory/", "", true)), output);
+    assertMissing(escapedDirectory, "A directory entry escaped the output directory");
+
+    Path outside = Files.createDirectory(parent.resolve("outside"));
+    Files.createSymbolicLink(output.toPath().resolve("link"), outside);
+    extract(output, "link/escaped.txt", "bad");
+    assertMissing(outside.resolve("escaped.txt"),
+        "An entry escaped through a symbolic link in the output directory");
+
+    System.out.println("OFZipUtil security regression tests passed");
+  }
+}

tests/android/OFZipUtil/src/main.cpp

@@ -0,0 +1,5 @@
+#ifndef TARGET_ANDROID
+int main() {
+	return 0;
+}
+#endif

tests/android/OFZipUtil/stubs/android/util/Log.java

@@ -0,0 +1,9 @@
+package android.util;
+
+public class Log
+{
+  public static int i(String tag, String message)
+  {
+    return 0;
+  }
+}

tests/android/OFZipUtil/stubs/androidx/annotation/Keep.java

@@ -0,0 +1,5 @@
+package androidx.annotation;
+
+public @interface Keep
+{
+}