Skip to content

Commit a131cfa

Browse files
tomas-sexeniantomas-sexenian
committed
Fix wrong directory traversal and zip bomb checks
1 parent 457efed commit a131cfa

File tree

2 files changed

+123
-7
lines changed

2 files changed

+123
-7
lines changed

‎gxcompress/src/main/java/com/genexus/compression/CompressionUtils.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import org.apache.commons.compress.archivers.sevenz.SevenZFile;
55
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
66
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
7+
import org.apache.commons.compress.compressors.gzip.GzipCompressorInputStream;
78

89
import java.io.File;
910
import java.io.FileInputStream;
@@ -16,6 +17,7 @@
1617
import static org.apache.commons.io.FilenameUtils.getExtension;
1718

1819
public class CompressionUtils {
20+
1921
public static int countArchiveEntries(File toCompress) throws IOException {
2022
String ext = getExtension(toCompress.getName()).toLowerCase();
2123
switch (ext) {
@@ -101,6 +103,70 @@ public static boolean isArchiveSafe(File toCompress, String targetDir) throws IO
101103
return true;
102104
}
103105

106+
private static final long MAX_PERMITTED_SIZE = 1024L * 1024 * 1024 * 15; //15GB
107+
108+
public static long estimateDecompressedSize(File archive) throws IOException {
109+
String ext = getExtension(archive.getName()).toLowerCase();
110+
long totalSize = 0;
111+
112+
switch (ext) {
113+
case "zip":
114+
case "jar":
115+
try (ZipFile zip = new ZipFile(archive)) {
116+
Enumeration<? extends ZipEntry> entries = zip.entries();
117+
while (entries.hasMoreElements()) {
118+
ZipEntry entry = entries.nextElement();
119+
if (!entry.isDirectory()) {
120+
long size = entry.getSize();
121+
totalSize += (size > 0) ? size : (archive.length() * 5);
122+
if (totalSize > MAX_PERMITTED_SIZE) {
123+
return totalSize;
124+
}
125+
}
126+
}
127+
}
128+
break;
129+
case "tar":
130+
try (FileInputStream fis = new FileInputStream(archive);
131+
TarArchiveInputStream tais = new TarArchiveInputStream(fis)) {
132+
TarArchiveEntry entry;
133+
while ((entry = tais.getNextEntry()) != null) {
134+
if (!entry.isDirectory()) {
135+
totalSize += entry.getSize();
136+
if (totalSize > MAX_PERMITTED_SIZE) {
137+
return totalSize;
138+
}
139+
}
140+
}
141+
}
142+
break;
143+
case "7z":
144+
try (SevenZFile sevenZFile = getSevenZFile(archive.getAbsolutePath())) {
145+
SevenZArchiveEntry entry;
146+
while ((entry = sevenZFile.getNextEntry()) != null) {
147+
if (!entry.isDirectory()) {
148+
totalSize += entry.getSize();
149+
if (totalSize > MAX_PERMITTED_SIZE) {
150+
return totalSize;
151+
}
152+
}
153+
}
154+
}
155+
break;
156+
case "gz":
157+
try (FileInputStream fis = new FileInputStream(archive);
158+
GzipCompressorInputStream ignored = new GzipCompressorInputStream(fis)) {
159+
totalSize = archive.length() * 10;
160+
}
161+
break;
162+
}
163+
if (totalSize < archive.length()) {
164+
totalSize = archive.length() * 5;
165+
}
166+
167+
return totalSize;
168+
}
169+
104170
private static SevenZFile getSevenZFile(final String specialPath) throws IOException {
105171
return SevenZFile.builder().setFile(getFile(specialPath)).get();
106172
}

‎gxcompress/src/main/java/com/genexus/compression/GXCompressor.java‎

Lines changed: 57 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,16 @@ public class GXCompressor implements IGXCompressor {
3434
private static final String FILE_NOT_EXISTS = "File does not exist: ";
3535
private static final String UNSUPPORTED_FORMAT = " is an unsupported format. Supported formats are zip, 7z, tar, gz and jar.";
3636
private static final String EMPTY_FILE = "The selected file is empty: ";
37+
private static final String PURGED_ARCHIVE = "After performing security checks, no valid files where left to compress";
3738
private static final String DIRECTORY_ATTACK = "Potential directory traversal attack detected: ";
3839
private static final String MAX_FILESIZE_EXCEEDED = "The file(s) selected for (de)compression exceed the maximum permitted file size of ";
3940
private static final String TOO_MANY_FILES = "Too many files have been added for (de)compression. Maximum allowed is ";
4041
private static final String ZIP_SLIP_DETECTED = "Zip slip or path traversal attack detected in archive: ";
42+
private static final String BIG_SINGLE_FILE = "Individual file exceeds maximum allowed size: ";
43+
private static final String PROCESSING_ERROR = "Error checking archive safety for file: ";
44+
private static final String ARCHIVE_SIZE_ESTIMATION_ERROR = "";
4145
private static final int MAX_FILES_ALLOWED = 1000;
46+
private static final long MAX_DECOMPRESSED_SIZE = 1024 * 1024 * 100; // 100MB limit
4247

4348
private static void storageMessages(String error, GXBaseCollection<SdtMessages_Message> messages) {
4449
try {
@@ -51,7 +56,7 @@ private static void storageMessages(String error, GXBaseCollection<SdtMessages_M
5156
log.error("Failed to store the following error message: {}", error, e);
5257
}
5358
}
54-
59+
5560
public static Boolean compress(ArrayList<String> files, String path, long maxCombinedFileSize, GXBaseCollection<SdtMessages_Message>[] messages) {
5661
if (files.isEmpty()){
5762
log.error(NO_FILES_ADDED);
@@ -76,25 +81,52 @@ public static Boolean compress(ArrayList<String> files, String path, long maxCom
7681
storageMessages(FILE_NOT_EXISTS + filePath, messages[0]);
7782
continue;
7883
}
79-
if (!normalizedPath.equals(file.getAbsolutePath())) {
84+
85+
// More permissive directory traversal check
86+
// Check if the canonical path points to a parent directory of the file
87+
// or if it points to a completely different location
88+
File absFile = file.getAbsoluteFile();
89+
if (!normalizedPath.startsWith(absFile.getParentFile().getCanonicalPath())) {
8090
log.error(DIRECTORY_ATTACK + "{}", filePath);
8191
storageMessages(DIRECTORY_ATTACK + filePath, messages[0]);
8292
return false;
8393
}
94+
8495
long fileSize = file.length();
96+
97+
if (maxCombinedFileSize > -1 && fileSize > maxCombinedFileSize) {
98+
log.error(BIG_SINGLE_FILE + filePath);
99+
storageMessages(BIG_SINGLE_FILE + filePath, messages[0]);
100+
files.clear();
101+
return false;
102+
}
103+
85104
totalSize += fileSize;
86105
if (maxCombinedFileSize > -1 && totalSize > maxCombinedFileSize) {
87106
log.error(MAX_FILESIZE_EXCEEDED + maxCombinedFileSize);
88107
storageMessages(MAX_FILESIZE_EXCEEDED + maxCombinedFileSize, messages[0]);
89-
toCompress = null;
90108
files.clear();
91109
return false;
92110
}
93111
toCompress[index++] = file;
94112
} catch (IOException e) {
95113
log.error("Error normalizing path for file: {}", filePath, e);
114+
return false;
96115
}
97116
}
117+
118+
if (index < toCompress.length) {
119+
File[] resized = new File[index];
120+
System.arraycopy(toCompress, 0, resized, 0, index);
121+
toCompress = resized;
122+
}
123+
124+
if (toCompress.length == 0) {
125+
log.error(PURGED_ARCHIVE);
126+
storageMessages(PURGED_ARCHIVE, messages[0]);
127+
return false;
128+
}
129+
98130
String format = CommonUtil.getFileType(path).toLowerCase();
99131
try {
100132
switch (format.toLowerCase()) {
@@ -150,8 +182,8 @@ public static Boolean decompress(String file, String path, GXBaseCollection<SdtM
150182
return false;
151183
}
152184
} catch (Exception e) {
153-
log.error("Error counting archive entries for file: {}", file, e);
154-
storageMessages("Error counting archive entries for file: " + file, messages[0]);
185+
log.error(PROCESSING_ERROR + file, e);
186+
storageMessages(PROCESSING_ERROR + file, messages[0]);
155187
return false;
156188
}
157189
try {
@@ -161,10 +193,28 @@ public static Boolean decompress(String file, String path, GXBaseCollection<SdtM
161193
return false;
162194
}
163195
} catch (Exception e) {
164-
log.error("Error checking archive safety for file: {}", file, e);
165-
storageMessages("Error checking archive safety for file: " + file, messages[0]);
196+
log.error(PROCESSING_ERROR+ file, e);
197+
storageMessages(PROCESSING_ERROR + file, messages[0]);
166198
return false;
167199
}
200+
201+
try {
202+
long totalSizeEstimate = CompressionUtils.estimateDecompressedSize(toCompress);
203+
204+
if (totalSizeEstimate > MAX_DECOMPRESSED_SIZE) {
205+
log.error("Potential zip bomb detected. Estimated size: " + totalSizeEstimate + " bytes");
206+
storageMessages("Potential zip bomb detected. Operation aborted for security reasons.", messages[0]);
207+
if (!toCompress.delete()) {
208+
log.error("Failed to delete suspicious archive: {}", file);
209+
}
210+
return false;
211+
}
212+
} catch (Exception e) {
213+
log.error(ARCHIVE_SIZE_ESTIMATION_ERROR + file, e);
214+
storageMessages(ARCHIVE_SIZE_ESTIMATION_ERROR+ file, messages[0]);
215+
return false;
216+
}
217+
168218
String extension = getExtension(toCompress.getName());
169219
try {
170220
switch (extension.toLowerCase()) {

0 commit comments

Comments
 (0)