[
https://issues.apache.org/jira/browse/WW-5659?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5659:
------------------------------
Description:
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|https://github.com/apache/struts/pull/1815] - reported the
problem; approach not adopted
was:
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
> 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|https://github.com/apache/struts/pull/1815] - reported the
> problem; approach not adopted
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
