WW-5352 Introducing the StrutsParameter annotation

[kusalk]

WW-5352

The final piece of the Struts 6.4 security overhaul. When this capability is enabled alongside the OGNL allowlist, security is greatly heightened with no additional complex configuration.

In addition to assisting OGNL allowlist configuration, the primary benefit of this new annotation is that it prevents inexperienced Struts developers from inadvertently introducing parameter injection points - as they often do not realise any public members on an Action class are parameter injectable. All parameter injection points will need to be explicitly annotated.

The OGNL allowlist capability can already be enabled using struts.allowlist.enable. It was introduced (#781) alongside the configuration options struts.allowlist.classes and struts.allowlist.packageNames using which you can define the allowlist. However, as one might have expected, the configuration of an exhaustive allowlist was a massively cumbersome task.

I later introduced #800 which was able to populate the allowlist on startup from all XML configuration provided to Struts. Whilst this significantly reduced the barrier to enabling the allowlist, with most applications only requiring that their model or DTO packages be allowlisted, I felt we could do better.

I had already planned to rework, strengthen and contribute an existing Struts interceptor capability used in Confluence - introduced in this PR as the @StrutsParameter annotation. As mentioned, its primary purpose is to prevent the unintentional introduction of parameter injection points. However, I now saw the opportunity to integrate this seamlessly with the OGNL allowlist capability.

Following a crucial contribution (#791) to make the SecurityMemberAccess policy more versatile, in this PR I was able to implement the capability to mutate the allowlist during runtime on a per-request basis.

The only code change required by applications is to annotate all Action class members intended for parameter injection with this new annotation. This can be scripted for large codebases by scanning for public members on classes which implement the Action interface.

This new annotation requirement is enabled using the option struts.parameters.requireAnnotations. I've added an additional option, struts.parameters.requireAnnotations.transitionMode, that applications can use to, as the name suggests, transition into this feature. When this mode is enabled, only 'nested' parameters, i.e. those represented by public getters on Action classes, will require annotations. This means public setters will still be exposed for parameter injection.

What's significant here is that in this mode, the auto-allowlisting capability is not degraded in any way, so it proves a useful transitioning option for applications that wish to enable the OGNL allowlist as soon as possible. These applications can then enable the full annotation capability once they have worked through annotating all their parameters and are ready.

Even with this annotation capability enabled, there will of course still be edge cases where applications will need to manually allowlist certain classes. One such edge case is when a POJO within a POJO needs to be parameter injected. The 'Showcase' application now requires zero manual allowlist configuration following the addition of the annotations.

I believe this to be a major step forward in Struts' security posture. Atlassian will be shipping both the OGNL allowlist and the StrutsParameter annotation capabilities (along with all other recently contributed security options) in Confluence DC 8.8.

I welcome all comments and feedback and encourage everyone to test both the allowlist and annotation capability with their own applications.

[kusalk]

I'm going to write some more acceptance tests before merging this but it's now ready for review :)

[lukaszlenart]

Astonish work, nice 👏

[lukaszlenart]

There is AnnotationUtils#isAnnotatedBy() - wouldn't be better to use it here or instead of getParameterAnnotation()?

[kusalk]

Doesn't look that utility method provides any additional value here - #getParameterAnnotation exists purely for overriding purposes. Applications can extend this default interceptor and add support for their own annotations or so on.

[kusalk]

Acceptance tests done :) Struts docs PR coming up

(but also I'm not sure it makes sense to include the showcase code for unit test coverage - I wrote that action specifically for acceptance tests so doesn't make a lot of sense to add unit tests for it IMO)

[sonarcloud]

Quality Gate failed

Failed conditions

71.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[lukaszlenart]

A few questions to clarify my doubts

[lukaszlenart]

Oh nice, I was going to ask about that, so this mechanism supports injecting incoming values directly into fields without a need to define setter, is that right? What about getter to fetch the value, is it still needed?

[kusalk]

So this PR doesn't add any new injection capabilities - OGNL has always been able to set public fields without setters. It has also never required a getter to set a value, only a setter.

A getter is only used when you want to access a nested setter (or public field). For example, the parameter name kusal.lukasz translates to getKusal(), then on the returned object, setLukasz(_).

What this PR does is use annotations to clearly mark the code path OGNL will take to perform parameter injection. The ParametersInterceptor is predicting the field or method OGNL will invoke, and if it is not annotated, it will be stripped out of the acceptable parameters map before OGNL attempts injection. In this way, when inspecting the source of an Action class, it is perfectly clear which methods and fields are exposed to the internet. We are essentially making OGNL more intuitive for developers - because right now, attackers seem to understand OGNL better than the developers using it! 😂

[lukaszlenart]

Ah.. I missed the public scope, I was hopping on some magic :D Anyway, thanks for the detailed explanation.

[lukaszlenart]

Wouldn't be better to use PROTOTYPE scope and do not use ThreadLocal instead?

[kusalk]

This bean was introduced as a way to mutate the allowlist read by SecurityMemberAccess - if it were prototype scope, ParametersInterceptor and SecurityMemberAccess would not share the same instance of ThreadAllowlist and thus this wouldn't be possible.

[lukaszlenart]

👍

[lukaszlenart]

Why do you use ThreadLocal? Wouldn't be better to use one global cache for the whole app?

[kusalk]

Yeah this is a good question - the reason I went with this design is because by isolating the allowlists on a per-request basis, it should make the application even more secure. Request A will not be able to access a class that might only be required by request B, and vice-versa. Note that we clear the allowlist at the end of every request. The less options attackers have to craft a payload, the better. Atlassian have seen some very creative payloads where our internal classes are chained together in unconventional ways.

There is also no performance benefit by making this a global cache as we don't currently cache the annotation lookups either. I don't expect the overhead of checking annotations per-request to be significant, but this is something we can reevaluate in the future.

[lukaszlenart]

And please fill the request in the Google program - you can donate money to charity if you want, your work requires recognition :)

[kusalk]

I appreciate that, I will look into it

And please do ask as many clarifying questions as you need :)

[lukaszlenart]

No more doubts, LGTM 👍 (I would give a bit of time to other reviewers)

[lukaszlenart]

@kusalk please merge this PR and I can start thinking about preparing a new release :)