Skip to content

Commit 3cebe29

Browse files
CopilotiBotPeaches
authored andcommitted
Harden OS.rmdir() to not follow symbolic links during deletion
Previously, OS.rmdir() would treat symlinks to directories as real directories and recurse into them, potentially deleting files outside the intended path. This is a security vulnerability when using `apktool d --force` on a symlinked output directory. Fix: Use Files.isSymbolicLink() to detect symlinks before recursing. Symlinks are now deleted directly (like regular files) without following them into their targets. Closes #4183
1 parent 4433d41 commit 3cebe29

3 files changed

Lines changed: 97 additions & 1 deletion

File tree

brut.j.util/build.gradle.kts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,6 @@ dependencies {
22
implementation(project(":brut.j.common"))
33
implementation(libs.commons.io)
44
implementation(libs.guava)
5+
6+
testImplementation(libs.junit)
57
}

brut.j.util/src/main/java/brut/util/OS.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ public static void rmdir(String dir) {
6161
}
6262

6363
public static void rmdir(File dir) {
64+
if (Files.isSymbolicLink(dir.toPath())) {
65+
rmfile(dir);
66+
return;
67+
}
6468
if (!dir.isDirectory()) {
6569
return;
6670
}
@@ -71,7 +75,7 @@ public static void rmdir(File dir) {
7175
}
7276

7377
for (File file : files) {
74-
if (file.isDirectory()) {
78+
if (!Files.isSymbolicLink(file.toPath()) && file.isDirectory()) {
7579
rmdir(file);
7680
} else {
7781
rmfile(file);
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/*
2+
* Copyright (C) 2010 Ryszard Wiśniewski <brut.alll@gmail.com>
3+
* Copyright (C) 2010 Connor Tumbleson <connor.tumbleson@gmail.com>
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* https://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package brut.util;
18+
19+
import org.junit.Test;
20+
21+
import java.io.File;
22+
import java.nio.file.Files;
23+
import java.nio.file.Path;
24+
25+
import static org.junit.Assert.*;
26+
27+
public class OSTest {
28+
29+
@Test
30+
public void testRmdirDoesNotFollowSymlinkToDirectory() throws Exception {
31+
// Create a real target directory with a file in it
32+
Path targetDir = Files.createTempDirectory("apktool-test-target");
33+
Path fileInTarget = targetDir.resolve("sensitive-file.txt");
34+
Files.write(fileInTarget, "sensitive content".getBytes());
35+
36+
// Create a symlink pointing to the target directory
37+
Path symlinkDir = Files.createTempDirectory("apktool-test-base");
38+
Files.delete(symlinkDir);
39+
Files.createSymbolicLink(symlinkDir, targetDir);
40+
41+
assertTrue("Symlink should exist before rmdir", Files.exists(symlinkDir));
42+
assertTrue("Target file should exist before rmdir", Files.exists(fileInTarget));
43+
44+
// Call rmdir on the symlink
45+
OS.rmdir(symlinkDir.toFile());
46+
47+
// The symlink should be removed
48+
assertFalse("Symlink should be removed by rmdir", Files.exists(symlinkDir));
49+
assertFalse("Symlink itself should not exist (not following)", Files.isSymbolicLink(symlinkDir));
50+
51+
// The real target directory and its contents should NOT be deleted
52+
assertTrue("Target directory should still exist", Files.exists(targetDir));
53+
assertTrue("File inside target directory should still exist", Files.exists(fileInTarget));
54+
55+
// Cleanup
56+
Files.delete(fileInTarget);
57+
Files.delete(targetDir);
58+
}
59+
60+
@Test
61+
public void testRmdirDoesNotFollowSymlinkInsideDirectory() throws Exception {
62+
// Create a real target directory with a file in it
63+
Path targetDir = Files.createTempDirectory("apktool-test-target");
64+
Path fileInTarget = targetDir.resolve("sensitive-file.txt");
65+
Files.write(fileInTarget, "sensitive content".getBytes());
66+
67+
// Create a directory that contains a symlink pointing to the target
68+
Path baseDir = Files.createTempDirectory("apktool-test-base");
69+
Path symlinkInDir = baseDir.resolve("symlink-to-target");
70+
Files.createSymbolicLink(symlinkInDir, targetDir);
71+
72+
assertTrue("Base dir should exist before rmdir", Files.exists(baseDir));
73+
assertTrue("Symlink inside dir should exist before rmdir", Files.exists(symlinkInDir));
74+
assertTrue("Target file should exist before rmdir", Files.exists(fileInTarget));
75+
76+
// Call rmdir on the base directory (which contains the symlink)
77+
OS.rmdir(baseDir.toFile());
78+
79+
// The base directory and symlink should be removed
80+
assertFalse("Base dir should be removed by rmdir", Files.exists(baseDir));
81+
82+
// The real target directory and its contents should NOT be deleted
83+
assertTrue("Target directory should still exist", Files.exists(targetDir));
84+
assertTrue("File inside target directory should still exist", Files.exists(fileInTarget));
85+
86+
// Cleanup
87+
Files.delete(fileInTarget);
88+
Files.delete(targetDir);
89+
}
90+
}

0 commit comments

Comments
 (0)