Skip to content

Commit

Permalink
Fix for access issue for 2.6 discovered in WW-5004 (2nd amended commit):
Browse files Browse the repository at this point in the history
- Restored ability to access public static fields (true by default).
- Introduced a boolean configuration flag (allowStaticFieldAccess).
- Replaced one remaining Boolean.parseBoolean() conversion in OgnlUtil use BooleanUtils.toBoolean().
- Enhanced unit tests to confirm proper operation of the fix.
- Replicating L. Lenart's change in PR#317:
  - Removed injection parameter for setAllowStaticMethodAccess in OgnlValueStackFactory.
  - Replaced with lazy retrieval of allowStaticMethodAccess from container.
  - Used same pattern for the new allowStaticFieldAccess flag.
  - Added retrieval methods for both flags from the container.
  • Loading branch information
JCgH4164838Gh792C124B5 committed Jan 30, 2019
1 parent 6183fa0 commit 925eb62
Show file tree
Hide file tree
Showing 16 changed files with 553 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ public void register(ContainerBuilder builder, LocatableProperties props)
props.setProperty(StrutsConstants.STRUTS_ENABLE_OGNL_EVAL_EXPRESSION, Boolean.FALSE.toString());
props.setProperty(StrutsConstants.STRUTS_CONFIGURATION_XML_RELOAD, Boolean.FALSE.toString());
props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, Boolean.FALSE.toString());
props.setProperty(StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, Boolean.TRUE.toString());
}

}
10 changes: 8 additions & 2 deletions core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public class OgnlUtil {
private Set<String> excludedPackageNames;

private Container container;
private boolean allowStaticFieldAccess = true;
private boolean allowStaticMethodAccess;
private boolean disallowProxyMemberAccess;

Expand Down Expand Up @@ -173,14 +174,19 @@ protected void setContainer(Container container) {
this.container = container;
}

@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS, required = false)
protected void setAllowStaticFieldAccess(String allowStaticFieldAccess) {
this.allowStaticFieldAccess = BooleanUtils.toBoolean(allowStaticFieldAccess);
}

@Inject(value = StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS, required = false)
protected void setAllowStaticMethodAccess(String allowStaticMethodAccess) {
this.allowStaticMethodAccess = BooleanUtils.toBoolean(allowStaticMethodAccess);
}

@Inject(value = StrutsConstants.STRUTS_DISALLOW_PROXY_MEMBER_ACCESS, required = false)
protected void setDisallowProxyMemberAccess(String disallowProxyMemberAccess) {
this.disallowProxyMemberAccess = Boolean.parseBoolean(disallowProxyMemberAccess);
this.disallowProxyMemberAccess = BooleanUtils.toBoolean(disallowProxyMemberAccess);
}

public boolean isDisallowProxyMemberAccess() {
Expand Down Expand Up @@ -699,7 +705,7 @@ protected Map createDefaultContext(Object root, ClassResolver classResolver) {
resolver = container.getInstance(CompoundRootAccessor.class);
}

SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticMethodAccess);
SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticMethodAccess, allowStaticFieldAccess);
memberAccess.setExcludedClasses(excludedClasses);
memberAccess.setExcludedPackageNamePatterns(excludedPackageNamePatterns);
memberAccess.setExcludedPackageNames(excludedPackageNames);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@ public class OgnlValueStack implements Serializable, ValueStack, ClearableValueS
private boolean devMode;
private boolean logMissingProperties;

