From 4a5edf640fb9e54eb22ee5421ba49cab9583a598 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 30 Aug 2023 12:49:14 +1000 Subject: [PATCH 1/3] WW-5342 Ban use of default package --- .../com/opensymphony/xwork2/ognl/SecurityMemberAccess.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 3e1c69d5db..2ce702008c 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -149,7 +149,8 @@ public boolean isAccessible(Map context, Object target, Member member, String pr } if (targetClass.getPackage() == null || memberClass.getPackage() == null) { - LOG.warn("The use of the default (unnamed) package is discouraged!"); + LOG.warn("The use of the default package is blocked!"); + return false; } if (isPackageExcluded(targetClass)) { @@ -225,7 +226,7 @@ protected boolean isPackageExcluded(Class clazz) { public static String toPackageName(Class clazz) { if (clazz.getPackage() == null) { - return ""; + throw new IllegalArgumentException("The use of the default package is blocked!"); } return clazz.getPackage().getName(); } From ff79c1b041871f56c6f5275a3a25b28960f07c91 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 30 Aug 2023 23:06:22 +1000 Subject: [PATCH 2/3] WW-5342 Implement default off option --- .../opensymphony/xwork2/ognl/OgnlUtil.java | 10 ++++++++++ .../xwork2/ognl/OgnlValueStack.java | 1 + .../xwork2/ognl/SecurityMemberAccess.java | 19 +++++++++++++++---- .../org/apache/struts2/StrutsConstants.java | 1 + .../xwork2/ognl/SecurityMemberAccessTest.java | 10 ++++++++++ 5 files changed, 37 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index 8f2872669c..b239719da2 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -92,6 +92,7 @@ public class OgnlUtil { private Container container; private boolean allowStaticFieldAccess = true; private boolean disallowProxyMemberAccess; + private boolean disallowDefaultPackageAccess; /** * Construct a new OgnlUtil instance for use with the framework @@ -287,6 +288,11 @@ protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) { this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess); } + @Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false) + protected void setDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) { + this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess); + } + /** * @param maxLength Injects the Struts OGNL expression maximum length. */ @@ -310,6 +316,10 @@ public boolean isDisallowProxyMemberAccess() { return disallowProxyMemberAccess; } + public boolean isDisallowDefaultPackageAccess() { + return disallowDefaultPackageAccess; + } + /** * Convenience mechanism to clear the OGNL Runtime Cache via OgnlUtil. May be utilized * by applications that generate many unique OGNL expressions over time. diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java index 336312c897..936619ae45 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStack.java @@ -94,6 +94,7 @@ protected void setOgnlUtil(OgnlUtil ognlUtil) { securityMemberAccess.useExcludedPackageNames(ognlUtil.getExcludedPackageNames()); securityMemberAccess.useExcludedPackageExemptClasses(ognlUtil.getExcludedPackageExemptClasses()); securityMemberAccess.disallowProxyMemberAccess(ognlUtil.isDisallowProxyMemberAccess()); + securityMemberAccess.disallowDefaultPackageAccess(ognlUtil.isDisallowDefaultPackageAccess()); } protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor accessor, CompoundRoot compoundRoot, boolean allowStaticFieldAccess) { diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 2ce702008c..4d01a952fc 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -56,6 +56,7 @@ public class SecurityMemberAccess implements MemberAccess { private Set excludedPackageNames = emptySet(); private Set excludedPackageExemptClasses = emptySet(); private boolean disallowProxyMemberAccess; + private boolean disallowDefaultPackageAccess; /** * SecurityMemberAccess @@ -148,9 +149,15 @@ public boolean isAccessible(Map context, Object target, Member member, String pr return false; } - if (targetClass.getPackage() == null || memberClass.getPackage() == null) { - LOG.warn("The use of the default package is blocked!"); - return false; + if (disallowDefaultPackageAccess) { + if (targetClass.getPackage() == null || targetClass.getPackage().getName().isEmpty()) { + LOG.warn("Class [{}] from the default package is excluded!", targetClass); + return false; + } + if (memberClass.getPackage() == null || memberClass.getPackage().getName().isEmpty()) { + LOG.warn("Class [{}] from the default package is excluded!", memberClass); + return false; + } } if (isPackageExcluded(targetClass)) { @@ -226,7 +233,7 @@ protected boolean isPackageExcluded(Class clazz) { public static String toPackageName(Class clazz) { if (clazz.getPackage() == null) { - throw new IllegalArgumentException("The use of the default package is blocked!"); + return ""; } return clazz.getPackage().getName(); } @@ -375,4 +382,8 @@ public void setDisallowProxyMemberAccess(boolean disallowProxyMemberAccess) { public void disallowProxyMemberAccess(boolean disallowProxyMemberAccess) { this.disallowProxyMemberAccess = disallowProxyMemberAccess; } + + public void disallowDefaultPackageAccess(boolean disallowDefaultPackageAccess) { + this.disallowDefaultPackageAccess = disallowDefaultPackageAccess; + } } diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index 29dfca4b98..f93370eaa6 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -448,6 +448,7 @@ public final class StrutsConstants { public static final String STRUTS_LOCALIZED_TEXT_PROVIDER = "struts.localizedTextProvider"; public static final String STRUTS_DISALLOW_PROXY_MEMBER_ACCESS = "struts.disallowProxyMemberAccess"; + public static final String STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS = "struts.disallowDefaultPackageAccess"; public static final String STRUTS_OGNL_AUTO_GROWTH_COLLECTION_LIMIT = "struts.ognl.autoGrowthCollectionLimit"; diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java index 15a9885c9e..f38a7dce66 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java @@ -315,6 +315,16 @@ public void testDefaultPackageExclusion() throws Exception { assertFalse("default package is excluded!", actual); } + @Test + public void testDefaultPackageExclusionSetting() throws Exception { + sma.disallowDefaultPackageAccess(true); + + Class clazz = Class.forName("PackagelessAction"); + boolean actual = sma.isAccessible(null, clazz.getConstructor().newInstance(), clazz.getMethod("execute"), null); + + assertFalse("default package isn't excluded!", actual); + } + @Test public void testDefaultPackageExclusion2() throws Exception { // given From bfe1f8cd30d9cc07a0a4b71169d4a8191d2199fc Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 30 Aug 2023 23:08:14 +1000 Subject: [PATCH 3/3] WW-5342 Optimise package exclusion check --- .../com/opensymphony/xwork2/ognl/SecurityMemberAccess.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java index 4d01a952fc..232c33e5aa 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java @@ -144,7 +144,6 @@ public boolean isAccessible(Map context, Object target, Member member, String pr } if (targetClass != memberClass && isClassExcluded(targetClass)) { - // Optimization: Already checked memberClass exclusion, so if-and-only-if targetClass == memberClass, this check is redundant. LOG.warn("Target class [{}] of target [{}] is excluded!", targetClass, target); return false; } @@ -168,7 +167,7 @@ public boolean isAccessible(Map context, Object target, Member member, String pr return false; } - if (isPackageExcluded(memberClass)) { + if (targetClass != memberClass && isPackageExcluded(memberClass)) { LOG.warn("Package [{}] of member [{}] are excluded!", memberClass.getPackage(), member); return false; }