[
https://issues.apache.org/jira/browse/WW-5758?focusedWorklogId=1045813&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1045813
]
ASF GitHub Bot logged work on WW-5758:
--------------------------------------
Author: ASF GitHub Bot
Created on: 05/Oct/26 19:20
Start Date: 05/Oct/26 19:20
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #1991:
URL: https://github.com/apache/struts/pull/1991#discussion_r4187837147
##########
core/src/test/java/org/apache/struts2/interceptor/AliasInterceptorTest.java:
##########
@@ -132,6 +138,42 @@ private SimpleAction executeWithBothNamesSubmitted(String
actionName) throws Exc
return (SimpleAction) proxy.getAction();
}
+ // An alias target that is not annotated with @StrutsParameter must not be
set when the source value
Review Comment:
The test name and its assertions already say what this comment says. Could
it go?
Please also add a case for the stack-to-stack branch with
`struts.chaining.requireAnnotations=true` (unannotated target stays untouched)
next to the existing one with it off.
##########
core/src/main/java/org/apache/struts2/interceptor/AliasInterceptor.java:
##########
@@ -189,17 +198,31 @@ public void setAliasesKey(String aliasesKey) {
continue;
}
Evaluated value = new Evaluated(stack.findValue(name));
+ boolean fromRawRequestParameter = false;
if (!value.isDefined()) {
- // workaround
+ // workaround: name did not resolve on the stack (e.g.
no earlier action in a
Review Comment:
Could this go back to a one-line comment? Something like `// name did not
resolve on the stack, fall back to the request parameter`. The comparison with
`ChainingInterceptor` belongs in the PR description, where it already is.
##########
core/src/main/java/org/apache/struts2/interceptor/AliasInterceptor.java:
##########
@@ -189,17 +198,31 @@ public void setAliasesKey(String aliasesKey) {
continue;
}
Evaluated value = new Evaluated(stack.findValue(name));
+ boolean fromRawRequestParameter = false;
if (!value.isDefined()) {
- // workaround
+ // workaround: name did not resolve on the stack (e.g.
no earlier action in a
+ // chain set it), so fall back to the raw HTTP
parameter of the same name - this
+ // is the one path through which attacker-controlled,
never-yet-authorized data
+ // reaches newStack.setValue below, so it is the one
path @StrutsParameter has to
+ // gate. A name that DID resolve on the stack names
something already on the
+ // stack - typically state an earlier,
properly-authorized bind already produced -
+ // and copying that between properties is the same
category of operation
+ // ChainingInterceptor performs without requiring
annotations by default.
HttpParameters contextParameters =
ActionContext.getContext().getParameters();
if (null != contextParameters) {
Parameter param = contextParameters.get(name);
if (param.isDefined()) {
value = new Evaluated(param.getValue());
+ fromRawRequestParameter = true;
}
}
}
+ if (fromRawRequestParameter &&
!parameterAuthorizer.isAuthorized(alias, authorizationTarget, action)) {
Review Comment:
This is where I'd expect the second branch: when the value came from the
stack, check it the same way, gated on `struts.chaining.requireAnnotations`
(injected like `ChainingInterceptor.setRequireAnnotations`, `required =
false`). With the chaining flag off (the default), behaviour stays exactly as
in this PR.
##########
core/src/test/java/org/apache/struts2/interceptor/AliasInterceptorTest.java:
##########
@@ -132,6 +138,42 @@ private SimpleAction executeWithBothNamesSubmitted(String
actionName) throws Exc
return (SimpleAction) proxy.getAction();
}
+ // An alias target that is not annotated with @StrutsParameter must not be
set when the source value
+ // comes from AliasInterceptor's documented fallback to the raw HTTP
parameter of the same name (used
+ // when the source name does not resolve on the stack) - that is the one
path through which
+ // attacker-controlled, never-yet-authorized data reaches the target. An
annotated target in the same
+ // aliases map, read through the exact same fallback, must still bind
normally. A THIRD, unannotated
+ // target whose source name resolves directly on the stack (no HTTP
parameter involved at all, the
+ // same category of copy ChainingInterceptor performs without an
annotation requirement by default)
+ // must also still bind normally - the check must not reach that path.
+ public void testUnannotatedAliasTargetIsRejected() throws Exception {
+ Map<String, Object> httpParams = new HashMap<>();
+ httpParams.put("rawSourceAnnotated", "allowed-value");
+ httpParams.put("rawSourceUnannotated", "PWNED_VIA_ALIAS");
Review Comment:
Please use a plain value here, e.g. `"value-from-request"`, to match the
other fixtures in this class.
##########
core/src/test/java/org/apache/struts2/interceptor/AliasInterceptorTest.java:
##########
@@ -132,6 +138,42 @@ private SimpleAction executeWithBothNamesSubmitted(String
actionName) throws Exc
return (SimpleAction) proxy.getAction();
}
+ // An alias target that is not annotated with @StrutsParameter must not be
set when the source value
+ // comes from AliasInterceptor's documented fallback to the raw HTTP
parameter of the same name (used
+ // when the source name does not resolve on the stack) - that is the one
path through which
+ // attacker-controlled, never-yet-authorized data reaches the target. An
annotated target in the same
+ // aliases map, read through the exact same fallback, must still bind
normally. A THIRD, unannotated
+ // target whose source name resolves directly on the stack (no HTTP
parameter involved at all, the
+ // same category of copy ChainingInterceptor performs without an
annotation requirement by default)
+ // must also still bind normally - the check must not reach that path.
+ public void testUnannotatedAliasTargetIsRejected() throws Exception {
+ Map<String, Object> httpParams = new HashMap<>();
+ httpParams.put("rawSourceAnnotated", "allowed-value");
+ httpParams.put("rawSourceUnannotated", "PWNED_VIA_ALIAS");
+ ActionContext extraContext =
ActionContext.of().withParameters(HttpParameters.create(httpParams).build());
+
+ XmlConfigurationProvider provider = new
StrutsXmlConfigurationProvider("struts-alias-authorization.xml");
+ container.inject(provider);
+ // loadConfigurationProviders tears down and rebuilds the whole
configuration on every call, so the
Review Comment:
This one is worth keeping, since the ordering trap isn't obvious. A single
line is enough, though.
##########
core/src/main/java/org/apache/struts2/interceptor/AliasInterceptor.java:
##########
@@ -189,17 +198,31 @@ public void setAliasesKey(String aliasesKey) {
continue;
}
Evaluated value = new Evaluated(stack.findValue(name));
+ boolean fromRawRequestParameter = false;
if (!value.isDefined()) {
- // workaround
+ // workaround: name did not resolve on the stack (e.g.
no earlier action in a
+ // chain set it), so fall back to the raw HTTP
parameter of the same name - this
+ // is the one path through which attacker-controlled,
never-yet-authorized data
+ // reaches newStack.setValue below, so it is the one
path @StrutsParameter has to
+ // gate. A name that DID resolve on the stack names
something already on the
+ // stack - typically state an earlier,
properly-authorized bind already produced -
+ // and copying that between properties is the same
category of operation
+ // ChainingInterceptor performs without requiring
annotations by default.
HttpParameters contextParameters =
ActionContext.getContext().getParameters();
if (null != contextParameters) {
Parameter param = contextParameters.get(name);
if (param.isDefined()) {
value = new Evaluated(param.getValue());
+ fromRawRequestParameter = true;
}
}
}
+ if (fromRawRequestParameter &&
!parameterAuthorizer.isAuthorized(alias, authorizationTarget, action)) {
+ LOG.debug("Alias target [{}] rejected by
@StrutsParameter authorization on target [{}]",
Review Comment:
`LOG.warn` here, please, and use the wording `ChainingInterceptor` uses,
e.g. `Alias: property [{}] not set on [{}] because it is not annotated with
@StrutsParameter`.
##########
core/src/test/resources/struts-alias-authorization.xml:
##########
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+-->
+<!DOCTYPE struts PUBLIC
+ "-//Apache Software Foundation//DTD Struts Configuration 6.0//EN"
+ "https://struts.apache.org/dtds/struts-6.0.dtd";>
+<struts>
+ <include file="xwork-test-default.xml"/>
+ <package name="alias-authorization" extends="xwork-test-default">
+
+ <!
Issue Time Tracking
-------------------
Worklog Id: (was: 1045813)
Time Spent: 20m (was: 10m)
> Enforce @StrutsParameter authorization in AliasInterceptor's raw-parameter
> fallback
> -----------------------------------------------------------------------------------
>
> Key: WW-5758
> URL: https://issues.apache.org/jira/browse/WW-5758
> Project: Struts 2
> Issue Type: Bug
> Components: Core
> Affects Versions: 7.0.0, 7.3.0, 7.4.0
> Reporter: Gouri Sankar A
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> AliasInterceptor copies a value from one named source to a developer-chosen
> target property, falling
> back to the raw HTTP request parameter of the source name when the source
> does not resolve on the stack
> (its own documented behavior, shown in its own javadoc example). That
> fallback value was never checked
> against @StrutsParameter before being written to the target, unlike every
> other binding path in the
> framework: ParametersInterceptor, ChainingInterceptor, CookieInterceptor, and
> the REST/JSON plugins (my
> own earlier finding, WW-5642, already merged) all call into the shared
> ParameterAuthorizer before
> binding. AliasInterceptor never did. alias is in defaultStack, so it runs on
> every request to any
> default-configured action.
> Tested on main at a235259 (7.5.0-SNAPSHOT), embedded Tomcat 10.1.34, stock
> struts-default, an action with
> one @StrutsParameter-annotated property and one not. Binding the unannotated
> property directly
> (?secret=DIRECT_ATTEMPT) is correctly rejected, stays untouched. The same
> property, reached by
> configuring aliases to map a raw HTTP parameter name onto it and sending that
> parameter instead
> (?rawParam=PWNED_VIA_ALIAS), is not rejected and takes the attacker supplied
> value directly.
> The fix is deliberately narrower than checking every alias target.
> AliasInterceptor has two different
> value sources: a name that resolves directly on the stack, which names state
> an earlier, properly
> authorized bind already produced such as an earlier action in a chain, and
> the raw HTTP parameter
> fallback, which is the only path that carries never authorized external
> input. Checking every target
> unconditionally would have also blocked the first, legitimate category. I
> built that broader version
> first, live tested it, found it silently broke a same request stack to stack
> copy with no HTTP parameter
> involved at all, and narrowed the check to apply only when the value came
> from the fallback, matching
> exactly what ChainingInterceptor already exempts by default for the same
> underlying reason.
> Affects 7.0.0 through 7.4.0, verified directly on all three released versions
> by reading the code at each
> tag. requireAnnotations has been the default since the very first 7.0 release
> and AliasInterceptor has
> never checked it. Does not affect 6.x: requireAnnotations is not set in 6.x's
> default.properties at all,
> confirmed directly on 6.12.0, so there is no default on control for any
> interceptor to bypass on that
> line.
> This is the same bug class as WW-5642, already merged: a secondary binding
> path that does not enforce
> @StrutsParameter the way ParametersInterceptor does.
> I have a patch with a regression test ready and will open a PR referencing
> this ticket.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