protected OgnlValueStack(XWorkConverter xworkConverter, CompoundRootAccessor accessor, TextProvider prov, boolean allowStaticAccess) {
setRoot(xworkConverter, accessor, new CompoundRoot(), allowStaticAccess);
protected OgnlValueStack(XWorkConverter xworkConverter, CompoundRootAccessor accessor, TextProvider prov, boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) {
setRoot(xworkConverter, accessor, new CompoundRoot(), allowStaticMethodAccess, allowStaticFieldAccess);
push(prov);
}

protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter, CompoundRootAccessor accessor, boolean allowStaticAccess) {
setRoot(xworkConverter, accessor, new CompoundRoot(vs.getRoot()), allowStaticAccess);
protected OgnlValueStack(ValueStack vs, XWorkConverter xworkConverter, CompoundRootAccessor accessor, boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) {
setRoot(xworkConverter, accessor, new CompoundRoot(vs.getRoot()), allowStaticMethodAccess, allowStaticFieldAccess);
}

@Inject
Expand All @@ -91,9 +91,9 @@ protected void setOgnlUtil(OgnlUtil ognlUtil) {
}

protected void setRoot(XWorkConverter xworkConverter, CompoundRootAccessor accessor, CompoundRoot compoundRoot,
boolean allowStaticMethodAccess) {
boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) {
this.root = compoundRoot;
this.securityMemberAccess = new SecurityMemberAccess(allowStaticMethodAccess);
this.securityMemberAccess = new SecurityMemberAccess(allowStaticMethodAccess, allowStaticFieldAccess);
this.context = Ognl.createDefaultContext(this.root, securityMemberAccess, accessor, new OgnlTypeConverterWrapper(xworkConverter));
context.put(VALUE_STACK, this);
((OgnlContext) context).setTraceEvaluations(false);
Expand Down Expand Up @@ -448,10 +448,11 @@ private Object readResolve() {
XWorkConverter xworkConverter = cont.getInstance(XWorkConverter.class);
CompoundRootAccessor accessor = (CompoundRootAccessor) cont.getInstance(PropertyAccessor.class, CompoundRoot.class.getName());
TextProvider prov = cont.getInstance(TextProvider.class, "system");
boolean allow = BooleanUtils.toBoolean(cont.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS));
OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor, prov, allow);
final boolean allowStaticMethod = BooleanUtils.toBoolean(cont.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS));
final boolean allowStaticField = BooleanUtils.toBoolean(cont.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS));
OgnlValueStack aStack = new OgnlValueStack(xworkConverter, accessor, prov, allowStaticMethod, allowStaticField);
aStack.setOgnlUtil(cont.getInstance(OgnlUtil.class));
aStack.setRoot(xworkConverter, accessor, this.root, allow);
aStack.setRoot(xworkConverter, accessor, this.root, allowStaticMethod, allowStaticField);

return aStack;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@

import java.util.Map;
import java.util.Set;
import org.apache.struts2.StrutsConstants;

/**
* Creates an Ognl value stack
Expand All @@ -49,7 +50,6 @@ public class OgnlValueStackFactory implements ValueStackFactory {
protected CompoundRootAccessor compoundRootAccessor;
protected TextProvider textProvider;
protected Container container;
private boolean allowStaticMethodAccess;

@Inject
protected void setXWorkConverter(XWorkConverter converter) {
Expand All @@ -60,21 +60,18 @@ protected void setXWorkConverter(XWorkConverter converter) {
protected void setTextProvider(TextProvider textProvider) {
this.textProvider = textProvider;
}

@Inject(value="allowStaticMethodAccess", required=false)
protected void setAllowStaticMethodAccess(String allowStaticMethodAccess) {
this.allowStaticMethodAccess = BooleanUtils.toBoolean(allowStaticMethodAccess);
}

public ValueStack createValueStack() {
ValueStack stack = new OgnlValueStack(xworkConverter, compoundRootAccessor, textProvider, allowStaticMethodAccess);
ValueStack stack = new OgnlValueStack(xworkConverter, compoundRootAccessor, textProvider,
containerAllowsStaticMethodAccess(), containerAllowsStaticFieldAccess());
container.inject(stack);
stack.getContext().put(ActionContext.CONTAINER, container);
return stack;
}

public ValueStack createValueStack(ValueStack stack) {
ValueStack result = new OgnlValueStack(stack, xworkConverter, compoundRootAccessor, allowStaticMethodAccess);
ValueStack result = new OgnlValueStack(stack, xworkConverter, compoundRootAccessor,
containerAllowsStaticMethodAccess(), containerAllowsStaticFieldAccess());
container.inject(result);
stack.getContext().put(ActionContext.CONTAINER, container);
return result;
Expand Down Expand Up @@ -116,4 +113,23 @@ protected void setContainer(Container container) throws ClassNotFoundException {
}
this.container = container;
}

/**
* Retrieve allowsStaticMethodAccess state from the container (allows for lazy fetching)
*
* @return
*/
protected boolean containerAllowsStaticMethodAccess() {
return BooleanUtils.toBoolean(container.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_METHOD_ACCESS));
}

