[ofbiz-framework] branch release17.12 updated: Fixed: Improve ObjectInputStream class (CVE-2019-0189) Improved: no functional change (OFBIZ-10837) (OFBIZ-11398)

This is an automated email from the ASF dual-hosted git repository.

jleroux pushed a commit to branch release17.12
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git


The following commit(s) were added to refs/heads/release17.12 by this push:
     new d31e137  Fixed: Improve ObjectInputStream class (CVE-2019-0189) Improved: no functional change (OFBIZ-10837) (OFBIZ-11398)
d31e137 is described below

commit d31e13729346aee0767a998a13bacfea726c870e
Author: Jacques Le Roux <[hidden email]>
AuthorDate: Mon Feb 24 11:55:52 2020 +0100

    Fixed: Improve ObjectInputStream class (CVE-2019-0189)
    Improved: no functional change
    (OFBIZ-10837) (OFBIZ-11398)
   
    Steps to generate:
    1. Navigate to - catalog/control/EditProdCatalog?prodCatalogId=TestCatalog
    2. Click on - CREATE SEO CATEGORY/PRODUCTS
    3. The broken page will be displayed
   
    The issue is due to the use of a GString in
    createMissingCategoryAndProductAltUrls().
   
    This:
        result.successMessageList = [
            "Categories updated: ${categoriesUpdated}",
            "Products updated: ${productsUpdated}"
   
    As it's common to use such expressions I have added the necessary
    org.codehaus.groovy.runtime.GStringImpl groovy.lang.GString
    to the white list of classes in listOfSafeObjectsForInputStream in
    SafeObjectInputStream.properties
   
    I finally have also decided to use this property as default and commented for
    committers to be aware that it should be also put in DEFAULT_WHITELIST_PATTERN
    in SafeObjectInputStream class. Because if, for a reason,
    listOfSafeObjectsForInputStream is empty OFBiz will still be protected
   
    Note: cherry picking did not work at all. The change was completely handled by
    hand. Fortunately, it was just about copyin 2 files from trunk
   
    Thanks: Dikpal Kanungo for reporting
---
 .../base/config/SafeObjectInputStream.properties   |  6 +-
 .../ofbiz/base/util/SafeObjectInputStream.java     | 93 +++++++++-------------
 2 files changed, 43 insertions(+), 56 deletions(-)

diff --git a/framework/base/config/SafeObjectInputStream.properties b/framework/base/config/SafeObjectInputStream.properties
index bdc5b4a..548eab7 100644
--- a/framework/base/config/SafeObjectInputStream.properties
+++ b/framework/base/config/SafeObjectInputStream.properties
@@ -21,7 +21,9 @@
 # If you encounter a related issue (object not in the whitelist),
 # you must provide a complete list of objects to pass to ObjectInputStream
 # through ListOfSafeObjectsForInputStream property
-# As an example, the a complete list of objects  used by OFBiz OOTB is commented out by default here.
+# As an example, the a complete list of objects  used by OFBiz OOTB is here.
 # You will need to add your objects/classes to this list.
+# OFBiz committers: don't forget to add newobjects in SafeObjectInputStream class too (as default there).
 
-#listOfSafeObjectsForInputStream=byte\\\\[\\\\], foo, SerializationInjector, \\\\[Z,\\\\[B,\\\\[S,\\\\[I,\\\\[J,\\\\[F,\\\\[D,\\\\[C, java..*, sun.util.calendar..*, org.apache.ofbiz..*
+
+listOfSafeObjectsForInputStream=byte\\\\[\\\\], foo, SerializationInjector, \\\\[Z,\\\\[B,\\\\[S,\\\\[I,\\\\[J,\\\\[F,\\\\[D,\\\\[C, java..*, sun.util.calendar..*, org.apache.ofbiz..*, org.codehaus.groovy.runtime.GStringImpl, groovy.lang.GString
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/SafeObjectInputStream.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/SafeObjectInputStream.java
index c48699a..2aebcde 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/util/SafeObjectInputStream.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/SafeObjectInputStream.java
@@ -1,4 +1,4 @@
-/*******************************************************************************
+/*
  * Licensed to the Apache Software Foundation (ASF) under one
  * or more contributor license agreements.  See the NOTICE file
  * distributed with this work for additional information
@@ -15,77 +15,62 @@
  * KIND, either express or implied.  See the License for the
  * specific language governing permissions and limitations
  * under the License.
- *******************************************************************************/
+ */
 package org.apache.ofbiz.base.util;
 
+import static java.util.stream.Collectors.collectingAndThen;
+import static java.util.stream.Collectors.joining;
+import static org.apache.ofbiz.base.util.UtilProperties.getPropertyValue;
+
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.ObjectInputStream;
 import java.io.ObjectStreamClass;
-import java.lang.reflect.Proxy;
-import java.util.List;
+import java.util.Arrays;
 import java.util.regex.Pattern;
 
 /**
- * ObjectInputStream
+ * SafeObjectInputStream
  *
+ * <p> An implementation of {@link java.io.ObjectInputStream} that ensure that
+ * only authorized class can be read from it.
  */
-public class SafeObjectInputStream extends java.io.ObjectInputStream {
-
-    private ClassLoader classloader;
-    private Pattern WHITELIST_PATTERN = null;
+public final class SafeObjectInputStream extends ObjectInputStream {
+    private static final String[] DEFAULT_WHITELIST_PATTERN = {
+            "byte\\[\\]", "foo", "SerializationInjector",
+            "\\[Z", "\\[B", "\\[S", "\\[I", "\\[J", "\\[F", "\\[D", "\\[C",
+            "java..*", "sun.util.calendar..*", "org.apache.ofbiz..*",
+            "org.codehaus.groovy.runtime.GStringImpl", "groovy.lang.GString"};
 
-    public SafeObjectInputStream(InputStream in, ClassLoader loader) throws IOException {
-        super(in);
-        this.classloader = loader;
-    }
+    /** The regular expression used to match serialized types. */
+    private final Pattern whitelistPattern;
 
-    public SafeObjectInputStream(InputStream in, ClassLoader loader, List<String> whitelist) throws IOException {
+    /**
+     * Instantiates a safe object input stream.
+     *
+     * @param in  the input stream to read
+     * @throws IOException when reading is not possible.
+     */
+    public SafeObjectInputStream(InputStream in) throws IOException {
         super(in);
-        this.classloader = loader;
-        StringBuilder bld = new StringBuilder("(");
-        for (int i = 0; i < whitelist.size(); i++) {
-            bld.append(whitelist.get(i));
-            if (i != whitelist.size() - 1) {
-                bld.append("|");
-            }
-        }
-        bld.append(")");
-        WHITELIST_PATTERN = Pattern.compile(bld.toString());
+        String safeObjectsProp = getPropertyValue("SafeObjectInputStream", "ListOfSafeObjectsForInputStream", "");
+        String[] whitelist = safeObjectsProp.isEmpty() ? DEFAULT_WHITELIST_PATTERN : safeObjectsProp.split(",");
+        whitelistPattern = Arrays.stream(whitelist)
+                .map(String::trim)
+                .filter(str -> !str.isEmpty())
+                .collect(collectingAndThen(joining("|", "(", ")"), Pattern::compile));
     }
 
-
-    /**
-     * @see java.io.ObjectInputStream#resolveClass(java.io.ObjectStreamClass)
-     */
     @Override
     protected Class<?> resolveClass(ObjectStreamClass classDesc) throws IOException, ClassNotFoundException {
-        if (!WHITELIST_PATTERN.matcher(classDesc.getName()).find()) {
-            Debug.logWarning("***Incompatible class***: " + classDesc.getName() +
-                    ". Please see OFBIZ-10837.  Report to dev ML if you use OFBiz without changes. "
-                    + "Else follow https://s.apache.org/45war"
-                    , "SafeObjectInputStream");
+        if (!whitelistPattern.matcher(classDesc.getName()).find()) {
+            Debug.logWarning("***Incompatible class***: "
+                    + classDesc.getName()
+                    + ". Please see OFBIZ-10837.  Report to dev ML if you use OFBiz without changes. "
+                    + "Else follow https://s.apache.org/45war",
+                    "SafeObjectInputStream");
             throw new ClassCastException("Incompatible class: " + classDesc.getName());
         }
-        
-        return ObjectType.loadClass(classDesc.getName(), classloader);
-    }
-
-    /**
-     * @see java.io.ObjectInputStream#resolveProxyClass(java.lang.String[])
-     */
-    @Override
-    protected Class<?> resolveProxyClass(String[] interfaces) throws IOException, ClassNotFoundException {
-        Class<?>[] cinterfaces = new Class<?>[interfaces.length];
-        for (int i = 0; i < interfaces.length; i++) {
-            cinterfaces[i] = classloader.loadClass(interfaces[i]);
-        }
-        //Proxy.getInvocationHandler(proxy)
-        
-        try {
-            return Proxy.getProxyClass(classloader, cinterfaces);
-        } catch (IllegalArgumentException e) {
-            throw new ClassNotFoundException(null, e);
-        }
-
+        return ObjectType.loadClass(classDesc.getName(), Thread.currentThread().getContextClassLoader());
     }
 }