Improved: Improve ImageManagementServices code (OFBIZ-13292)

Better handling of PNG files

Author: JacquesLeRoux

dependencies.gradle

@@ -17,6 +17,7 @@
  * under the License.
  */
 dependencies {
+    implementation 'com.drewnoakes:metadata-extractor:2.19.0'
     implementation 'com.github.ben-manes.caffeine:caffeine:3.1.8'
     implementation 'com.google.zxing:core:3.5.3'
     implementation 'com.googlecode.concurrentlinkedhashmap:concurrentlinkedhashmap-lru:1.4.2'

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

@@ -23,6 +23,7 @@
 import java.awt.Image;
 import java.awt.Transparency;
 import java.awt.image.BufferedImage;
+import java.io.DataInputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
@@ -39,13 +40,18 @@
 import java.nio.file.Paths;
 import java.nio.file.StandardOpenOption;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Base64;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
 import java.util.UUID;
+import java.util.zip.DataFormatException;
+import java.util.zip.Inflater;
 
 import javax.imageio.ImageIO;
 import javax.imageio.ImageReader;
@@ -96,6 +102,10 @@
 import org.w3c.dom.Document;
 import org.xml.sax.SAXException;
 
+import com.drew.imaging.ImageMetadataReader;
+import com.drew.imaging.ImageProcessingException;
+import com.drew.metadata.Directory;
+import com.drew.metadata.Tag;
 import com.lowagie.text.pdf.PdfReader;
 
 public class SecuredUpload {
@@ -244,7 +254,7 @@ public static boolean isValidFileName(String fileToCheck, Delegator delegator) t
      * @throws ImageReadException
      */
     public static boolean isValidFile(String fileToCheck, String fileType, Delegator delegator) throws IOException, ImageReadException {
-        // Allow all
+        // Allow all uploads w/o check
         if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) {
             return true;
         }
@@ -387,9 +397,17 @@ && imageMadeSafe(fileName)
      */
     private static boolean imageMadeSafe(String fileName) {
         File file = new File(fileName);
-        boolean safeState = false;
         boolean fallbackOnApacheCommonsImaging;
 
+        if (!noWebshellInMetadata(file)) {
+            return false;
+        }
+        if (!noWebshellInPNG(file)) {
+            return false;
+        }
+
+        boolean safeState = false;
+
         if ((file != null) && file.exists() && file.canRead() && file.canWrite()) {
             try (OutputStream fos = Files.newOutputStream(file.toPath(), StandardOpenOption.WRITE)) {
                 // Get the image format
@@ -475,6 +493,114 @@ private static boolean imageMadeSafe(String fileName) {
         return safeState;
     }
 
+    private static boolean noWebshellInMetadata(File file) {
+        com.drew.metadata.Metadata metadata = null;
+        try {
+            metadata = ImageMetadataReader.readMetadata(file);
+        } catch (ImageProcessingException | IOException error) {
+            Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
+        }
+
+        for (Directory directory : metadata.getDirectories()) {
+            for (Tag tag : directory.getTags()) {
+                try {
+                    if (!isValidText(tag.toString(), Collections.emptyList())) {
+                        Debug.logError("================== Not saved for security reason ==================", MODULE);
+                        return false;
+                    }
+                } catch (IOException error) {
+                    Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
+                    return false;
+                }
+            }
+            for (String error : directory.getErrors()) {
+                Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
+                return false;
+            }
+        }
+        return true;
+    }
+
+    private static boolean noWebshellInPNG(File file) {
+        try {
+            ImageIO.read(file);
+            if (!isPNG(file)) {
+                return true; // Not a PNG file, it's OK so far
+            }
+        } catch (IOException error) {
+            Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
+            return false;
+        }
+
+        try (DataInputStream stream = new DataInputStream(new FileInputStream(file));) {
+            byte[] data = new byte[8];
+            stream.readFully(data); //Read PNG Header
+            while (true) {
+                data = new byte[4];
+                stream.readFully(data); //Read Length
+                int length = ((data[0] & 0xFF) << 24)
+                        | ((data[1] & 0xFF) << 16)
+                        | ((data[2] & 0xFF) << 8)
+                        | (data[3] & 0xFF); //Byte array to int
+                stream.readFully(data); //Read Name
+                String name = new String(data); //Byte array to String
+                if (name.equals("IDAT")) {
+                    data = new byte[length];
+                    stream.readFully(data); //Read Data
+                    return inflate(data);
+                } else { //Don't care about other chunks
+                    data = new byte[length + 4]; //Data length + 4 byte CRC
+                    stream.readFully(data); //Skip Data and CRC.
+                }
+            }
+        } catch (IOException error) {
+            Debug.logError("================== Not saved for security reason, wrong PNG IDAT (weird) ==================" + error, MODULE);
+            return false;
+        }
+    }
+
+    private static boolean isPNG(File file) throws IOException {
+        Path filePath = Paths.get(file.getPath());
+        byte[] bytesFromFile = Files.readAllBytes(filePath);
+        ImageFormat imageFormat = Imaging.guessFormat(bytesFromFile);
+        return (imageFormat.equals(ImageFormats.PNG));
+    }
+
+    private static boolean inflate(byte[] data) {
+        Inflater inflater = new Inflater();
+        inflater.setInput(data);
+        byte[] result = new byte[data.length * 5]; // Inflating ratio max is 5/1
+        try {
+            while (!inflater.finished()) {
+                int count = inflater.inflate(result);
+                if (count == 0) {
+                    if (!inflater.needsInput()) { // Not everything read
+                        inflater.inflate(result);
+                    } else if (inflater.needsDictionary()) { // Dictionary to be loaded
+                        inflater.setDictionary(result);
+                        inflater.getAdler();
+                    }
+                }
+            }
+            if (inflater.getRemaining() > 0) { // There is more than image data in IDAT, check it
+                byte[] remaining = Arrays.copyOfRange(data, (int) inflater.getBytesRead(), (int) inflater.getBytesRead() + inflater.getRemaining());
+                String toCheck = new String(remaining, "UTF-8");
+                byte[] decoded = Base64.getDecoder().decode(toCheck);
+                String decodedStr = new String(decoded, StandardCharsets.UTF_8);
+                if (!isValidText(decodedStr, Collections.emptyList())) {
+                    Debug.logError("================== Not saved for security reason ==================", MODULE);
+                    inflater.end();
+                    return false;
+                }
+            }
+        } catch (DataFormatException | IOException error) {
+            Debug.logError("================== Not saved for security reason ==================" + error, MODULE);
+            inflater.end();
+            return false;
+        }
+        return true;
+    }
+
     /**
      * Is it a supported image format, including SVG?
      * @param fileName
@@ -826,8 +952,8 @@ private static boolean isValidVideoFile(String fileName) throws IOException {
     /**
      * Does this text file contains a Freemarker Server-Side Template Injection (SSTI) using freemarker.template.utility.Execute? Etc.
      * @param fileName must be an UTF-8 encoded text file
-     * @param encodedContent TODO
-     * @return true if the text file does not contains a Freemarker SSTI
+     * @param encodedContent true id the file content is encoded
+     * @return true if the text file does not contains a Freemarker SSTI or other issues
      * @throws IOException
      */
     private static boolean isValidTextFile(String fileName, Boolean encodedContent) throws IOException {