Prevent arbitrary file writes with malicious resource names. (#3484)

* refactor: rename sanitize function * fix: expose getDir * fix: safe handling of untrusted resource names - fixes: GHSA-2hqv-2xv4-5h5w * test: sample file for GHSA-2hqv-2xv4-5h5w * refactor: avoid detection of absolute files for resource check * chore: enable info mode on gradle * test: skip test on windows * chore: debug windows handling * fix: normalize entry with file separators * fix: normalize filepath after cleansing * chore: Android paths are not OS specific * refactor: use java.nio for path traversal checking * chore: align path separator on Windows for Zip files * chore: rework towards basic directory traversal * chore: remove '--info' on build.yml
iBotPeaches · Jan 5, 2024 · 93e7d6b · 93e7d6b
1 parent 077b200
commit 93e7d6b
Show file tree

Hide file tree

Showing 10 changed files with 100 additions and 20 deletions.
diff --git a/brut.apktool/apktool-lib/src/main/java/brut/androlib/ApkBuilder.java b/brut.apktool/apktool-lib/src/main/java/brut/androlib/ApkBuilder.java
@@ -482,7 +482,7 @@ private void copyUnknownFiles(ZipOutputStream outputFile, Map<String, String> fi
             File inputFile;
 
             try {
-                inputFile = new File(unknownFileDir, BrutIO.sanitizeUnknownFile(unknownFileDir, unknownFileInfo.getKey()));
+                inputFile = new File(unknownFileDir, BrutIO.sanitizeFilepath(unknownFileDir, unknownFileInfo.getKey()));
             } catch (RootUnknownFileException | InvalidUnknownFileException | TraversalUnknownFileException exception) {
                 LOGGER.warning(String.format("Skipping file %s (%s)", unknownFileInfo.getKey(), exception.getMessage()));
                 continue;

diff --git a/brut.apktool/apktool-lib/src/main/java/brut/androlib/res/ResourcesDecoder.java b/brut.apktool/apktool-lib/src/main/java/brut/androlib/res/ResourcesDecoder.java
@@ -154,12 +154,12 @@ public void decodeResources(File outDir) throws AndrolibException {
         decoders.setDecoder("xml", new XmlPullStreamDecoder(axmlParser, getResXmlSerializer()));
 
         ResFileDecoder fileDecoder = new ResFileDecoder(decoders);
-        Directory in, out;
+        Directory in, out, outRes;
 
         try {
             out = new FileDirectory(outDir);
             in = mApkInfo.getApkFile().getDirectory();
-            out = out.createDir("res");
+            outRes = out.createDir("res");
         } catch (DirectoryException ex) {
             throw new AndrolibException(ex);
         }
@@ -169,14 +169,14 @@ public void decodeResources(File outDir) throws AndrolibException {
 
             LOGGER.info("Decoding file-resources...");
             for (ResResource res : pkg.listFiles()) {
-                fileDecoder.decode(res, in, out, mResFileMapping);
+                fileDecoder.decode(res, in, outRes, mResFileMapping);
             }
 
             LOGGER.info("Decoding values */* XMLs...");
             for (ResValuesFile valuesFile : pkg.listValuesFiles()) {
-                generateValuesFile(valuesFile, out, xmlSerializer);
+                generateValuesFile(valuesFile, outRes, xmlSerializer);
             }
-            generatePublicXml(pkg, out, xmlSerializer);
+            generatePublicXml(pkg, outRes, xmlSerializer);
         }
 
         AndrolibException decodeError = axmlParser.getFirstError();

diff --git a/brut.apktool/apktool-lib/src/main/java/brut/androlib/res/decoder/ResFileDecoder.java b/brut.apktool/apktool-lib/src/main/java/brut/androlib/res/decoder/ResFileDecoder.java
@@ -25,6 +25,7 @@
 import brut.directory.DirUtil;
 import brut.directory.Directory;
 import brut.directory.DirectoryException;
+import brut.util.BrutIO;
 
 import java.io.*;
 import java.util.Map;
@@ -44,8 +45,15 @@ public void decode(ResResource res, Directory inDir, Directory outDir, Map<Strin
         ResFileValue fileValue = (ResFileValue) res.getValue();
         String inFilePath = fileValue.toString();
         String inFileName = fileValue.getStrippedPath();
-        String outResName = res.getFilePath();
         String typeName = res.getResSpec().getType().getName();
+        String outResName = res.getFilePath();
+
+        if (BrutIO.detectPossibleDirectoryTraversal(outResName)) {
+            outResName = inFileName;
+            LOGGER.warning(String.format(
+                "Potentially malicious file path: %s, using instead %s", res.getFilePath(), outResName
+            ));
+        }
 
         String ext = null;
         String outFileName;

diff --git a/...pktool/apktool-lib/src/test/java/brut/androlib/decode/ResourceDirectoryTraversalTest.java b/...pktool/apktool-lib/src/test/java/brut/androlib/decode/ResourceDirectoryTraversalTest.java
@@ -0,0 +1,65 @@
+/*
+ *  Copyright (C) 2010 Ryszard Wiśniewski <brut.alll@gmail.com>
+ *  Copyright (C) 2010 Connor Tumbleson <connor.tumbleson@gmail.com>
+ *
+ *  Licensed 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 brut.androlib.decode;
+
+import brut.androlib.ApkDecoder;
+import brut.androlib.BaseTest;
+import brut.androlib.Config;
+import brut.androlib.TestUtils;
+import brut.common.BrutException;
+import brut.directory.ExtFile;
+import brut.util.OS;
+import brut.util.OSDetection;
+import org.junit.AfterClass;
+import org.junit.Assume;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.IOException;
+
+import static org.junit.Assert.assertTrue;
+
+public class ResourceDirectoryTraversalTest extends BaseTest {
+
+    @BeforeClass
+    public static void beforeClass() throws Exception {
+        TestUtils.cleanFrameworkFile();
+        sTmpDir = new ExtFile(OS.createTempDirectory());
+        TestUtils.copyResourceDir(ResourceDirectoryTraversalTest.class, "decode/arbitrary-write/", sTmpDir);
+        Assume.assumeFalse(OSDetection.isWindows());
+    }
+
+    @AfterClass
+    public static void afterClass() throws BrutException {
+        OS.rmdir(sTmpDir);
+    }
+
+    @Test
+    public void checkIfMaliciousRawFileIsDisassembledProperly() throws BrutException, IOException {
+        String apk = "GHSA-2hqv-2xv4-5h5w.apk";
+
+        Config config = Config.getDefaultConfig();
+        config.forceDelete = true;
+        ApkDecoder apkDecoder = new ApkDecoder(config, new File(sTmpDir + File.separator + apk));
+        File outDir = new File(sTmpDir + File.separator + apk + ".out");
+        apkDecoder.decode(outDir);
+
+        File pocTestFile =  new File(outDir,"res/raw/poc");
+        assertTrue(pocTestFile.exists());
+    }
+}
diff --git a/brut.apktool/apktool-lib/src/test/java/brut/androlib/util/UnknownDirectoryTraversalTest.java b/brut.apktool/apktool-lib/src/test/java/brut/androlib/util/UnknownDirectoryTraversalTest.java
@@ -51,7 +51,7 @@ public static void afterClass() throws BrutException {
 
     @Test
     public void validFileTest() throws IOException, BrutException {
-        String validFilename = BrutIO.sanitizeUnknownFile(sTmpDir, "file");
+        String validFilename = BrutIO.sanitizeFilepath(sTmpDir, "file");
         assertEquals(validFilename, "file");
 
         File validFile = new File(sTmpDir, validFilename);
@@ -60,18 +60,18 @@ public void validFileTest() throws IOException, BrutException {
 
     @Test(expected = TraversalUnknownFileException.class)
     public void invalidBackwardFileTest() throws IOException, BrutException {
-        BrutIO.sanitizeUnknownFile(sTmpDir, "../file");
+        BrutIO.sanitizeFilepath(sTmpDir, "../file");
     }
 
     @Test(expected = RootUnknownFileException.class)
     public void invalidRootFileTest() throws IOException, BrutException {
         String rootLocation = OSDetection.isWindows() ? "C:/" : File.separator;
-        BrutIO.sanitizeUnknownFile(sTmpDir, rootLocation + "file");
+        BrutIO.sanitizeFilepath(sTmpDir, rootLocation + "file");
     }
 
     @Test(expected = InvalidUnknownFileException.class)
     public void noFilePassedTest() throws IOException, BrutException {
-        BrutIO.sanitizeUnknownFile(sTmpDir, "");
+        BrutIO.sanitizeFilepath(sTmpDir, "");
     }
 
     @Test(expected = TraversalUnknownFileException.class)
@@ -83,12 +83,12 @@ public void invalidBackwardPathOnWindows() throws IOException, BrutException {
             invalidPath = "..\\..\\app.exe";
         }
 
-        BrutIO.sanitizeUnknownFile(sTmpDir, invalidPath);
+        BrutIO.sanitizeFilepath(sTmpDir, invalidPath);
     }
 
     @Test
     public void validDirectoryFileTest() throws IOException, BrutException {
-        String validFilename = BrutIO.sanitizeUnknownFile(sTmpDir, "dir" + File.separator + "file");
+        String validFilename = BrutIO.sanitizeFilepath(sTmpDir, "dir" + File.separator + "file");
         assertEquals("dir" + File.separator + "file", validFilename);
     }
 }
diff --git a/brut.apktool/apktool-lib/src/test/resources/decode/arbitrary-write/GHSA-2hqv-2xv4-5h5w.apk b/brut.apktool/apktool-lib/src/test/resources/decode/arbitrary-write/GHSA-2hqv-2xv4-5h5w.apk
diff --git a/brut.j.dir/src/main/java/brut/directory/DirUtil.java b/brut.j.dir/src/main/java/brut/directory/DirUtil.java
@@ -89,7 +89,7 @@ public static void copyToDir(Directory in, File out, String fileName)
             } else if (!in.containsDir(fileName) && !in.containsFile(fileName)) {
                 // Skip copies of directories/files not found.
             } else {
-                String cleanedFilename = BrutIO.sanitizeUnknownFile(out, fileName);
+                String cleanedFilename = BrutIO.sanitizeFilepath(out, fileName);
                 if (! cleanedFilename.isEmpty()) {
                     File outFile = new File(out, cleanedFilename);
                     //noinspection ResultOfMethodCallIgnored

diff --git a/brut.j.dir/src/main/java/brut/directory/FileDirectory.java b/brut.j.dir/src/main/java/brut/directory/FileDirectory.java
@@ -119,7 +119,7 @@ private void loadAll() {
         }
     }
 
-    private File getDir() {
+    public File getDir() {
         return mDir;
     }
 }
diff --git a/brut.j.dir/src/main/java/brut/directory/ZipUtils.java b/brut.j.dir/src/main/java/brut/directory/ZipUtils.java
@@ -59,8 +59,8 @@ private static void processFolder(final File folder, final ZipOutputStream zipOu
             throws BrutException, IOException {
         for (final File file : folder.listFiles()) {
             if (file.isFile()) {
-                final String cleanedPath = BrutIO.sanitizeUnknownFile(folder, file.getPath().substring(prefixLength));
-                final ZipEntry zipEntry = new ZipEntry(BrutIO.normalizePath(cleanedPath));
+                final String cleanedPath = BrutIO.sanitizeFilepath(folder, file.getPath().substring(prefixLength));
+                final ZipEntry zipEntry = new ZipEntry(BrutIO.adaptSeparatorToUnix(cleanedPath));
 
                 // aapt binary by default takes in parameters via -0 arsc to list extensions that shouldn't be
                 // compressed. We will replicate that behavior

diff --git a/brut.j.util/src/main/java/brut/util/BrutIO.java b/brut.j.util/src/main/java/brut/util/BrutIO.java
@@ -74,8 +74,8 @@ public static CRC32 calculateCrc(InputStream input) throws IOException {
         return crc;
     }
 
-    public static String sanitizeUnknownFile(final File directory, final String entry) throws IOException, BrutException {
-        if (entry.length() == 0) {
+    public static String sanitizeFilepath(final File directory, final String entry) throws IOException, BrutException {
+        if (entry.isEmpty()) {
             throw new InvalidUnknownFileException("Invalid Unknown File");
         }
 
@@ -94,7 +94,14 @@ public static String sanitizeUnknownFile(final File directory, final String entr
         return canonicalEntryPath.substring(canonicalDirPath.length());
     }
 
-    public static String normalizePath(String path) {
+    public static boolean detectPossibleDirectoryTraversal(String entry) {
+        if (OSDetection.isWindows()) {
+            return entry.contains("..\\") || entry.contains("\\..");
+        }
+        return entry.contains("../") || entry.contains("/..");
+    }
+
+    public static String adaptSeparatorToUnix(String path) {
         char separator = File.separatorChar;
 
         if (separator != '/') {