github-advanced-security[bot] commented on code in PR #1872:
URL: https://github.com/apache/struts/pull/1872#discussion_r3878340751
##########
core/src/main/java/org/apache/struts2/interceptor/parameter/StrutsParameterAuthorizer.java:
##########
@@ -115,28 +115,102 @@
long paramDepth = parameterName.codePoints().mapToObj(c -> (char)
c).filter(NESTING_CHARS::contains).count();
- // ModelDriven exemption: only exempt when the action explicitly
implements ModelDriven
- // and the target is its model object. This prevents non-ModelDriven
root objects
- // (e.g. JSONInterceptor's configurable rootObject) from bypassing
annotation checks.
- if (target != action && action instanceof ModelDriven) {
- LOG.debug("ModelDriven target detected (action implements
ModelDriven), exempting from @StrutsParameter annotation requirement");
- return true;
+ int nestingIndex = indexOfAny(parameterName, NESTING_CHARS_STR);
+ String rootProperty = nestingIndex == -1 ? parameterName :
parameterName.substring(0, nestingIndex);
+ if (rootProperty.isEmpty()) {
+ LOG.debug("Parameter [{}] begins with a nesting character, so it
names no root property to authorize; rejecting",
+ parameterName);
+ return false;
}
+ String normalisedRootProperty =
Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1);
- // Transition mode: depth-0 (non-nested) parameters are exempt
+ // Transition mode: depth-0 (non-nested) parameters are exempt.
Checked before the ModelDriven
+ // exemption so that it also covers a ModelDriven action's own
members, which would otherwise
+ // have no migration path once the exemption is scoped to the model.
if (requireAnnotationsTransitionMode && paramDepth == 0) {
LOG.debug("Annotation transition mode enabled, exempting
non-nested parameter [{}] from @StrutsParameter annotation requirement",
parameterName);
return true;
}
- int nestingIndex = indexOfAny(parameterName, NESTING_CHARS_STR);
- String rootProperty = nestingIndex == -1 ? parameterName :
parameterName.substring(0, nestingIndex);
- String normalisedRootProperty =
Character.toLowerCase(rootProperty.charAt(0)) + rootProperty.substring(1);
+ // ModelDriven exemption: only exempt when the action explicitly
implements ModelDriven
+ // and the target is its model object. This prevents non-ModelDriven
root objects
+ // (e.g. JSONInterceptor's configurable rootObject) from bypassing
annotation checks.
+ if (target != action && action instanceof ModelDriven) {
+ return isAuthorizedOnModelDrivenAction(normalisedRootProperty,
target, action, paramDepth);
+ }
return hasValidAnnotatedMember(normalisedRootProperty, target,
paramDepth);
}
+ /**
+ * Decides authorization for a {@link ModelDriven} action, whose model is
on top of the value stack.
+ * <p>
+ * Returning an object from {@code getModel()} declares that object to be
request surface, so anything the
+ * model itself can take is exempt from the {@link StrutsParameter}
requirement. The exemption stops there:
+ * OGNL resolves the parameter name against the whole stack, which also
holds the action, so a property
+ * declared on the action is still subject to the annotation requirement.
Without that distinction a
+ * ModelDriven action would silently expose its own members.
+ * <p>
+ * A property declared on neither is allowed, since it cannot be reaching
a member of the action - typically
+ * it is bound by a custom OGNL property accessor on the model, such as a
Map-backed model.
+ */
+ protected boolean isAuthorizedOnModelDrivenAction(String rootProperty,
Object model, Object action, long paramDepth) {
+ if (declaresProperty(model, rootProperty, paramDepth)) {
+ LOG.debug("Property [{}] belongs to the ModelDriven model,
exempting from @StrutsParameter annotation requirement",
+ rootProperty);
+ return true;
+ }
+ if (!declaresProperty(action, rootProperty, paramDepth)) {
+ LOG.debug("Property [{}] is declared on neither the model nor the
action, exempting from @StrutsParameter annotation requirement",
+ rootProperty);
+ return true;
+ }
+ LOG.debug("Property [{}] is declared on the ModelDriven action itself,
applying the @StrutsParameter annotation requirement",
+ rootProperty);
+ return hasValidAnnotatedMember(rootProperty, action, paramDepth);
+ }
+
+ /**
+ * Whether {@code target} can itself take {@code property} at this depth -
as a bean property whose relevant
+ * accessor exists, the setter for a depth-0 parameter and the getter for
a nested one, or as a public instance
+ * field. Any {@link StrutsParameter} annotation is irrelevant here; this
asks only what the object can absorb.
+ * <p>
+ * It has to be bindability rather than the name alone, because OGNL walks
the stack until an object actually
+ * accepts the assignment. A model which merely names the property without
being able to take it - a getter-only
+ * property under a depth-0 parameter, say - does not absorb that
parameter: OGNL moves on to the action, and an
+ * exemption granted on the name alone would hand over the action's own
member, which is the very thing this
+ * scoping exists to prevent. Inherited public fields count for the same
reason, that OGNL can set them.
+ */
+ protected boolean declaresProperty(Object target, String property, long
paramDepth) {
+ BeanInfo beanInfo = getBeanInfo(target);
+ if (beanInfo != null &&
Arrays.stream(beanInfo.getPropertyDescriptors())
+ .filter(desc -> desc.getName().equals(property))
+ .anyMatch(desc -> (paramDepth == 0 ? desc.getWriteMethod() :
desc.getReadMethod()) != null)) {
+ return true;
+ }
+ return declaresBindablePublicField(target, property, paramDepth);
+ }
+
+ /**
+ * Whether {@code target} exposes {@code property} as a public instance
field that this parameter could bind
+ * through. {@link Class#getField} rather than {@code getDeclaredField},
since an inherited public field is just
+ * as settable as a declared one. Static fields are not per-instance
request surface, and a final field cannot
+ * take a depth-0 assignment, so neither counts as absorbing the parameter.
+ */
+ protected boolean declaresBindablePublicField(Object target, String
property, long paramDepth) {
+ Field field;
+ try {
+ field = ultimateClass(target).getField(property);
Review Comment:
## SonarCloud / Reflection should not be vulnerable to injection attacks
<!--SONAR_ISSUE_KEY:AaBHAzt2KNzgUaIETn2l-->Change this code to not construct
class or method names directly from user-controlled data. <p>See more on <a
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AaBHAzt2KNzgUaIETn2l&open=AaBHAzt2KNzgUaIETn2l&pullRequest=1872";>SonarQube
Cloud</a></p>
[Show more
details](https://github.com/apache/struts/security/code-scanning/1438)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
