Fixed: Prevent deserialization of untrusted data in JMS listener

[jacopoc]

• 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

[JacquesLeRoux]

Hi Jacopo,

I'm not sure why you removed
"org.codehaus.groovy.runtime.GStringImpl", "groovy.lang.GString"
from SafeObjectInputStream::SafeObjectInputStream.
They were added because of https://s.apache.org/pitaw
But indeed the issue can't be reproduced once the PR patch applied.
Despite the code

    result.successMessageList = [
        "Categories updated: ${categoriesUpdated}",
        "Products updated: ${productsUpdated}"

still being in CatalogServices.groovy::createMissingCategoryAndProductAltUrls

Could it be related with now using Groovy 5.0.0-alpha-11, or another specific change?

[jacopoc]

@JacquesLeRoux This is just an experiment and work in progress.
At the moment you are not getting the error because the (less permissive) allow list that I have introduced is not used because the property allowListof SafeObjectInputStream is still the same as before.

[JacquesLeRoux]

OK thanks, please let me know when I should review :)

[jacopoc]

@JacquesLeRoux this PR is ready for review. I’d appreciate it if you could check whether all the previously reported issues (e.g., in ticket OFBIZ-10837 ) that were fixed at the time are still properly addressed.

[JacquesLeRoux]

Hi Jacopo,

I tested the issues from the 2 1st related Jiras and reviewed the last related Jira, it's OK with me.

As I explained above you need to put back "org.codehaus.groovy.runtime.GStringImpl", "groovy.lang.GString" in SafeObjectInputStream::DEFAULT_ALLOWLIST_PATTERN.
Because if for a reason it's not in SafeObjectInputStream.properties, or SafeObjectInputStream.properties does not exist, then you cross the error fixed with https://issues.apache.org/jira/browse/OFBIZ-11398
It's a weak probability but can't be neglected.

On the other hand, "sun.util.calendar." can be removed from both SafeObjectInputStream::DEFAULT_ALLOWLIST_PATTERN. and SafeObjectInputStream.properties since it's no longer used in OFBiz current supported branches after https://issues.apache.org/jira/browse/OFBIZ-12721

For "Serialization of custom Java objects (cus-obj) is no longer supported. " I guess you decided that because none custom Java objects are serialised OOTB, or is there another reason?

I must say I did not completely check the change in createXStream::createXStream. At 1st glance it's OK.

The rest is OK with me.

[jacopoc]

@JacquesLeRoux, thank you.

I introduced these changes because I am working to fix this:
https://github.com/apache/ofbiz-framework/security/code-scanning/1158

I'd like to keep DEFAULT_ALLOWLIST_PATTERN as minimal as possible in order to easily allow framework setups that do not require to deserialize special objects, such as GString, that may open some security related concerns.

This is the primary reason to remove the ability to deserialize custom object (cus-obj) as they are difficult/impossible to check for safety.

Thanks for your note on removing "sun.util.calendar": I will have a look at it.

[JacquesLeRoux]

I understand for custom object. Then another way must be found for OFBIZ-11398, even if It's a weak probability.

[jacopoc]

I think it is fine, assuming that the feature mentioned in OFBIZ-11398 requires the current configuration in SafeObjectInputStream.properties. There are many other features in OFBiz that work only with specific configurations.

[jacopoc]

@JacquesLeRoux I have added a commit to remove sun.util.calendar

[JacquesLeRoux]

HI Jacopo,

I think it is fine, assuming that the feature mentioned in OFBIZ-11398 requires the current configuration in SafeObjectInputStream.properties. There are many other features in OFBiz that work only with specific configurations.

I'd not agree (because in this case it's about - a weak - security issue) if the other side was not as you said:

I'd like to keep DEFAULT_ALLOWLIST_PATTERN as minimal as possible in order to easily allow framework setups that do not require to deserialize special objects, such as GString, that may open some security related concerns

I see nothing else to add.

[jacopoc]

@JacquesLeRoux we clearly disagree; however, since I don’t want this detail to hold back my work, I’ve pushed a commit adding the GString* classes to DEFAULT_ALLOWLIST_PATTERN, as you requested.

[JacquesLeRoux]

Actually, when saying
if the other side was not as you said:
I was somehow agreeing. We face 2 possible hazards, between 2 hazards better select the less

Summary: it's OK with me.