[
https://issues.apache.org/jira/browse/WW-5712?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5712:
------------------------------
Description:
{{ParameterAuthorizingModule.updateBuilder}} wraps the properties returned by
{{BeanDeserializerBuilder.getProperties()}} with
{{AuthorizingSettableBeanProperty}}, but it never consults
{{BeanDeserializerBuilder.getAnySetter()}}. The {{SettableAnyProperty}} is
therefore left unwrapped, so unknown keys dispatched to a Jackson
{{@JsonAnySetter}} are set without passing through the {{@StrutsParameter}}
authorization context — while an ordinary unannotated setter on the same class,
in the same request body, is correctly rejected.
All three any-setter forms are affected (method, field and {{@JsonCreator}}
parameter). Both {{JacksonJsonHandler}} and {{JacksonXmlHandler}} register the
module, so JSON and XML bodies behave the same. {{@JsonUnwrapped}} is not
affected, and an any-setter beneath a rejected parent is already skipped
through {{skipChildren()}}.
The obvious fix does not work: path-based authorization resolves a parameter's
root name against declared members of the target, so {{pathFor(key)}} matches
nothing for a dynamic key by construction. Routing any-setter keys through the
existing check would reject every key and break every existing
{{@JsonAnySetter}} user. Consent has to be expressed at the sink rather than at
the key.
Proposed shape:
* A new annotation member, {{@StrutsParameter(allowDynamicKeys = true)}},
marking one any-setter as permitted to receive arbitrary keys. A plain
{{@StrutsParameter}} is deliberately not sufficient — arbitrary-key access
should have to be typed out, not granted by accident.
* Enforcement behind a plugin-local constant,
{{struts.rest.anySetter.requireAnnotations}}, defaulting to {{false}}, so
nothing changes for existing applications until they opt in. Same shape as
{{struts.chaining.requireAnnotations}}; the default can be revisited in a major
release.
* {{depth()}} keeps its current meaning, counted beneath the dynamic key. The
default {{depth = 0}} accepts a scalar value under any key and rejects a nested
object under it; {{depth = 1}} permits one level of nesting. That is the rule
an annotated {{Map}} property already follows.
* {{@Target}} stays {{METHOD, FIELD}}. A {{@JsonCreator}}-parameter any-setter
has nowhere to carry the marker and is rejected with a warning when enforcement
is on. Narrowing that rare form now is preferable to advertising an annotation
placement the framework does not honour elsewhere, and {{@Target}} can be
widened later but never narrowed.
* The wrapper mirrors {{AuthorizingSettableBeanProperty}}: delegate untouched
when the authorization context is inactive; on rejection log a warning, mark
the context redacted and call {{skipChildren()}} so nested graphs are never
constructed.
Test coverage should include default-off compatibility, the method and field
forms, missing and explicit consent, depth 0/1/2, creator parameters,
unauthorized parents, {{@JsonUnwrapped}}, both the JSON and XML handlers, and
context cleanup.
Documentation follow-up: the {{@StrutsParameter}} page states that the JSON and
REST plugins authorize each property during deserialization "so that
unauthorized fields are never set". That is not accurate as written — correct
the wording and document the any-setter behaviour explicitly.
Reported by Darren Carreras (GitHub: carrerasdarren-cell), who has a patch
implementing the above and will open the pull request.
Independently discovered and reported by Satoshi Ogawa of Mitsui Bussan Secure
Directions, Inc., via JPCERT/CC.
was:
{{ParameterAuthorizingModule.updateBuilder}} wraps the properties returned by
{{BeanDeserializerBuilder.getProperties()}} with
{{AuthorizingSettableBeanProperty}}, but it never consults
{{BeanDeserializerBuilder.getAnySetter()}}. The {{SettableAnyProperty}} is
therefore left unwrapped, so unknown keys dispatched to a Jackson
{{@JsonAnySetter}} are set without passing through the {{@StrutsParameter}}
authorization context — while an ordinary unannotated setter on the same class,
in the same request body, is correctly rejected.
All three any-setter forms are affected (method, field and {{@JsonCreator}}
parameter). Both {{JacksonJsonHandler}} and {{JacksonXmlHandler}} register the
module, so JSON and XML bodies behave the same. {{@JsonUnwrapped}} is not
affected, and an any-setter beneath a rejected parent is already skipped
through {{skipChildren()}}.
The obvious fix does not work: path-based authorization resolves a parameter's
root name against declared members of the target, so {{pathFor(key)}} matches
nothing for a dynamic key by construction. Routing any-setter keys through the
existing check would reject every key and break every existing
{{@JsonAnySetter}} user. Consent has to be expressed at the sink rather than at
the key.
Proposed shape:
* A new annotation member, {{@StrutsParameter(allowDynamicKeys = true)}},
marking one any-setter as permitted to receive arbitrary keys. A plain
{{@StrutsParameter}} is deliberately not sufficient — arbitrary-key access
should have to be typed out, not granted by accident.
* Enforcement behind a plugin-local constant,
{{struts.rest.anySetter.requireAnnotations}}, defaulting to {{false}}, so
nothing changes for existing applications until they opt in. Same shape as
{{struts.chaining.requireAnnotations}}; the default can be revisited in a major
release.
* {{depth()}} keeps its current meaning, counted beneath the dynamic key. The
default {{depth = 0}} accepts a scalar value under any key and rejects a nested
object under it; {{depth = 1}} permits one level of nesting. That is the rule
an annotated {{Map}} property already follows.
* {{@Target}} stays {{METHOD, FIELD}}. A {{@JsonCreator}}-parameter any-setter
has nowhere to carry the marker and is rejected with a warning when enforcement
is on. Narrowing that rare form now is preferable to advertising an annotation
placement the framework does not honour elsewhere, and {{@Target}} can be
widened later but never narrowed.
* The wrapper mirrors {{AuthorizingSettableBeanProperty}}: delegate untouched
when the authorization context is inactive; on rejection log a warning, mark
the context redacted and call {{skipChildren()}} so nested graphs are never
constructed.
Test coverage should include default-off compatibility, the method and field
forms, missing and explicit consent, depth 0/1/2, creator parameters,
unauthorized parents, {{@JsonUnwrapped}}, both the JSON and XML handlers, and
context cleanup.
Documentation follow-up: the {{@StrutsParameter}} page states that the JSON and
REST plugins authorize each property during deserialization "so that
unauthorized fields are never set". That is not accurate as written — correct
the wording and document the any-setter behaviour explicitly.
Reported by Darren Carreras (GitHub: carrerasdarren-cell), who has a patch
implementing the above and will open the pull request.
> ParameterAuthorizingModule does not wrap the Jackson any-setter in the REST
> plugin
> ----------------------------------------------------------------------------------
>
> Key: WW-5712
> URL: https://issues.apache.org/jira/browse/WW-5712
> Project: Struts 2
> Issue Type: Bug
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 1h
> Remaining Estimate: 0h
>
> {{ParameterAuthorizingModule.updateBuilder}} wraps the properties returned by
> {{BeanDeserializerBuilder.getProperties()}} with
> {{AuthorizingSettableBeanProperty}}, but it never consults
> {{BeanDeserializerBuilder.getAnySetter()}}. The {{SettableAnyProperty}} is
> therefore left unwrapped, so unknown keys dispatched to a Jackson
> {{@JsonAnySetter}} are set without passing through the {{@StrutsParameter}}
> authorization context — while an ordinary unannotated setter on the same
> class, in the same request body, is correctly rejected.
> All three any-setter forms are affected (method, field and {{@JsonCreator}}
> parameter). Both {{JacksonJsonHandler}} and {{JacksonXmlHandler}} register
> the module, so JSON and XML bodies behave the same. {{@JsonUnwrapped}} is not
> affected, and an any-setter beneath a rejected parent is already skipped
> through {{skipChildren()}}.
> The obvious fix does not work: path-based authorization resolves a
> parameter's root name against declared members of the target, so
> {{pathFor(key)}} matches nothing for a dynamic key by construction. Routing
> any-setter keys through the existing check would reject every key and break
> every existing {{@JsonAnySetter}} user. Consent has to be expressed at the
> sink rather than at the key.
> Proposed shape:
> * A new annotation member, {{@StrutsParameter(allowDynamicKeys = true)}},
> marking one any-setter as permitted to receive arbitrary keys. A plain
> {{@StrutsParameter}} is deliberately not sufficient — arbitrary-key access
> should have to be typed out, not granted by accident.
> * Enforcement behind a plugin-local constant,
> {{struts.rest.anySetter.requireAnnotations}}, defaulting to {{false}}, so
> nothing changes for existing applications until they opt in. Same shape as
> {{struts.chaining.requireAnnotations}}; the default can be revisited in a
> major release.
> * {{depth()}} keeps its current meaning, counted beneath the dynamic key. The
> default {{depth = 0}} accepts a scalar value under any key and rejects a
> nested object under it; {{depth = 1}} permits one level of nesting. That is
> the rule an annotated {{Map}} property already follows.
> * {{@Target}} stays {{METHOD, FIELD}}. A {{@JsonCreator}}-parameter
> any-setter has nowhere to carry the marker and is rejected with a warning
> when enforcement is on. Narrowing that rare form now is preferable to
> advertising an annotation placement the framework does not honour elsewhere,
> and {{@Target}} can be widened later but never narrowed.
> * The wrapper mirrors {{AuthorizingSettableBeanProperty}}: delegate untouched
> when the authorization context is inactive; on rejection log a warning, mark
> the context redacted and call {{skipChildren()}} so nested graphs are never
> constructed.
> Test coverage should include default-off compatibility, the method and field
> forms, missing and explicit consent, depth 0/1/2, creator parameters,
> unauthorized parents, {{@JsonUnwrapped}}, both the JSON and XML handlers, and
> context cleanup.
> Documentation follow-up: the {{@StrutsParameter}} page states that the JSON
> and REST plugins authorize each property during deserialization "so that
> unauthorized fields are never set". That is not accurate as written — correct
> the wording and document the any-setter behaviour explicitly.
> Reported by Darren Carreras (GitHub: carrerasdarren-cell), who has a patch
> implementing the above and will open the pull request.
> Independently discovered and reported by Satoshi Ogawa of Mitsui Bussan
> Secure Directions, Inc., via JPCERT/CC.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
