Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent arbitrary file writes with malicious resource names. #3484

Merged
merged 15 commits into from
Jan 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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)
Expand All @@ -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);
}
}
Binary file not shown.
2 changes: 1 addition & 1 deletion brut.j.dir/src/main/java/brut/directory/DirUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion brut.j.dir/src/main/java/brut/directory/FileDirectory.java
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private void loadAll() {
}
}

private File getDir() {
public File getDir() {
return mDir;
}
}
4 changes: 2 additions & 2 deletions brut.j.dir/src/main/java/brut/directory/ZipUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,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
Expand Down
13 changes: 10 additions & 3 deletions brut.j.util/src/main/java/brut/util/BrutIO.java
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

Expand All @@ -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 != '/') {
Expand Down