Skip to content

verify jws and jwe bodies sent with DELETE and other bodyless methods - #3670

Open
rootvector2 wants to merge 3 commits into
apache:mainfrom
rootvector2:jose-bodyless-method-verify
Open

rootvector2 wants to merge 3 commits into
apache:mainfrom
rootvector2:jose-bodyless-method-verify

Conversation

@rootvector2

Copy link
Copy Markdown
Contributor

JwsContainerRequestFilter, JwsJsonContainerRequestFilter, JweContainerRequestFilter and JweJsonContainerRequestFilter return without reading the entity for DELETE, GET, HEAD, OPTIONS and TRACE, and the four client response filters do the same for responses to DELETE, HEAD and OPTIONS, but JAX-RS still reads a body on those methods, so a DELETE with an unsigned or wrongly signed body reaches the resource unverified while a correctly signed one arrives as the raw JWS string (found reviewing the jose-jaxrs filters, the same gap #3632 closed for http-signature); the filters now skip only when hasEntity() is false.

The JOSE request filters returned before reading the entity for DELETE,
GET, HEAD, OPTIONS and TRACE, and the client response filters did the
same for responses to DELETE, HEAD and OPTIONS, so a body sent with
those methods was never verified or decrypted. Skip them only when
there is no entity.

@coheigea coheigea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings, most severe first:

  1. Possible way to skip verification (needs checking per server). Whether a request body gets verified now depends on hasEntity(). On the server side, that method returns false as soon as the client’s own Content-Length header says 0, without looking at the stream. A request with Content-Length: 0 plus Transfer-Encoding: chunked and an unsigned body could skip verification, if the servlet container reads the chunked body but keeps the stale header. Whether this works depends on the container. The filters should check the stream itself, not the header.
  2. Subclasses that override isMethodWithNoContent change behaviour without warning. It’s protected and used to mean “never process this method”. Now it means “skip only if the body is also empty”. Overrides that relied on the old meaning will start failing with 400 or an exception.
  3. The same skip condition is copied into 8 classes. It would be safer as one helper in the abstract base classes. The method check is also mostly redundant now, because every filter already returns early on empty content.
  4. Test gaps. The client test fakes hasEntity() instead of using the real ClientResponseContextImpl, so the header shortcut is never exercised. Four of the changed filters have no tests: the JWE client response filter and the JWS JSON, JWE JSON client and JWE JSON server filters.
  5. Small extra cost on every GET. Those requests used to return immediately. Now each one buffers and probes the request stream through hasEntity().

isMethodWithNoContent now returns false by default and lives in the four
abstract base classes, so every filter reads the entity whatever the
method and relies on the existing empty-content return. Subclasses that
override it keep its old meaning: the method is skipped entirely.

The client tests use the real ClientRequestContextImpl and
ClientResponseContextImpl, every one of the eight filters has a test,
and a DELETE body sent with Content-Length: 0 is still verified.
@rootvector2

Copy link
Copy Markdown
Contributor Author

Pushed a rework that takes hasEntity() out of the decision entirely.

  1. The filters no longer consult the headers: they read the stream and return on the existing empty-content check, so a Content-Length: 0 plus chunked body is verified like any other. JwsContainerRequestFilterTest.testZeroContentLengthDoesNotSkipVerification covers that case. The opt-in checkEmptyStream path still calls hasEntity() as before; it's off by default and predates this change, so I left it alone.
  2. isMethodWithNoContent keeps its old meaning for subclasses that override it (the method is skipped entirely). It just returns false by default now, so existing overrides aren't reinterpreted.
  3. The hook lives in the four abstract base classes and the per-filter copies plus the new condition are gone, so the filter diffs are now pure deletions. The default method skip was redundant with the empty-content return, as you said.
  4. The client tests build the real ClientRequestContextImpl and ClientResponseContextImpl (cxf-rt-rs-client added as a test dependency), and all eight filters have tests; 12 of the 24 fail on main.
  5. A bodyless GET now costs one read() returning -1 instead of returning immediately. I don't see a way around that: knowing whether a body is there means touching the stream, since the headers can't be trusted per point 1. The hasEntity() probe it replaces did the same read plus buffering.

The JOSE systests in systests/rs-security still pass (118 tests).

@coheigea coheigea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New findings, most severe first:

  1. NPE when there is no entity stream. GET, HEAD and OPTIONS requests now reach IOUtils.readStringFromStream / readBytesFromStream, which reject a null stream. ContainerRequestContextImpl.getEntityStream() can return null: for example, a GET with no body over the local transport never sets an input stream on the server message. That request now fails with 500, where before it was skipped. A null check before reading, in all 8 filters, fixes it.
  2. super.isMethodWithNoContent() now returns false. A subclass that calls super to extend the old method list (for example, to also skip PATCH) quietly stops skipping GET/HEAD/OPTIONS/DELETE.
  3. The opt-in checkEmptyStream=true setting still skips on Content-Length: 0. It goes through hasEntity(), which trusts the header, for any method including POST. This was already true before the PR and isn’t tested.
  4. No test covers a missing stream. The test helpers always supply one, so finding 1 isn’t caught.

…he jose filters

ContainerRequestContext.getEntityStream() and ClientResponseContext
.getEntityStream() can return null, for example for a GET over the local
transport or a response without an entity, and IOUtils rejects a null
stream. All eight filters now return when there is no stream.

checkEmptyStream went through hasEntity(), which returns false on a
Content-Length: 0 header without looking at the stream. The filters
already read the stream and return on empty content, so the hasEntity()
shortcut is dropped and the setting no longer has any effect.

The isMethodWithNoContent Javadoc now spells out the old default for
subclasses that extend it through super.
@rootvector2

Copy link
Copy Markdown
Contributor Author

Pushed 8b5911b.

  1. All eight filters now return when getEntityStream() is null, before anything reaches IOUtils. That covers a GET with no stream on the server and a client response with no entity, where ClientResponseContextImpl.getEntityStream() returns null too.
  2. I've kept the default at false. The old list returned by super is exactly the DELETE/GET skip this PR removes, so I don't see a way to keep it for super callers without the default filter skipping those bodies again. A subclass doing super.isMethodWithNoContent(m) || "PATCH".equals(m) still returns early on bodyless GET/HEAD/OPTIONS/DELETE (missing or empty stream); the only difference it sees is that a body sent with those methods now gets verified. The Javadoc now spells out the old lists, so an override that wants them skipped entirely can list them itself.
  3. Fixed rather than just tested. The filters already read the stream and return on empty content, so the isCheckEmptyStream() && !hasEntity() shortcut only ever added trust in Content-Length. It's gone; setCheckEmptyStream stays for config compatibility and is documented as having no effect. testCheckEmptyStreamDoesNotTrustZeroContentLength covers a wrongly signed body with Content-Length: 0 on both the server and client filter.
  4. Each of the eight filters has a missing-stream test now (testGetWithoutEntityStreamIsAccepted / testResponseWithoutEntityIsAccepted). All 10 new tests fail on the previous commit. The module's 34 tests pass with checkstyle and PMD, and the 118 JOSE systests in systests/rs-security still pass.

@coheigea coheigea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. A few remaining points:

1. An empty entity stream is closed and left on the message (server filters)

IOUtils.readStringFromStream / readBytesFromStream close the stream they read. When the content is empty, the filters return without putting a fresh stream back. Every body-less GET or DELETE now closes the underlying servlet input stream, which these filters never touched before. Anything that reads the body afterwards (a later filter, a ReaderInterceptor, an optional entity or form parameter) gets a closed stream. On some containers that throws ("Stream closed").

Suggested fix, in JwsContainerRequestFilter, JwsJsonContainerRequestFilter, JweContainerRequestFilter and JweJsonContainerRequestFilter:

if (StringUtils.isEmpty(content)) {   // or encryptedContent.length == 0
    context.setEntityStream(new ByteArrayInputStream(new byte[0]));
    return;
}

2. Same issue in the client response filters

The 4 client filters now read and close the stream for empty DELETE/HEAD/OPTIONS responses too, and don't put a replacement back. A later response.readEntity(...) or another ClientResponseFilter can then hit an IOException where it used to get an empty entity. The same one-line reset fixes it.

3. checkEmptyStream is now a no-op

The getter and setter remain in the 4 abstract base classes, but the flag isn't read anywhere, and the only notice is in the Javadoc. Please add @Deprecated to the accessors so anyone setting it in Spring/Blueprint config gets some signal.

4. Test gap

None of the new tests checks that the entity stream can still be read after an empty body goes through a filter. Asserting that IOUtils.readStringFromStream(message.getContent(InputStream.class)) returns "" without throwing after filtering an empty GET/DELETE would cover points 1 and 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants