Lukasz Lenart created WW-5659:
---------------------------------
Summary: WithLazyParams resolves dynamic interceptor params onto
the shared interceptor instance
Key: WW-5659
URL: https://issues.apache.org/jira/browse/WW-5659
Project: Struts 2
Issue Type: Bug
Components: Core Interceptors
Reporter: Lukasz Lenart
h3. Problem
{{WithLazyParams.LazyParamInjector#injectParams}} resolves {{${...}}}
interceptor
parameters per request and writes the resolved values straight onto the
interceptor
instance:
{code:java}
// WithLazyParams.java:78-84
public Interceptor injectParams(Interceptor interceptor, Map<String, String>
params, ActionContext invocationContext) {
for (Map.Entry<String, String> entry : params.entrySet()) {
Object paramValue = textParser.evaluate(new char[]{'$'},
entry.getValue(), valueEvaluator, TextParser.DEFAULT_LOOP_COUNT);
ognlUtil.setProperty(entry.getKey(), paramValue, interceptor,
invocationContext.getContextMap());
}
return interceptor;
}
{code}
That interceptor is a singleton. It is built once at configuration-parse time
({{InterceptorBuilder}}:73-74 and :175-177) and reused for every request. When
an action
references an interceptor stack without overriding params,
{{InterceptorBuilder}}:79
(_result.addAll(stackConfig.getInterceptors())_) hands out the same
{{InterceptorMapping}}
objects, so the same interceptor instance is shared across actions as well.
The result is that request-scoped state is written to process-wide state with no
synchronisation.
h3. Consequence for file upload validation
{{ActionFileUploadInterceptor}} is currently the only {{WithLazyParams}}
implementer. Its
resolved policy lands in plain instance fields
({{AbstractFileUploadInterceptor}}:62-64, written at :85, :94, :103) and is
read back
later by {{acceptFile}} ({{AbstractFileUploadInterceptor}}:134, :141, :148).
Per request the order is:
* {{DefaultActionInvocation}}:269 - resolve and write the shared fields
* {{DefaultActionInvocation}}:275 - {{intercept()}} -> {{acceptFile()}} reads
the shared fields
Nothing guards the interval between the two. Two concurrent requests that
resolve
different policies can therefore have their {{allowedTypes}},
{{allowedExtensions}} and
{{maximumSize}} cross over, and a request can be validated against another
request's
upload policy.
This only affects applications that opt into the dynamic {{${...}}} form added
in
WW-5585. {{struts-default.xml}}:60 declares {{actionFileUpload}} with no
params, so the
default configuration writes nothing and is unaffected. Static (non-expression)
params
resolve to the same value on every request and are likewise unaffected.
Note that {{actionFileUpload}} sits at position 15 of {{defaultStack}}, ahead of
{{staticParams}} (19), {{actionMappingParams}} (20) and {{params}} (21), so no
request
parameter has been bound to the action at resolution time. The expression's
*value* is
not attacker-supplied. Which policy gets selected can still depend on
request-derived
state populated by {{prepare}} (11), {{scopedModelDriven}} (13) or
{{modelDriven}} (14) -
the shipped showcase does exactly that in
{{DynamicFileUploadAction#prepareUpload}}:136-139.
h3. Secondary defect
{{DefaultActionInvocation}}:262-267 calls {{params.putAll(...)}} on the map
returned by
{{InterceptorMapping#getParams}}:60-62, which is the live shared map. That is an
unsynchronised write to a shared {{HashMap}} on every request.
h3. Proposed fix
Fix the {{WithLazyParams}} contract rather than patching the one implementer,
so that no
future implementer can reintroduce the same bug. Resolved parameters should be
written
into a per-invocation object supplied by the interceptor and passed back to it,
leaving
the interceptor singleton immutable after {{init()}}:
{code:java}
public interface WithLazyParams<P> {
P newLazyParams();
String intercept(ActionInvocation invocation, P lazyParams) throws
Exception;
}
{code}
Targeting a clean interface change in the next minor release. A design document
will
follow.
h3. References
* WW-5585 - introduced dynamic parameter evaluation for file upload validation
* GitHub PR #1815 - reported the problem; approach not adopted
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
