CVE-2024-21633: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: Apktool is a tool for reverse engineering Android APK files. In versions 2.9.1 and prior, Apktool infers resource files' output path according to their resource names which can be manipulated by attacker to place files at desired location on the system Apktool runs on. Affected environments are those in which an attacker may write/overwrite any file that user has write access, and either user name is known or cwd is under user folder. Commit d348c43b24a9de350ff6e5bd610545a10c1fc712 contains a patch for this issue.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 7.8
Published: 2024-01-03T17:15Z
Repository: https://github.com/iBotPeaches/Apktool
Language: Java
Files changed: brut.apktool/apktool-lib/src/main/java/brut/androlib/ApkBuilder.java, brut.apktool/apktool-lib/src/main/java/brut/androlib/res/ResourcesDecoder.java, brut.apktool/apktool-lib/src/main/java/brut/androlib/res/decoder/ResFileDecoder.java, brut.apktool/apktool-lib/src/test/java/brut/androlib/decode/ResourceDirectoryTraversalTest.java, brut.apktool/apktool-lib/src/test/java/brut/androlib/util/UnknownDirectoryTraversalTest.java, brut.j.dir/src/main/java/brut/directory/DirUtil.java, brut.j.dir/src/main/java/brut/directory/FileDirectory.java, brut.j.dir/src/main/java/brut/directory/ZipUtils.java, brut.j.util/src/main/java/brut/util/BrutIO.java
Commit date: 2024-01-02 06:11:03 -0500
Commit message: 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
Diff: 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
index 12153e25..5c9e3de4 100644
--- a/brut.apktool/apktool-lib/src/main/java/brut/androlib/ApkBuilder.java
+++ b/brut.apktool/apktool-lib/src/main/java/brut/androlib/ApkBuilder.java
@@ -478,11 +478,11 @@ public class ApkBuilder {
         // loop through unknown files
         for (Map.Entry<String,String> unknownFileInfo : files.entrySet()) {
             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
index a9507b6b..7ee7f77e 100644
--- 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
@@ -152,33 +152,33 @@ public class ResourcesDecoder {
 
         AXmlResourceParser axmlParser = new AXmlResourceParser(mResTable);
         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);
         }
 
         ExtMXSerializer xmlSerializer = getResXmlSerializer();
         for (ResPackage pkg : mResTable.listMainPackages()) {
 
             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();
         if (decodeError != null) {
             throw decodeError;
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
index 9bab7c98..979d89a8 100644
--- 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
@@ -23,10 +23,11 @@ import brut.androlib.res.data.ResResource;
 import brut.androlib.res.data.value.ResBoolValue;
 import brut.androlib.res.data.value.ResFileValue;
 import brut.directory.DirUtil;
 import brut.directory.Directory;
 import brut.directory.DirectoryException;
+import brut.util.BrutIO;
 
 import java.io.*;
 import java.util.Map;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -42,12 +43,19 @@ public class ResFileDecoder {
             throws AndrolibException {
 
         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;
         int extPos = inFileName.lastIndexOf(".");
         if (extPos == -1) {
diff --git a/brut.apktool/apktool-lib/src/test/java/brut/androlib/decode/ResourceDirectoryTraversalTest.java b/brut.apktool/apktool-lib/src/test/java/brut/androlib/decode/ResourceDirectoryTraversalTest.java
new file mode 100644
index 00000000..7d308db5
--- /dev/null
+++ b/brut.apktool/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
index ad22e9d0..269b3fc0 100644
--- 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
@@ -49,31 +49,31 @@ public class UnknownDirectoryTraversalTest extends BaseTest {
         OS.rmdir(sTmpDir);
     }
 
     @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);
         assertTrue(validFile.isFile());
     }
 
     @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)
     public void invalidBackwardPathOnWindows() throws IOException, BrutException {
         String invalidPath;
@@ -81,14 +81,14 @@ public class UnknownDirectoryTraversalTest extends BaseTest {
             invalidPath = "../../app";
         } else {
             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
new file mode 100644
index 00000000..cc177ce3
Binary files /dev/null and b/brut.apktool/apktool-lib/src/test/resources/decode/arbitrary-write/GHSA-2hqv-2xv4-5h5w.apk differ
diff --git a/brut.j.dir/src/main/java/brut/directory/DirUtil.java b/brut.j.dir/src/main/java/brut/directory/DirUtil.java
index bfc1e0a1..906fdac2 100644
--- a/brut.j.dir/src/main/java/brut/directory/DirUtil.java
+++ b/brut.j.dir/src/main/java/brut/directory/DirUtil.java
@@ -87,11 +87,11 @@ public class DirUtil {
                 OS.rmdir(new File(out, fileName));
                 in.getDir(fileName).copyToDir(new File(out, 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
                     outFile.getParentFile().mkdirs();
                     BrutIO.copyAndClose(in.getFileInput(fileName), Files.newOutputStream(outFile.toPath()));
diff --git a/brut.j.dir/src/main/java/brut/directory/FileDirectory.java b/brut.j.dir/src/main/java/brut/directory/FileDirectory.java
index 554900bd..8a9f9915 100644
--- a/brut.j.dir/src/main/java/brut/directory/FileDirectory.java
+++ b/brut.j.dir/src/main/java/brut/directory/FileDirectory.java
@@ -117,9 +117,9 @@ public class FileDirectory extends AbstractDirectory {
                 } catch (DirectoryException ignored) {}
             }
         }
     }
 
-    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
index 7cd81471..db9dcdf3 100644
--- a/brut.j.dir/src/main/java/brut/directory/ZipUtils.java
+++ b/brut.j.dir/src/main/java/brut/directory/ZipUtils.java
@@ -55,12 +55,12 @@ public class ZipUtils {
 
     private static void processFolder(final File folder, final ZipOutputStream zipOutputStream, final int prefixLength)
             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
                 final String extension = FilenameUtils.getExtension(file.getAbsolutePath());
                 if (mDoNotCompress != null && (mDoNotCompress.contains(extension) || mDoNotCompress.contains(zipEntry.getName()))) {
diff --git a/brut.j.util/src/main/java/brut/util/BrutIO.java b/brut.j.util/src/main/java/brut/util/BrutIO.java
index ede9a853..feecc129 100644
--- a/brut.j.util/src/main/java/brut/util/BrutIO.java
+++ b/brut.j.util/src/main/java/brut/util/BrutIO.java
@@ -72,12 +72,12 @@ public class BrutIO {
             crc.update(buffer, 0, bytesRead);
         }
         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");
         }
 
         if (new File(entry).isAbsolute()) {
             throw new RootUnknownFileException("Absolute Unknown Files is not allowed");
@@ -92,11 +92,18 @@ public class BrutIO {
 
         // https://stackoverflow.com/q/2375903/455008
         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 != '/') {
             return path.replace(separator, '/');
         }

Vulnerable code:                 inputFile = new File(unknownFileDir, BrutIO.sanitizeUnknownFile(unknownFileDir, unknownFileInfo.getKey()));
        Directory in, out;
            out = out.createDir("res");
                fileDecoder.decode(res, in, out, mResFileMapping);
                generateValuesFile(valuesFile, out, xmlSerializer);
            generatePublicXml(pkg, out, xmlSerializer);
        String outResName = res.getFilePath();
        String validFilename = BrutIO.sanitizeUnknownFile(sTmpDir, "file");
        BrutIO.sanitizeUnknownFile(sTmpDir, "../file");
        BrutIO.sanitizeUnknownFile(sTmpDir, rootLocation + "file");
        BrutIO.sanitizeUnknownFile(sTmpDir, "");
        BrutIO.sanitizeUnknownFile(sTmpDir, invalidPath);
        String validFilename = BrutIO.sanitizeUnknownFile(sTmpDir, "dir" + File.separator + "file");
                String cleanedFilename = BrutIO.sanitizeUnknownFile(out, fileName);
    private File getDir() {
                final String cleanedPath = BrutIO.sanitizeUnknownFile(folder, file.getPath().substring(prefixLength));
                final ZipEntry zipEntry = new ZipEntry(BrutIO.normalizePath(cleanedPath));
    public static String sanitizeUnknownFile(final File directory, final String entry) throws IOException, BrutException {
        if (entry.length() == 0) {
    public static String normalizePath(String path) {
Fixed code:                 inputFile = new File(unknownFileDir, BrutIO.sanitizeFilepath(unknownFileDir, unknownFileInfo.getKey()));
        Directory in, out, outRes;
            outRes = out.createDir("res");
                fileDecoder.decode(res, in, outRes, mResFileMapping);
                generateValuesFile(valuesFile, outRes, xmlSerializer);
            generatePublicXml(pkg, outRes, xmlSerializer);
import brut.util.BrutIO;
        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
            ));
        }
/*
 *  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());
    }
}
        String validFilename = BrutIO.sanitizeFilepath(sTmpDir, "file");
        BrutIO.sanitizeFilepath(sTmpDir, "../file");
        BrutIO.sanitizeFilepath(sTmpDir, rootLocation + "file");
        BrutIO.sanitizeFilepath(sTmpDir, "");
        BrutIO.sanitizeFilepath(sTmpDir, invalidPath);
        String validFilename = BrutIO.sanitizeFilepath(sTmpDir, "dir" + File.separator + "file");
                String cleanedFilename = BrutIO.sanitizeFilepath(out, fileName);
    public File getDir() {
                final String cleanedPath = BrutIO.sanitizeFilepath(folder, file.getPath().substring(prefixLength));
                final ZipEntry zipEntry = new ZipEntry(BrutIO.adaptSeparatorToUnix(cleanedPath));
    public static String sanitizeFilepath(final File directory, final String entry) throws IOException, BrutException {
        if (entry.isEmpty()) {
    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) {