[
https://issues.apache.org/jira/browse/WW-5659?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5659:
------------------------------
Fix Version/s: 7.3.0
> 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
> Priority: Major
> Fix For: 7.3.0
>
>
> 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)
