Skip to content

Commit f3cf6a1

Browse files
jacopocjacopoc
committed
Fixed: Prevent deserialization of untrusted data in JMS listener
- Restrict XStream allowlist in UtilXml to the exact types handled by XmlSerializer.serializeSingle - Replace the broad java..* and org.apache.ofbiz..* wildcards in SafeObjectInputStream with an explicit minimal allowlist of the same safe types - Remove cus-obj custom Java deserialization support from XmlSerializer (both serialize and deserialize paths) as it allowed arbitrary Serializable objects over JMS - Move the isExport() authorization check before XmlSerializer.deserialize in AbstractJmsListener.runService so gadget chains cannot fire for non-exported or unknown services
1 parent 81f802f commit f3cf6a1

5 files changed

Lines changed: 65 additions & 48 deletions

File tree

‎framework/base/src/main/java/org/apache/ofbiz/base/util/SafeObjectInputStream.java‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,35 @@
3838
*/
3939
public final class SafeObjectInputStream extends ObjectInputStream {
4040
private static final String[] DEFAULT_ALLOWLIST_PATTERN = {
41-
"byte\\[\\]", "foo", "SerializationInjector",
41+
"byte\\[\\]",
4242
"\\[Z", "\\[B", "\\[S", "\\[I", "\\[J", "\\[F", "\\[D", "\\[C",
43-
"java..*", "sun.util.calendar..*", "org.apache.ofbiz..*",
44-
"org.codehaus.groovy.runtime.GStringImpl", "groovy.lang.GString"};
43+
"java\\.lang\\.String",
44+
"java\\.lang\\.Number",
45+
"java\\.lang\\.Integer",
46+
"java\\.lang\\.Long",
47+
"java\\.lang\\.Float",
48+
"java\\.lang\\.Double",
49+
"java\\.lang\\.Boolean",
50+
"java\\.util\\.Locale",
51+
"java\\.math\\.BigDecimal",
52+
"java\\.sql\\.Timestamp",
53+
"java\\.sql\\.Date",
54+
"java\\.sql\\.Time",
55+
"java\\.util\\.Date",
56+
"java\\.util\\.ArrayList",
57+
"java\\.util\\.LinkedList",
58+
"java\\.util\\.Stack",
59+
"java\\.util\\.Vector",
60+
"java\\.util\\.TreeSet",
61+
"java\\.util\\.HashSet",
62+
"java\\.util\\.HashMap",
63+
"java\\.util\\.Properties",
64+
"java\\.util\\.Hashtable",
65+
"java\\.util\\.WeakHashMap",
66+
"java\\.util\\.TreeMap",
67+
"java\\.util\\.Arrays\\$ArrayList",
68+
"org\\.apache\\.ofbiz\\.entity\\.GenericValue",
69+
"org\\.apache\\.ofbiz\\.entity\\.GenericPK"};
4570
private static final String[] DEFAULT_DENYLIST = {"rmi", "<"};
4671

4772
/** The regular expression used to match serialized types. */

‎framework/base/src/main/java/org/apache/ofbiz/base/util/UtilXml.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,15 +91,21 @@ private UtilXml() { }
9191

9292
private static XStream createXStream() {
9393
XStream xstream = new XStream();
94-
/* This method is a pure helper method for XStream 1.4.x.
95-
* It initializes an XStream instance with a white list of well-known and simple types of the Java runtime
96-
* as it is done in XStream 1.5.x by default. This method will do therefore nothing in XStream 1.5
97-
* and could be removed them
98-
*/
99-
// XStream.setupDefaultSecurity(xstream);
100-
/* You may want to enhance the white list created by XStream::setupDefaultSecurity (or by default with XStream 1.5)
101-
* using xstream::allowTypesByWildcard with your own classes
102-
*/
94+
// Allow only the concrete types that XmlSerializer.serializeSingle handles explicitly.
95+
// All other types are blocked to prevent deserialization gadget chain attacks.
96+
// Class names are used as strings to avoid a compile-time dependency on framework/entity.
97+
xstream.allowTypes(new String[]{
98+
"java.lang.String", "java.lang.Integer", "java.lang.Long",
99+
"java.lang.Float", "java.lang.Double", "java.lang.Boolean",
100+
"java.util.Locale", "java.math.BigDecimal",
101+
"java.sql.Timestamp", "java.sql.Date", "java.sql.Time", "java.util.Date",
102+
"java.util.ArrayList", "java.util.LinkedList", "java.util.Stack",
103+
"java.util.Vector", "java.util.TreeSet", "java.util.HashSet",
104+
"java.util.HashMap", "java.util.Properties", "java.util.Hashtable",
105+
"java.util.WeakHashMap", "java.util.TreeMap",
106+
"org.apache.ofbiz.entity.GenericValue",
107+
"org.apache.ofbiz.entity.GenericPK"
108+
});
103109
return xstream;
104110
}
105111

