Skip to content

Commit d1bfb92

Browse files
jacopocjacopoc
committed
Improved: Explicitly reject JPEG and GIF uploads with trailing bytes
The imageMadeSafe() re-encoding pipeline already strips trailing bytes for all formats, but JPEG and GIF uploads containing a webshell payload appended after their respective terminators were silently sanitized rather than explicitly rejected. Added noWebshellInJPEG() and noWebshellInGIF(), called from imageMadeSafe() alongside the existing noWebshellInPNG() check. noWebshellInJPEG() walks the JPEG marker structure from SOI, handling entropy-coded scan data with byte stuffing, and rejects the file if any bytes follow the EOI (FF D9) marker. noWebshellInGIF() parses the GIF block structure (extensions, image descriptors, and their sub-block chains) and rejects the file if any bytes follow the Trailer (0x3B). (cherry picked from commit e1fc556)
1 parent 53b47a5 commit d1bfb92

1 file changed

Lines changed: 158 additions & 0 deletions

File tree

‎framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,6 +568,12 @@ private static boolean imageMadeSafe(String fileName) {
568568
if (!noWebshellInPNG(file)) {
569569
return false;
570570
}
571+
if (!noWebshellInJPEG(file)) {
572+
return false;
573+
}
574+
if (!noWebshellInGIF(file)) {
575+
return false;
576+
}
571577

572578
boolean safeState = false;
573579

@@ -738,6 +744,158 @@ private static boolean noWebshellInPNG(File file) {
738744
}
739745
}
740746

