WW-5468 Exempt ModelDriven Actions from @StrutsParameter requirement #1072
Conversation
| if (action instanceof ModelDriven<?> && !ActionContext.getContext().getValueStack().peek().equals(action)) { | ||
| LOG.debug("Model driven Action detected, exempting from @StrutsParameter annotation requirement and OGNL allowlisting model type"); | ||
| // (Exempted by annotation on com.opensymphony.xwork2.ModelDriven#getModel) | ||
| return hasValidAnnotatedMember("model", action, paramDepth + 1); |
There was a problem hiding this comment.
This conditional block and the added annotation on the ModelDriven interface comprise the core fix
| if (paramDepth >= 1) { | ||
| allowlistClass(relevantMethod.getReturnType()); | ||
| allowlistClass(propDesc.getPropertyType()); |
There was a problem hiding this comment.
Minor fix here to ensure the specific model type is allowlisted rather than the type declared in the interface (Object.class) - thanks @lukaszlenart
| @@ -44,7 +44,6 @@ public String getFoo() { | |||
| /** | |||
| * @return the model to be pushed onto the ValueStack after the Action itself | |||
| */ | |||
| @StrutsParameter(depth = 2) | |||
There was a problem hiding this comment.
I removed all these annotations on ModelDriven Action model getters as they are no longer required (not that they were being detected correctly previously)
There was a problem hiding this comment.
Replaced tabs with spaces to fix the janky formatting
| @@ -343,6 +360,14 @@ public Map<String, Pojo> getPublicPojoMapDepthTwo() { | |||
| } | |||
| } | |||
|
|
|||
| class Pojo { | |||
| static class ModelAction implements ModelDriven<Pojo> { | |||
There was a problem hiding this comment.
There is one more case where ModelDriven<Object> is used and then getModel() can return conditional model, either a List<Pojo> or Pojo itself. See that example
There was a problem hiding this comment.
Ah interesting, let me test this
There was a problem hiding this comment.
I think in such cases, the app developer should manually OGNL allowlist any required types, I'm not confident of a secure way to auto allowlist in this case
There was a problem hiding this comment.
Yeah, it can be tricky, but having such a note should be enough 👍
kusalk
force-pushed
the
fix/WW-5468-modeldriven-2
branch
from
9a5080d to
f6c17e9
Compare
|
WW-5468 Exempt ModelDriven Actions from @StrutsParameter requirement
WW-5468
The
@StrutsParameterrequirement was designed to protect against arbitrary getters and setters on the Action class from being invoked by users and/or attackers. However, if an Action is using a dedicated model object alongside theModelDrivenInterceptor(which ensures the Action is not on the root of the value stack) much of this risk is mitigated. I suggest we exempt this specific scenario from requiring the@StrutsParameterannotation.Closes #1071