Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WW-5343 Make SecurityMemberAccess an extensible bean #791

Merged
merged 20 commits into from
Nov 26, 2023
Merged

WW-5343 Make SecurityMemberAccess an extensible bean #791

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
79ffc86
WW-5343 Delete unused code and consolidate constructors
kusalk Aug 31, 2023
0825329
WW-5343 Extract ConfigParseUtil
kusalk Nov 14, 2023
b0b80ba
WW-5343 Extract deprecated methods as default interface methods
kusalk Nov 14, 2023
9e556e9
WW-5343 Deprecate unnecessary setter
kusalk Nov 14, 2023
90344b3
WW-5343 Make SecurityMemberAccess a prototype bean
kusalk Nov 14, 2023
7e92a8d
WW-5343 Refactor OgnlValueStackFactory to utilise SecurityMemberAcces…
kusalk Nov 14, 2023
b518635
WW-5343 Update OgnlUtil#createDefaultContext to utilise SecurityMembe…
kusalk Nov 14, 2023
4490d9d
WW-5343 Move configuration injection from OgnlUtil to SecurityMemberA…
kusalk Nov 14, 2023
8bf47b3
WW-5343 Fix OgnlUtilTest#testBeanMapExpressions
kusalk Nov 14, 2023
62988f7
WW-5343 Fix unit test compilation errors
kusalk Nov 14, 2023
ceff6cd
WW-5343 Remove unnecessary method
kusalk Nov 16, 2023
f87d7d7
WW-5343 Add missing license
kusalk Nov 17, 2023
a402e5c
WW-5343 Revert and fix serializability
kusalk Nov 17, 2023
d0d10d9
WW-5343 Fix MemberAccess access blocked tests
kusalk Nov 17, 2023
68f5584
WW-5343 Remove defunct test now that constant is required
kusalk Nov 17, 2023
05a9973
Merge branch 'master' into WW-5343-sec-extend
kusalk Nov 17, 2023
9640f5b
WW-5343 Migrate tests to SecurityMemberAccessTest
kusalk Nov 22, 2023
eba79a7
WW-5343 Fix final test
kusalk Nov 22, 2023
85d2c74
WW-5343 Clean up bootstrap constants
kusalk Nov 26, 2023
7929d86
WW-5343 Address SonarCloud code smells
kusalk Nov 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
import com.opensymphony.xwork2.ognl.OgnlReflectionProvider;
import com.opensymphony.xwork2.ognl.OgnlUtil;
import com.opensymphony.xwork2.ognl.OgnlValueStackFactory;
import com.opensymphony.xwork2.ognl.SecurityMemberAccess;
import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor;
import com.opensymphony.xwork2.util.CompoundRoot;
import com.opensymphony.xwork2.util.OgnlTextParser;
Expand Down Expand Up @@ -133,7 +134,6 @@ public class DefaultConfiguration implements Configuration {
Map<String, Object> constants = new HashMap<>();
constants.put(StrutsConstants.STRUTS_DEVMODE, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_OGNL_LOG_MISSING_PROPERTIES, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_OGNL_ENABLE_EVAL_EXPRESSION, Boolean.FALSE);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constant is not required for bootstrapping

constants.put(StrutsConstants.STRUTS_OGNL_ENABLE_EXPRESSION_CACHE, Boolean.TRUE);
constants.put(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, Boolean.FALSE);
constants.put(StrutsConstants.STRUTS_I18N_RELOAD, Boolean.FALSE);
Expand All @@ -142,6 +142,7 @@ public class DefaultConfiguration implements Configuration {
constants.put(StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE, 10000);
constants.put(StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_TYPE, OgnlCacheFactory.CacheType.BASIC);
constants.put(StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_MAXSIZE, 10000);
constants.put(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, Boolean.TRUE);
lukaszlenart marked this conversation as resolved.
Show resolved Hide resolved
BOOTSTRAP_CONSTANTS = Collections.unmodifiableMap(constants);
}

Expand Down Expand Up @@ -385,6 +386,7 @@ protected Container createBootstrapContainer(List<ContainerProvider> providers)
builder.factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON);
builder.factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON);
builder.factory(OgnlUtil.class, Scope.SINGLETON);
builder.factory(SecurityMemberAccess.class, Scope.PROTOTYPE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is a good idea, attacker could get access to the SMA just by having access to the Container

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given it's prototype scope, I don't believe it to make any difference

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, the container would return a new instance, and not the one that is being used by the ValueStack

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant it will be possible creating a new instance via containers methods, yet we excluded the whole package com.opensymphony.xwork2.inject which means attacker cannot use container in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I also don't think there's anything useful an attacker can do with a fresh SecurityMemberAccess instance

builder.factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON);