/**
* Retrieve allowStaticFieldAccess state from the container (allows for lazy fetching)
*
* @return
*/
protected boolean containerAllowsStaticFieldAccess() {
return BooleanUtils.toBoolean(container.getInstance(String.class, StrutsConstants.STRUTS_ALLOW_STATIC_FIELD_ACCESS));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.apache.logging.log4j.Logger;

import java.lang.reflect.AccessibleObject;
import java.lang.reflect.Field;
import java.lang.reflect.Member;
import java.lang.reflect.Modifier;
import java.util.Collections;
Expand All @@ -40,6 +41,7 @@ public class SecurityMemberAccess implements MemberAccess {

private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class);

private final boolean allowStaticFieldAccess;
private final boolean allowStaticMethodAccess;
private Set<Pattern> excludeProperties = Collections.emptySet();
private Set<Pattern> acceptProperties = Collections.emptySet();
Expand All @@ -51,18 +53,24 @@ public class SecurityMemberAccess implements MemberAccess {
/**
* SecurityMemberAccess
* - access decisions based on whether member is static (or not)
* - block or allow access to properties (configureable-after-construction)
* - block or allow access to properties (configurable-after-construction)
*
* @param allowStaticMethodAccess
* @param allowStaticFieldAccess
*/
public SecurityMemberAccess(boolean allowStaticMethodAccess) {
public SecurityMemberAccess(boolean allowStaticMethodAccess, boolean allowStaticFieldAccess) {
this.allowStaticMethodAccess = allowStaticMethodAccess;
this.allowStaticFieldAccess = allowStaticFieldAccess;
}

public boolean getAllowStaticMethodAccess() {
return allowStaticMethodAccess;
}

public boolean getAllowStaticFieldAccess() {
return allowStaticFieldAccess;
}

@Override
public Object setup(Map context, Object target, Member member, String propertyName) {
Object result = null;
Expand Down Expand Up @@ -102,20 +110,21 @@ public boolean isAccessible(Map context, Object target, Member member, String pr
return true;
}

if (!checkStaticMethodAccess(member)) {
if (!checkStaticMemberAccess(member)) {
LOG.warn("Access to static [{}] is blocked!", member);
return false;
}

final Class memberClass = member.getDeclaringClass();
final int memberModifiers = member.getModifiers();

if (isClassExcluded(memberClass)) {
LOG.warn("Declaring class of member type [{}] is excluded!", member);
return false;
}

// target can be null in case of accessing static fields, since OGNL 3.2.8
final Class targetClass = Modifier.isStatic(member.getModifiers()) ? memberClass : target.getClass();
final Class targetClass = Modifier.isStatic(memberModifiers) ? memberClass : target.getClass();

if (isPackageExcluded(targetClass.getPackage(), memberClass.getPackage())) {
LOG.warn("Package [{}] of target class [{}] of target [{}] or package [{}] of member [{}] are excluded!", targetClass.getPackage(), targetClass,
Expand All @@ -133,16 +142,31 @@ public boolean isAccessible(Map context, Object target, Member member, String pr
return false;
}

return Modifier.isPublic(member.getModifiers()) && isAcceptableProperty(propertyName);
return Modifier.isPublic(memberModifiers) && isAcceptableProperty(propertyName);
}

protected boolean checkStaticMethodAccess(Member member) {
/**
* Check access for static members
*
* Static non-field access result is a logical and of allowStaticMethodAccess and public.
* Static field access result is a logical and of allowStaticFieldAccess and public.
* Note: For non-static members, the result is always true.
*
* @param member
*
* @return
*/
protected boolean checkStaticMemberAccess(Member member) {
final int modifiers = member.getModifiers();
if (Modifier.isStatic(modifiers)) {
if (allowStaticMethodAccess) {
LOG.debug("Support for accessing static methods [member: {}] is deprecated!", member);
if (member instanceof Field) {
return allowStaticFieldAccess && Modifier.isPublic(modifiers);
} else {
if (allowStaticMethodAccess) {
LOG.debug("Support for accessing static methods [member: {}] is deprecated!", member);
}
return allowStaticMethodAccess && Modifier.isPublic(modifiers);
}
return allowStaticMethodAccess;
} else {
return true;
}
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/org/apache/struts2/StrutsConstants.java
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,9 @@ public final class StrutsConstants {
/** The name of the parameter to create when mapping an id (used by some action mappers) */
public static final String STRUTS_ID_PARAMETER_NAME = "struts.mapper.idParameterName";

/** The name of the parameter to determine whether static field access will be allowed in OGNL expressions or not */
public static final String STRUTS_ALLOW_STATIC_FIELD_ACCESS = "struts.ognl.allowStaticFieldAccess";

/** The name of the parameter to determine whether static method access will be allowed in OGNL expressions or not */
public static final String STRUTS_ALLOW_STATIC_METHOD_ACCESS = "struts.ognl.allowStaticMethodAccess";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,7 @@ private ValueStack createStubValueStack(final Map<String, Object> actual) {
ValueStack stack = new OgnlValueStack(
container.getInstance(XWorkConverter.class),
(CompoundRootAccessor) container.getInstance(PropertyAccessor.class, CompoundRoot.class.getName()),
container.getInstance(TextProvider.class, "system"), true) {
container.getInstance(TextProvider.class, "system"), true, true) {
@Override
public void setValue(String expr, Object value) {
actual.put(expr, value);
Expand Down
Loading

0 comments on commit 925eb62

Please sign in to comment.