‎framework/base/src/test/java/org/apache/ofbiz/base/util/UtilObjectTests.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,11 @@ public void testGetObject() {
216216
assertNotNull("groovySerializableBytes", groovySerializableBytes);
217217
assertNull("groovyDeserializable", UtilObject.getObject(groovySerializableBytes));
218218

219+
// SerializationInjector is a test-only class; allow it explicitly for this assertion.
220+
// Note: the allowList value is treated as a regex, so '.' is used instead of '$'
221+
// (dot matches any character including the inner-class separator '$').
222+
UtilProperties.setPropertyValueInMemory("SafeObjectInputStream", "allowList",
223+
"org.apache.ofbiz.base.util.UtilObjectTests.SerializationInjector");
219224
byte[] injectorBytes = UtilObject.getBytes(new SerializationInjector(false, false));
220225
assertNotNull("injectorBytes good", injectorBytes);
221226
assertNotNull("injector good", UtilObject.getObject(injectorBytes));

‎framework/entity/src/main/java/org/apache/ofbiz/entity/serialize/XmlSerializer.java‎

Lines changed: 8 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020

2121
import java.io.FileNotFoundException;
2222
import java.io.IOException;
23-
import java.io.Serializable;
2423
import java.lang.ref.WeakReference;
2524
import java.math.BigDecimal;
2625
import java.math.RoundingMode;
@@ -48,10 +47,8 @@
4847
import javax.xml.parsers.ParserConfigurationException;
4948

5049
import org.apache.ofbiz.base.util.Debug;
51-
import org.apache.ofbiz.base.util.StringUtil;
5250
import org.apache.ofbiz.base.util.UtilGenerics;
5351
import org.apache.ofbiz.base.util.UtilMisc;
54-
import org.apache.ofbiz.base.util.UtilObject;
5552
import org.apache.ofbiz.base.util.UtilXml;
5653
import org.apache.ofbiz.entity.Delegator;
5754
import org.apache.ofbiz.entity.GenericPK;
@@ -259,18 +256,11 @@ public static Element serializeSingle(Object object, Document document) throws S
259256
}
260257

261258
public static Element serializeCustom(Object object, Document document) throws SerializeException {
262-
if (object instanceof Serializable) {
263-
byte[] objBytes = UtilObject.getBytes(object);
264-
if (objBytes == null) {
265-
throw new SerializeException("Unable to serialize object; null byte array returned");
266-
}
267-
String byteHex = StringUtil.toHexString(objBytes);
268-
Element element = document.createElement("cus-obj");
269-
// this is hex encoded so does not need to be in a CDATA block
270-
element.appendChild(document.createTextNode(byteHex));
271-
return element;
272-
}
273-
throw new SerializeException("Cannot serialize object of class " + object.getClass().getName());
259+
Debug.logError("Serialization of custom Java objects (cus-obj) is no longer supported. "
260+
+ "This feature has been removed for security reasons. Object class: "
261+
+ object.getClass().getName(), MODULE);
262+
throw new SerializeException("Serialization of custom Java objects is not supported. "
263+
+ "Object class: " + object.getClass().getName());
274264
}
275265

276266
public static Element makeElement(String elementName, Object value, Document document) {
@@ -467,17 +457,9 @@ public static Object deserializeSingle(Element element, Delegator delegator) thr
467457
public static Object deserializeCustom(Element element) throws SerializeException {
468458
String tagName = element.getLocalName();
469459
if ("cus-obj".equals(tagName)) {
470-
String value = UtilXml.elementValue(element);
471-
if (value != null) {
472-
byte[] valueBytes = StringUtil.fromHexString(value);
473-
if (valueBytes != null) {
474-
Object obj = UtilObject.getObject(valueBytes);
475-
if (obj != null) {
476-
return obj;
477-
}
478-
}
479-
}
480-
throw new SerializeException("Problem deserializing object from byte array + " + element.getLocalName());
460+
Debug.logError("Deserialization of cus-obj elements is no longer supported. "
461+
+ "This feature has been removed for security reasons.", MODULE);
462+
throw new SerializeException("Deserialization of cus-obj elements is not supported.");
481463
}
482464
throw new SerializeException("Cannot deserialize element named " + element.getLocalName());
483465
}

‎framework/service/src/main/java/org/apache/ofbiz/service/jms/AbstractJmsListener.java‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,13 @@ protected Map<String, Object> runService(MapMessage message) {
7171
return null;
7272
}
7373

74+
// Authorization check BEFORE deserialization to prevent gadget chain attacks.
75+
ModelService model = dispatcher.getDispatchContext().getModelService(serviceName);
76+
if (!model.isExport()) {
77+
Debug.logWarning("Attempt to invoke a non-exported service: " + serviceName, MODULE);
78+
return null;
79+
}
80+
7481
Object o = XmlSerializer.deserialize(xmlContext, dispatcher.getDelegator());
7582

7683
if (Debug.verboseOn()) {
@@ -81,20 +88,12 @@ protected Map<String, Object> runService(MapMessage message) {
8188
}
8289
} catch (JMSException je) {
8390
Debug.logError(je, "Problems reading message.", MODULE);
91+
} catch (GenericServiceException e) {
92+
Debug.logError(e, "Unable to get ModelService for service: " + serviceName, MODULE);
8493
} catch (Exception e) {
8594
Debug.logError(e, "Problems deserializing the service context.", MODULE);
8695
}
8796

88-
try {
89-
ModelService model = dispatcher.getDispatchContext().getModelService(serviceName);
90-
if (!model.isExport()) {
91-
Debug.logWarning("Attempt to invoke a non-exported service: " + serviceName, MODULE);
92-
return null;
93-
}
94-
} catch (GenericServiceException e) {
95-
Debug.logError(e, "Unable to get ModelService for service : " + serviceName, MODULE);
96-
}
97-
9897
if (Debug.verboseOn()) {
9998
Debug.logVerbose("Running service: " + serviceName, MODULE);
10099
}

0 commit comments

Comments
 (0)