[#825] Cap the batchRequest count per SOAP body and the request body size in the DSML gateway - #835
Conversation
createSocket() has been binding the new client socket to the target server address instead of connecting to it since OpenIdentityPlatform#279, so every plain or StartTLS connection made through org.opends.server.tools.LDAPConnection fails with "Address already in use" (server on the same host) or "Cannot assign requested address" (remote server). Affects the DSML gateway, stop-ds, manage-account and the other tools built on LDAPConnectionArgumentParser.
… on missing Content-Type performLDAPRequest() returns null for an abandon request, but doPost() dereferenced the result unconditionally, so a batch containing <abandonRequest/> ended in a NullPointerException; as the connection was closed after the loop instead of in a finally, one LDAP connection was leaked per request. messageFactory was only assigned when a SOAP 1.1 or SOAP 1.2 Content-Type header was present, and was then dereferenced both when parsing the request and when sending the response: a POST without Content-Type ended in a NullPointerException, and, when an error response had already been queued, in an empty HTTP 200 instead of that error. A missing or unsupported Content-Type is now answered with a malformedRequest batch response. Also log the failure instead of printing the stack trace when the response cannot be sent, and add regression tests for both defects.
Report the failure to send the response to the container log: the java.util.logging record was dropped, as connectToHost() resets the LogManager and turns the root logger off on every non-verbose connection. This needs super.init(config), without which getServletContext() throws. Drop the authzid of the previous batch request before setting the new one: the connection options are shared by the whole SOAP body and addSASLProperty() appends to the values of a key, so a second authRequest made SASL PLAIN reject a multi-valued authzid. Now that the connection is never reused, make it a loop local and remove the dead null check that guarded the reuse. Build the malformed Content-Type response with createXMLParsingErrorResponse(), like the other two malformed paths, so that the requestID is recovered; and keep reading the headers after a malformed Authorization one, so that the reply keeps the SOAP version of the request. Cover the SOAP 1.2 path, the per-batch-request connection and the authzid, let the fake LDAP endpoint serve several connections and fail the test on a server-side error, and pin the createSocket() regression of OpenIdentityPlatform#279 with a test in the module that owns it.
…uest of a SOAP body The connection options are built once per doPost() and shared by all the batch requests of the SOAP body, but the authzid was dropped only when the next batch request carried an authRequest of its own. A body whose first batch request asks for an authorization identity and whose second does not left the first authzid in the options, so the operations of the second one ran under an identity the request never asked for. It is gated on ldap.authzidtypeisid=true, which the shipped web.xml leaves at false, and still subject to the proxied-auth privileges of the server. The clearing now happens at the top of every iteration, before the authRequest is looked at. DSMLServletTestCase records the authorization identity of every SASL bind at the fake endpoint: the existing test now asserts the identities themselves instead of the mere absence of an error, and a new one pins the mixed body, where the second batch request must bind with no authzid at all. The four remaining Logger.getLogger(PKG_NAME) calls are replaced by getServletContext().log(), so the class has a single logging sink: they were dead for the reason already documented for the response path, which moves to the class javadoc.
…nd the request body size in the DSML gateway Every batchRequest element of a SOAP body is executed over its own connection and bind, and password verification is deliberately expensive, so a small POST holding many batchRequest elements amplified into many binds; the SOAP message is also parsed into memory, so an unbounded body was an unbounded allocation. Cap both: ldap.dsml.batchrequests.max (default 1, as DSMLv2 describes a single batchRequest per body) and ldap.dsml.request.maxsize (default 10485760 bytes). Excess elements and oversized bodies are rejected with a notAttempted errorResponse; the declared Content-Length is refused without reading the body, and chunked bodies are capped while streamed.
maximthomas
left a comment
There was a problem hiding this comment.
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
NumberFormatExceptionas control flow:DSMLServlet.java:306throws into its owncatchto reuse the message. Reads as an accident, and breaks if the catch ever narrows — parse, then checkparsed < 1separately.- Duplicated parsing:
positiveValue()does the same job as the inlineDEREF_ANYURI_MAXSIZEblock atDSMLServlet.java:235-247. Good chance to collapse it. printStackTrace()ininit(): the newServletExceptiongets stack-printed and rewrapped bycatch (Exception je). Message survives, but #811 routed everything else togetServletContext().log()and documented it in the class javadoc.- Param naming:
ldap.dsml.batchrequests.maxnext toldap.dsml.request.maxsizeandldap.dsml.dereference.anyuri.maxsize. Cosmetic, but a compat break after release — decide now. - No boundary test: a body of exactly
requestMaxSizeis accepted (checked), but nothing pins it, so an off-by-one incount()would go unnoticed. - Weak excess-cap assertion:
testExcessBatchRequestsAreRejectedByDefaultuses abandon requests, which emit no response element, so it only asserts op types. AsearchRequestin the first element would prove partial results still come back alongside thenotAttempted. - Follow-up issue: this closes bind amplification, not operation amplification. Under the new defaults a single POST still carries ~90k
<compareRequest>elements assertinguserPassword, 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.
Fixes #825.
Stacked on #811 — the first four commits belong to that PR (this fix builds on its rewrite of the
doPost()loop); merge #811 first, after which this PR reduces to the last commit.What
Since #811 every
batchRequestelement of a SOAP body is executed over its ownLDAPConnectionand 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 holdingNbatchRequestelements amplified intoNexpensive binds. The SOAP message is also parsed into memory with no size bound.How
Two new
web.xmlcontext-params next to the existingldap.*ones:ldap.dsml.batchrequests.max(default 1) — cap on the number ofbatchRequestelements accepted per SOAP body, enforced before the element is even schema-validated. DSMLv2 describes a singlebatchRequestper 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 anotAttemptederrorResponse.ldap.dsml.request.maxsize(default 10485760 bytes) — cap on the size of the request body. A declaredContent-Lengthover the cap is refused without reading the body at all; chunked bodies (and clients lying about their length) are capped while streamed by aCappedInputStreamwhich fails instead of truncating.A non-positive or non-numeric value for either parameter is rejected at servlet initialisation.
Tests
New
DSMLServletTestCasecases against the fake LDAP server: the excessbatchRequestis rejected with exactly one bind under the default cap, an oversized declared body and an oversized chunked body are both rejected without the directory server ever being contacted, and invalid parameter values failinit(). The multi-batch tests from #811 now raise the cap explicitly to 2.mvn -pl opendj-dsml-servlet test: 73 tests, 0 failures.