[#825] Cap the batchRequest count per SOAP body and the request body size in the DSML gateway

[vharseko]

Fixes #825.

Was stacked on #811; now that it is merged the branch is rebased onto master and holds only this PR's own two commits: the fix and the review round.

What

Since #811 every batchRequest element of a SOAP body is executed over its own LDAPConnection and therefore its own bind. Password verification is deliberately expensive (PBKDF2, bcrypt and the other salted schemes are tuned to be slow), and a failed bind costs the same as a successful one, so a single small POST holding N batchRequest elements amplified into N expensive binds. The SOAP message is also parsed into memory with no size bound.

How

Two new web.xml context-params next to the existing ldap.* ones:

• ldap.dsml.batchrequests.max (default 1) — cap on the number of elements accepted per SOAP body, enforced before the element is even schema-validated. DSMLv2 describes a single batchRequest per SOAP body, and before [#809] Fix DSML gateway NPE on abandonRequest and on missing Content-Type #811 the second and later elements never worked anyway (they were silently skipped), so the default breaks no working client. Excess elements are rejected with a notAttempted errorResponse, and the results of the elements under the cap still reach the client next to the error.
• ldap.dsml.request.maxsize (default 10485760 bytes) — cap on the size of the request body. A declared Content-Length over the cap is refused without reading the body at all: the check runs before the malformed-Content-Type fallback which SAX-parses the body to recover the requestID, and it never adds a second error next to one already reported. Chunked bodies (and clients lying about their length) are capped while streamed by a CappedInputStream which fails instead of truncating.

A non-positive or non-numeric value for either parameter is rejected at servlet initialisation. Neither rejection message echoes the configured cap to the unauthenticated client.

The review round also closed CodeQL's java/input-resource-leak on the request stream with try-with-resources (a whitespace-heavy diff — read DSMLServlet.java with git diff -w), routed init() errors to getServletContext().log() instead of printStackTrace(), and collapsed the duplicated context-param parsing into positiveValue().

Tests

New DSMLServletTestCase cases against the fake LDAP server: the excess batchRequest is rejected with exactly one bind under the default cap while the search results of the first element still come back next to the error, an oversized declared body and an oversized chunked body are both rejected without the directory server ever being contacted, an oversized body never doubles an already-reported credentials error nor gets SAX-parsed when the Content-Type is missing, a body of exactly the cap is accepted, and invalid parameter values fail init(). The multi-batch tests from #811 raise the cap explicitly to 2. mvn -pl opendj-dsml-servlet test: 76 test invocations, 0 failures.

[maximthomas]

The design is right: enforce the count cap before unmarshalling, fail rather than truncate on the size cap, keep defaults in code so a stale web.xml is still protected. Verified locally — mvn -o -pl opendj-dsml-servlet test → 73/73, and on master the if (connection == null) guard means the 2nd+ batchRequest is already silently skipped, so the default of 1 breaks no working client.

One blocking defect. Note this can't merge before #811 anyway.

Content-Length check is unguarded and misplaced (blocking)

opendj-dsml-servlet/src/main/java/org/opends/dsml/protocol/DSMLServlet.java:565. Every other pre-parse error path guards on batchResponses.isEmpty(); this one doesn't, and it runs after the messageFactory == null fallback. Two consequences, both confirmed by instrumenting the test harness.

Bad credentials + oversized declared length yields two errors in one reply:

<batchResponse>
  <errorResponse type="authenticationFailed"><message>...Unable to retrieve credentials.</message></errorResponse>
  <errorResponse type="notAttempted"><message>...larger than ldap.dsml.request.maxsize=10485760 bytes...</message></errorResponse>
</batchResponse>

No Content-Type + oversized declared length: two errors again, and the reply carries requestID="1" — proof that createXMLParsingErrorResponse() SAX-parsed the whole body. That is the work the check exists to avoid, and it contradicts "refused without reading the body at all" in the PR description. Capped at 10 MiB so not exploitable, but the guarantee doesn't hold.

Move the check above the messageFactory == null block — still after the header loop, so the client's SOAP version is preserved — and guard it:

if ( batchResponses.isEmpty() && req.getContentLengthLong() > requestMaxSize ) {
  // Reject before anything reads the stream: the malformed Content-Type path
  // below SAX-parses the body to recover the requestID.
  batchResponses.add(createErrorResponse(objFactory, requestSizeExceeded()));
}

Worth a test per case — they slipped through because nothing covers them.

Rejection message doesn't match what is counted (minor)

DSMLServlet.java:601. batchRequestCount counts every SOAPElement child, not only batchRequest elements — correct for the DoS goal, since it runs before unmarshalling. But the message then claims "N batchRequest element(s)" for a body mixing element types. Drop the word:

+ " element(s): the remaining elements were not attempted."

Cap value disclosed pre-authentication (minor)

DSMLServlet.java:816. requestSizeExceeded() hands an unauthenticated client the configured ldap.dsml.request.maxsize verbatim. Config disclosure for no benefit the client can act on:

LocalizableMessage.raw("The request body is larger than the configured maximum: not attempted.")

Nits

• NumberFormatException as control flow: DSMLServlet.java:306 throws into its own catch to reuse the message. Reads as an accident, and breaks if the catch ever narrows — parse, then check parsed < 1 separately.
• Duplicated parsing: positiveValue() does the same job as the inline DEREF_ANYURI_MAXSIZE block at DSMLServlet.java:235-247. Good chance to collapse it.
• printStackTrace() in init(): the new ServletException gets stack-printed and rewrapped by catch (Exception je). Message survives, but #811 routed everything else to getServletContext().log() and documented it in the class javadoc.
• Param naming: ldap.dsml.batchrequests.max next to ldap.dsml.request.maxsize and ldap.dsml.dereference.anyuri.maxsize. Cosmetic, but a compat break after release — decide now.
• No boundary test: a body of exactly requestMaxSize is accepted (checked), but nothing pins it, so an off-by-one in count() would go unnoticed.
• Weak excess-cap assertion: testExcessBatchRequestsAreRejectedByDefault uses abandon requests, which emit no response element, so it only asserts op types. A searchRequest in the first element would prove partial results still come back alongside the notAttempted.
• Follow-up issue: this closes bind amplification, not operation amplification. Under the new defaults a single POST still carries ~90k <compareRequest> elements asserting userPassword, each triggering the same slow password-scheme comparison, all on one bind. Out of scope here — a per-batch operation cap is a real decision against DSMLv2 batch semantics and wants its own issue.

[vharseko]

Thanks — all addressed in 102f1ec (the branch is rebased onto master now that #811 is merged, so the PR is down to its own two commits).

Blocking — declared-size check: moved above the messageFactory == null fallback (still after the header loop, so the client's SOAP version is preserved) and guarded on batchResponses.isEmpty(). Both regressions are pinned by new tests: testOversizedDeclaredBodyDoesNotDoubleACredentialsError (single authenticationFailed, no notAttempted) and testOversizedDeclaredBodyWithoutContentTypeIsNotParsed (single notAttempted, no malformedRequest, and no requestID in the reply — the proof the SAX pass never ran).

Count message: reworded to "The SOAP body holds more elements than the configured maximum: the remaining elements were not attempted." While at it I also dropped the configured value from this message — it disclosed ldap.dsml.batchrequests.max exactly the way the size message disclosed its cap.

Cap disclosure: requestSizeExceeded() now returns your suggested wording verbatim; the param names stay in code comments/javadoc for maintainers.

Nits:

• positiveValue() parses first and checks < 1 separately — no more NumberFormatException as control flow. A two-arg overload took over the inline DEREF_ANYURI_MAXSIZE block (setMaxUriContentLength() still enforces positivity on its own).
• init() rethrows ServletException untouched and routes everything else to getServletContext().log(), per the [#809] Fix DSML gateway NPE on abandonRequest and on missing Content-Type #811 convention.
• Boundary test added (testBodyOfExactlyTheMaximumSizeIsAccepted), and testExcessBatchRequestsAreRejectedByDefault now runs a searchRequest in the first element, asserting the searchResponse arrives next to the notAttempted (the fake server answers searches with an empty success result).
• Param naming: kept ldap.dsml.batchrequests.max — the .maxsize suffix of its siblings means "maximum size in bytes", while this one caps a count, and .max keeps that distinction readable. Say the word if you'd rather rename before release.
• Follow-up: agreed on the operation-amplification issue (a per-batch operation cap is a real decision against DSMLv2 batch semantics) — opened as DSML gateway: cap the number of operations per batchRequest, a compare on userPassword costs a password verification #843.

Also closed CodeQL's java/input-resource-leak on the request stream with try-with-resources around the rest of doPost() — the DSMLServlet.java diff is mostly that indent shift; git diff -w shows the ~70 real lines.

mvn -o -pl opendj-dsml-servlet test: 76 test invocations, 0 failures.

[maximthomas]

There were no changes since the last approval