builder.factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON);
Expand Down
2 changes: 2 additions & 0 deletions ...ain/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
import com.opensymphony.xwork2.ognl.OgnlReflectionProvider;
import com.opensymphony.xwork2.ognl.OgnlUtil;
import com.opensymphony.xwork2.ognl.OgnlValueStackFactory;
import com.opensymphony.xwork2.ognl.SecurityMemberAccess;
import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor;
import com.opensymphony.xwork2.ognl.accessor.HttpParametersPropertyAccessor;
import com.opensymphony.xwork2.ognl.accessor.ObjectAccessor;
Expand Down Expand Up @@ -230,6 +231,7 @@ public void register(ContainerBuilder builder, LocatableProperties props)
.factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON)
.factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON)
.factory(OgnlUtil.class, Scope.SINGLETON)
.factory(SecurityMemberAccess.class, Scope.PROTOTYPE)
.factory(OgnlGuard.class, StrutsOgnlGuard.class, Scope.SINGLETON)
.factory(CollectionConverter.class, Scope.SINGLETON)
.factory(ArrayConverter.class, Scope.SINGLETON)
Expand Down
218 changes: 71 additions & 147 deletions core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/
package com.opensymphony.xwork2.ognl;

import com.opensymphony.xwork2.config.ConfigurationException;
import com.opensymphony.xwork2.conversion.impl.XWorkConverter;
import com.opensymphony.xwork2.inject.Container;
import com.opensymphony.xwork2.inject.Inject;
Expand Down Expand Up @@ -46,22 +45,16 @@
import java.lang.reflect.Method;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;

import static com.opensymphony.xwork2.util.TextParseUtil.commaDelimitedStringToSet;
import static com.opensymphony.xwork2.util.ConfigParseUtil.toClassesSet;
import static com.opensymphony.xwork2.util.ConfigParseUtil.toNewPatternsSet;
import static com.opensymphony.xwork2.util.ConfigParseUtil.toPackageNamesSet;
import static java.util.Collections.emptySet;
import static java.util.Collections.unmodifiableSet;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toSet;
import static org.apache.commons.lang3.StringUtils.strip;
import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_CLASSES;
import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_ENABLE;
import static org.apache.struts2.StrutsConstants.STRUTS_ALLOWLIST_PACKAGE_NAMES;
import static org.apache.struts2.ognl.OgnlGuard.EXPR_BLOCKED;