747+
private static boolean noWebshellInJPEG(File file) {
748+
try {
749+
byte[] bytes = Files.readAllBytes(file.toPath());
750+
if (!Imaging.guessFormat(bytes).equals(ImageFormats.JPEG)) {
751+
return true; // Not a JPEG file, it's OK so far
752+
}
753+
// SOI marker check
754+
if (bytes.length < 4 || (bytes[0] & 0xFF) != 0xFF || (bytes[1] & 0xFF) != 0xD8) {
755+
Debug.logError("================== Not saved for security reason, malformed JPEG ==================", MODULE);
756+
return false;
757+
}
758+
int pos = 2;
759+
while (pos < bytes.length) {
760+
if ((bytes[pos] & 0xFF) != 0xFF) {
761+
Debug.logError("================== Not saved for security reason, malformed JPEG marker ==================", MODULE);
762+
return false;
763+
}
764+
// Skip 0xFF fill bytes (valid marker padding per JPEG spec)
765+
while (pos < bytes.length && (bytes[pos] & 0xFF) == 0xFF) {
766+
pos++;
767+
}
768+
if (pos >= bytes.length) {
769+
Debug.logError("================== Not saved for security reason, JPEG missing EOI ==================", MODULE);
770+
return false;
771+
}
772+
int marker = bytes[pos++] & 0xFF;
773+
if (marker == 0xD9) {
774+
// EOI — reject any trailing bytes
775+
if (pos != bytes.length) {
776+
Debug.logError("================ Not saved for security reason, JPEG has trailing bytes after EOI ================", MODULE);
777+
return false;
778+
}
779+
return true;
780+
} else if (marker >= 0xD0 && marker <= 0xD8) {
781+
// SOI (0xD8) and RST0–RST7 (0xD0–0xD7) — no length field
782+
continue;
783+
} else if (marker == 0xDA) {
784+
// SOS: length-prefixed header followed by entropy-coded scan data
785+
if (pos + 2 > bytes.length) return false;
786+
int len = ((bytes[pos] & 0xFF) << 8) | (bytes[pos + 1] & 0xFF);
787+
if (len < 2 || pos + len > bytes.length) return false;
788+
pos += len; // Skip SOS header
789+
// Scan entropy-coded data, respecting byte stuffing (FF 00) and restart markers
790+
while (pos < bytes.length - 1) {
791+
if ((bytes[pos] & 0xFF) == 0xFF) {
792+
int next = bytes[pos + 1] & 0xFF;
793+
if (next == 0x00 || (next >= 0xD0 && next <= 0xD7)) {
794+
pos += 2; // Stuffed 0xFF or RST — part of scan data
795+
} else {
796+
break; // Real marker — stop scanning scan data
797+
}
798+
} else {
799+
pos++;
800+
}
801+
}
802+
} else {
803+
// Regular length-delimited segment
804+
if (pos + 2 > bytes.length) return false;
805+
int len = ((bytes[pos] & 0xFF) << 8) | (bytes[pos + 1] & 0xFF);
806+
if (len < 2 || pos + len > bytes.length) return false;
807+
pos += len;
808+
}
809+
}
810+
Debug.logError("================== Not saved for security reason, JPEG missing EOI ==================", MODULE);
811+
return false;
812+
} catch (IOException error) {
813+
Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
814+
return false;
815+
}
816+
}
817+
818+
private static boolean noWebshellInGIF(File file) {
819+
try {
820+
byte[] bytes = Files.readAllBytes(file.toPath());
821+
if (!Imaging.guessFormat(bytes).equals(ImageFormats.GIF)) {
822+
return true; // Not a GIF file, it's OK so far
823+
}
824+
// Header: "GIF87a" or "GIF89a"
825+
if (bytes.length < 13) return false;
826+
String gifHeader = new String(bytes, 0, 6, StandardCharsets.US_ASCII);
827+
if (!"GIF87a".equals(gifHeader) && !"GIF89a".equals(gifHeader)) {
828+
Debug.logError("================== Not saved for security reason, malformed GIF ==================", MODULE);
829+
return false;
830+
}
831+
int pos = 6;
832+
// Logical Screen Descriptor: packed byte at offset 4 within LSD
833+
int packed = bytes[pos + 4] & 0xFF;
834+
boolean hasGCT = (packed & 0x80) != 0;
835+
int gctSize = packed & 0x07;
836+
pos += 7;
837+
// Skip Global Color Table
838+
if (hasGCT) {
839+
int gctBytes = 3 * (1 << (gctSize + 1));
840+
if (pos + gctBytes > bytes.length) return false;
841+
pos += gctBytes;
842+
}
843+
// Parse blocks until Trailer
844+
while (pos < bytes.length) {
845+
int blockType = bytes[pos++] & 0xFF;
846+
if (blockType == 0x3B) {
847+
// Trailer — reject any trailing bytes
848+
if (pos != bytes.length) {
849+
Debug.logError("=============== Not saved for security reason, GIF has trailing bytes after Trailer ===============", MODULE);
850+
return false;
851+
}
852+
return true;
853+
} else if (blockType == 0x21) {
854+
// Extension: label byte + sub-blocks
855+
if (pos >= bytes.length) return false;
856+
pos++; // Skip extension label
857+
pos = skipGIFSubBlocks(bytes, pos);
858+
if (pos < 0) return false;
859+
} else if (blockType == 0x2C) {
860+
// Image Descriptor: 9 bytes
861+
if (pos + 9 > bytes.length) return false;
862+
int imagePacked = bytes[pos + 8] & 0xFF;
863+
boolean hasLCT = (imagePacked & 0x80) != 0;
864+
int lctSize = imagePacked & 0x07;
865+
pos += 9;
866+
if (hasLCT) {
867+
int lctBytes = 3 * (1 << (lctSize + 1));
868+
if (pos + lctBytes > bytes.length) return false;
869+
pos += lctBytes;
870+
}
871+
pos++; // LZW minimum code size
872+
pos = skipGIFSubBlocks(bytes, pos);
873+
if (pos < 0) return false;
874+
} else {
875+
Debug.logError("================== Not saved for security reason, unknown GIF block type ==================", MODULE);
876+
return false;
877+
}
878+
}
879+
Debug.logError("================== Not saved for security reason, GIF missing Trailer ==================", MODULE);
880+
return false;
881+
} catch (IOException error) {
882+
Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
883+
return false;
884+
}
885+
}
886+
887+
private static int skipGIFSubBlocks(byte[] bytes, int pos) {
888+
while (pos < bytes.length) {
889+
int blockSize = bytes[pos++] & 0xFF;
890+
if (blockSize == 0) {
891+
return pos; // Block Terminator
892+
}
893+
pos += blockSize;
894+
if (pos > bytes.length) return -1;
895+
}
896+
return -1; // Reached EOF without Block Terminator
897+
}
898+
741899
private static boolean isPNG(File file) throws IOException {
742900
Path filePath = Paths.get(file.getPath());
743901
byte[] bytesFromFile = Files.readAllBytes(filePath);

0 commit comments

Comments
 (0)