bound the operation and invocation counts in a RequestFactory request

[alphacharlie-dev]

SimpleRequestProcessor iterates two attacker-supplied lists with no size limit.

Operations (:504):

List<OperationMessage> operations = req.getOperations();
...
state.getBeansForPayload(operations);           // :509 — one domain object per operation
for (AutoBean<? extends BaseProxy> bean : beans) {   // :512 — no size guard

Each entry costs a domain-object allocation, a reflective service.setProperty dispatch
(:550, :562) and a JSR-303 validation (:575-625).

Invocations (:428):

for (InvocationMessage invocation : invocations) {      // :436 — no size guard
  ...
  domainReturnValue = service.invoke(domainMethod, args.toArray());   // :463 — reflective
  paths.addAll(invocation.getPropertyRefs());           // :464 — uncapped accumulation

The deobfuscator constrains which operations resolve via its compile-time SHA1 whitelist, but not
how many times a valid one may appear — so an unauthenticated caller repeats a legitimate entry
thousands of times inside a few KB of JSON.

propertyRefs compound it. Resolver.expandPropertyRefs (:403-415) emits one entry per
dot-separated prefix with no depth bound, so a.b.c becomes three entries and deep references
amplify further.

The change

Cap operations and invocations at 1000, propertyRefs at 100 per invocation, and prefix expansion
at depth 32, rejecting anything larger with ReportableException.

These numbers are conservative starting points, not measured thresholds. They're the part of
this PR I'd most like a maintainer's opinion on — if RequestFactory clients legitimately batch more
than 1000 operations, the cap is wrong and should be raised or made configurable.

Testing

Full JRE suite: 25 suites / 2701 tests / 0 failures, identical to the main baseline.

Worth flagging: I ran only the JRE (*JreSuite) suites. RequestFactory has additional coverage
in the GWT-mode/browser suites that I could not execute in my environment, and of the patches I'm
proposing this is the one I'd least want taken on trust. If CI covers those, that's the check I'd
want to see.

Scope note

This addresses two findings I reported separately to the VRP (uncapped operations; uncapped
invocations + propertyRefs). They're the same root cause in the same file, so splitting the PR
would create two changes that conflict with each other. Happy to split if you'd rather review them
apart.

Provenance

Found during a security review of GWT 2.13.1, confirmed still present on main. Standalone
reproductions exist for both (bounded probes at N=5000 showing the loops are unguarded) plus tests
asserting the capped behaviour. I have not demonstrated an actual denial of service — the step from
"unbounded loop" to "service down" is analytical, not measured.

Reported to the Google OSS VRP, which declined on repository-tier grounds rather than the merits and
suggested bringing it here.

[zbynek]

Worth flagging: I ran only the JRE (*JreSuite) suites. RequestFactory has additional coverage
in the GWT-mode/browser suites that I could not execute in my environment, and of the patches I'm
proposing this is the one I'd least want taken on trust. If CI covers those, that's the check I'd
want to see.

You can run the full suite as GItHub action (full-check.yml) on your fork.

Also, could you please create an issue that escribes what actual problem are you trying to solve? Right now it's not formally required, but it's generally helpful (see #10387 for possible policy change).

[niloc132]

I think this probably makes sense, and likewise that the constraints are likely higher than anyone would reasonably want, but as you called out, N=5000 doesn't crash, so the limits are at a minimum 80% lower than they need to be (could be raised 500% with no loss in functionality), and as implemented are not configurable by users. That is: if an application does make aggressive use of these in a way your PR doesn't anticipate, we could be breaking their application. As a toolkit, we definitely don't want to hard code unchangable limits, double so "conservative" limits that have no grounding.

The "property refs per invocation" works out to mean something like "fields per returned proxy", and while 100 is quite high in the context of a UI, it is far from unheard of to have a persisted object containing more than 100 fields. That's then only one I would definitely raise the default value of out of the box.

Property refs are also suspect as a candidate for this in that they are effectively bounded by the size of the proxy object's fields - if the developer has defined 101 properties on a given proxy, who are we to say they are wrong? If they've defined only 20, why even accept a list of 100 top level properties?

propertyRefs compound it. Resolver.expandPropertyRefs (:403-415) emits one entry per
dot-separated prefix with no depth bound, so a.b.c becomes three entries and deep references
amplify further.

While this is true, it is again bounded by the size of the request - "a.b.c" is "a", "a.b", "a.b.c" and each is bounded within the scope of the others, in terms of the size of the response to build, and invalid property refs will have no real impact on the work done or the response.

One option could be allowing these to be defined at runtime via system properties, though that requires four different system properties set in the worse case. It might be nice to have a different way to define these limitations such that the application could has the potential to interact with them ("operation X is more expensive than Y", "this user is an admin, so bulk operations are not unexpected", etc).

Finally, the RF SimpleRequestProcessor, by its name, isn't intended to be all things to all people, and might not be the appropriate layer for DoS defense. For example, if the limit is 1000 invocation and 100 property refs per invocation, an attacker could historically send a larger request, but with these limitations can just send the same request 10x. There is no amplification aspect to this as far as I can tell (actually the opposite, since two invocations can return the same instance and their property ref sets will be merged), so we're bounded by upload size linearly - so with a modest degree of overhead, and attacker can 10x or 1000x the attack still by sending more requests rather than larger requests.

RequestFactoryServlet does use the system property "gwt.rpc.dumpPayload" for debugging purposes, but other properties are read from the servlet config init params. A scheme where SimpleRequestProcess and the like can be configured to support varying sizes and servlets (or non-servlet http interfaces) can then pass in their own desired configuration might make sense?

[niloc132]

This should be defined as a constant, defined in a consistent location/format as the others (and configurable).

[zbynek]

"a.b.c" is "a", "a.b", "a.b.c" and each is bounded within the scope of the others,

I guess that's the only part of this PR where the avoided work is provably bigger than linear in terms of input size (?) since

System.out.println(expandPropertyRefs(Set.of("a.".repeat(90_000) + "b")).size());

will OOM for default JVM settings, though the input string is ~200KiB.

[niloc132]

"a.b.c" is "a", "a.b", "a.b.c" and each is bounded within the scope of the others,

I guess that's the only part of this PR where the avoided work is provably bigger than linear in terms of input size (?) since

System.out.println(expandPropertyRefs(Set.of("a.".repeat(90_000) + "b")).size());

will OOM for default JVM settings, though the input string is ~200KiB.

True - I was thinking here in the scope of actually doing the JPA calls, 90k serial sql calls seems bad, but probably either is garbage and stops after a few steps, or resolves the same object when a loop is found and work terminates. Perhaps the better fix (besides "something more than 5 and less than 90k") is to lazily enumerate the list rather than produce it up front..?