Expand All @@ -84,27 +77,15 @@ public class OgnlUtil {
private final OgnlGuard ognlGuard;

private boolean devMode;
private boolean enableExpressionCache = true;
private boolean enableExpressionCache;
private boolean enableEvalExpression;

private Set<String> excludedClasses = emptySet();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the configuration is now injected directly into SecurityMemberAccess except for devMode configuration

private Set<Pattern> excludedPackageNamePatterns = emptySet();
private Set<String> excludedPackageNames = emptySet();
private Set<String> excludedPackageExemptClasses = emptySet();

private boolean enforceAllowlistEnabled = false;
private Set<String> allowlistClasses = emptySet();
private Set<String> allowlistPackageNames = emptySet();

private Set<String> devModeExcludedClasses = emptySet();
private Set<Pattern> devModeExcludedPackageNamePatterns = emptySet();
private Set<String> devModeExcludedPackageNames = emptySet();
private Set<String> devModeExcludedPackageExemptClasses = emptySet();
private String devModeExcludedClasses = "";
private String devModeExcludedPackageNamePatterns = "";
private String devModeExcludedPackageNames = "";
private String devModeExcludedPackageExemptClasses = "";

private Container container;
private boolean allowStaticFieldAccess = true;
private boolean disallowProxyMemberAccess = false;
private boolean disallowDefaultPackageAccess = false;

/**
* Construct a new OgnlUtil instance for use with the framework
Expand Down Expand Up @@ -175,166 +156,110 @@ protected void setEnableEvalExpression(String evalExpression) {
}
}

@Inject(value = StrutsConstants.STRUTS_EXCLUDED_CLASSES, required = false)
/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
protected void setExcludedClasses(String commaDelimitedClasses) {
excludedClasses = toNewClassesSet(excludedClasses, commaDelimitedClasses);
}

@Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_CLASSES, required = false)
protected void setDevModeExcludedClasses(String commaDelimitedClasses) {
devModeExcludedClasses = toNewClassesSet(devModeExcludedClasses, commaDelimitedClasses);
this.devModeExcludedClasses = commaDelimitedClasses;
}

private static Set<String> toClassesSet(String newDelimitedClasses) throws ConfigurationException {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved these utility methods into their own class

Set<String> classNames = commaDelimitedStringToSet(newDelimitedClasses);
validateClasses(classNames, OgnlUtil.class.getClassLoader());
return unmodifiableSet(classNames);
}

private static Set<String> toNewClassesSet(Set<String> oldClasses, String newDelimitedClasses) throws ConfigurationException {
Set<String> classNames = commaDelimitedStringToSet(newDelimitedClasses);
validateClasses(classNames, OgnlUtil.class.getClassLoader());
Set<String> excludedClasses = new HashSet<>(oldClasses);
excludedClasses.addAll(classNames);
return unmodifiableSet(excludedClasses);
}

private static void validateClasses(Set<String> classNames, ClassLoader validatingClassLoader) throws ConfigurationException {
for (String className : classNames) {
try {
validatingClassLoader.loadClass(className);
} catch (ClassNotFoundException e) {
throw new ConfigurationException("Cannot load class for exclusion/exemption configuration: " + className, e);
}
}
}

@Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false)
/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
protected void setExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) {
excludedPackageNamePatterns = toNewPatternsSet(excludedPackageNamePatterns, commaDelimitedPackagePatterns);
}

@Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAME_PATTERNS, required = false)
protected void setDevModeExcludedPackageNamePatterns(String commaDelimitedPackagePatterns) {
devModeExcludedPackageNamePatterns = toNewPatternsSet(devModeExcludedPackageNamePatterns, commaDelimitedPackagePatterns);
this.devModeExcludedPackageNamePatterns = commaDelimitedPackagePatterns;
}

private static Set<Pattern> toNewPatternsSet(Set<Pattern> oldPatterns, String newDelimitedPatterns) throws ConfigurationException {
Set<String> patterns = commaDelimitedStringToSet(newDelimitedPatterns);
Set<Pattern> newPatterns = new HashSet<>(oldPatterns);
for (String pattern: patterns) {
try {
newPatterns.add(Pattern.compile(pattern));
} catch (PatternSyntaxException e) {
throw new ConfigurationException("Excluded package name patterns could not be parsed due to invalid regex: " + pattern, e);
}
}
return unmodifiableSet(newPatterns);
}

@Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES, required = false)
/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
protected void setExcludedPackageNames(String commaDelimitedPackageNames) {
excludedPackageNames = toNewPackageNamesSet(excludedPackageNames, commaDelimitedPackageNames);
}

@Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_NAMES, required = false)
protected void setDevModeExcludedPackageNames(String commaDelimitedPackageNames) {
devModeExcludedPackageNames = toNewPackageNamesSet(devModeExcludedPackageNames, commaDelimitedPackageNames);
}

private static Set<String> toPackageNamesSet(String newDelimitedPackageNames) throws ConfigurationException {
Set<String> packageNames = commaDelimitedStringToSet(newDelimitedPackageNames)
.stream().map(s -> strip(s, ".")).collect(toSet());
validatePackageNames(packageNames);
return unmodifiableSet(packageNames);
}

private static Set<String> toNewPackageNamesSet(Collection<String> oldPackageNames, String newDelimitedPackageNames) throws ConfigurationException {
Set<String> packageNames = commaDelimitedStringToSet(newDelimitedPackageNames)
.stream().map(s -> strip(s, ".")).collect(toSet());
validatePackageNames(packageNames);
Set<String> newPackageNames = new HashSet<>(oldPackageNames);
newPackageNames.addAll(packageNames);
return unmodifiableSet(newPackageNames);
this.devModeExcludedPackageNames = commaDelimitedPackageNames;
}

private static void validatePackageNames(Collection<String> packageNames) {
if (packageNames.stream().anyMatch(s -> Pattern.compile("\\s").matcher(s).find())) {
throw new ConfigurationException("Excluded package names could not be parsed due to erroneous whitespace characters: " + packageNames);
}
}

@Inject(value = StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false)
/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
public void setExcludedPackageExemptClasses(String commaDelimitedClasses) {
excludedPackageExemptClasses = toClassesSet(commaDelimitedClasses);
}

@Inject(value = StrutsConstants.STRUTS_DEV_MODE_EXCLUDED_PACKAGE_EXEMPT_CLASSES, required = false)
public void setDevModeExcludedPackageExemptClasses(String commaDelimitedClasses) {
devModeExcludedPackageExemptClasses = toClassesSet(commaDelimitedClasses);
this.devModeExcludedPackageExemptClasses = commaDelimitedClasses;
}

/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
public Set<String> getExcludedClasses() {
return excludedClasses;
return toClassesSet(container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_CLASSES));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated getters are still functional for API compatibility

}

/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
public Set<Pattern> getExcludedPackageNamePatterns() {
return excludedPackageNamePatterns;
return toNewPatternsSet(emptySet(), container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAME_PATTERNS));
}

/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
public Set<String> getExcludedPackageNames() {
return excludedPackageNames;
return toPackageNamesSet(container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_PACKAGE_NAMES));
}

/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
public Set<String> getExcludedPackageExemptClasses() {
return excludedPackageExemptClasses;
}

@Inject(value = STRUTS_ALLOWLIST_ENABLE, required = false)
protected void setEnforceAllowlistEnabled(String enforceAllowlistEnabled) {
this.enforceAllowlistEnabled = BooleanUtils.toBoolean(enforceAllowlistEnabled);
}

@Inject(value = STRUTS_ALLOWLIST_CLASSES, required = false)
protected void setAllowlistClasses(String commaDelimitedClasses) {
allowlistClasses = toClassesSet(commaDelimitedClasses);
}

@Inject(value = STRUTS_ALLOWLIST_PACKAGE_NAMES, required = false)
protected void setAllowlistPackageNames(String commaDelimitedPackageNames) {
allowlistPackageNames = toPackageNamesSet(commaDelimitedPackageNames);
}

public boolean isEnforceAllowlistEnabled() {
return enforceAllowlistEnabled;
}

public Set<String> getAllowlistClasses() {
return allowlistClasses;
}

public Set<String> getAllowlistPackageNames() {
return allowlistPackageNames;
return toClassesSet(container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_PACKAGE_EXEMPT_CLASSES));
}

@Inject
protected void setContainer(Container container) {
this.container = container;
}

@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false)
/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
protected void setAllowStaticFieldAccess(String allowStaticFieldAccess) {
this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess);
}

@Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false)
/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) {
this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess);
}

@Inject(value = StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS, required = false)
/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
protected void setDisallowDefaultPackageAccess(String disallowDefaultPackageAccess) {
this.disallowDefaultPackageAccess = BooleanUtils.toBoolean(disallowDefaultPackageAccess);
}

/**
Expand All @@ -356,12 +281,20 @@ protected void applyExpressionMaxLength(String maxLength) {
}
}

/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
public boolean isDisallowProxyMemberAccess() {
return disallowProxyMemberAccess;
return BooleanUtils.toBoolean(container.getInstance(String.class, StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS));
}

/**
* @deprecated since 6.4.0, no replacement.
*/
@Deprecated
public boolean isDisallowDefaultPackageAccess() {
return disallowDefaultPackageAccess;
return BooleanUtils.toBoolean(container.getInstance(String.class, StrutsConstants.STRUTS_DISALLOW_DEFAULT_PACKAGE_ACCESS));
}

/**
Expand Down Expand Up @@ -922,8 +855,7 @@ protected Map<String, Object> createDefaultContext(Object root, ClassResolver cl
resolver = container.getInstance(CompoundRootAccessor.class);
}

SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticFieldAccess);
memberAccess.disallowProxyMemberAccess(disallowProxyMemberAccess);
SecurityMemberAccess memberAccess = container.getInstance(SecurityMemberAccess.class);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a prototype bean so will be a new instance per context


if (devMode) {
if (!warnReported.get()) {
Expand All @@ -934,14 +866,6 @@ protected Map<String, Object> createDefaultContext(Object root, ClassResolver cl
memberAccess.useExcludedPackageNamePatterns(devModeExcludedPackageNamePatterns);
memberAccess.useExcludedPackageNames(devModeExcludedPackageNames);
memberAccess.useExcludedPackageExemptClasses(devModeExcludedPackageExemptClasses);
} else {
memberAccess.useExcludedClasses(getExcludedClasses());
memberAccess.useExcludedPackageNamePatterns(getExcludedPackageNamePatterns());
memberAccess.useExcludedPackageNames(getExcludedPackageNames());
memberAccess.useExcludedPackageExemptClasses(getExcludedPackageExemptClasses());
memberAccess.useEnforceAllowlistEnabled(isEnforceAllowlistEnabled());
memberAccess.useAllowlistClasses(getAllowlistClasses());
memberAccess.useAllowlistPackageNames(getAllowlistPackageNames());
}

return Ognl.createDefaultContext(root, memberAccess, resolver, defaultConverter);
Expand Down
Loading