spring-security: Thread-unsafe usage of HttpServletResponse corrupting jetty HttpFields state in reactive ResponseBodyEmitter handlers

Affects: 5.2.5.RELEASE

We have a Spring Web MVC + jetty application using SSE for notifications. We recently converted our SSE controller from returning a SseEmitter to returning a Flux<ServerSentEvent<?>>, whereupon we started seeing sporadic NullPointerExceptions from jetty Response.setHeader calls for something on the order of 0.02% of requests for these SSE endpoints:

2020-05-12 11:15:53.837|WARN|1|qtp453671855-27|||org.eclipse.jetty.server.HttpChannel|MAIN||||/...
java.lang.NullPointerException: null
	at org.eclipse.jetty.http.HttpFields.put(HttpFields.java:630) ~[jetty-http-9.4.20.v20190813.jar!/:9.4.20.v20190813]
	at org.eclipse.jetty.http.HttpFields.put(HttpFields.java:658) ~[jetty-http-9.4.20.v20190813.jar!/:9.4.20.v20190813]
	at org.eclipse.jetty.server.Response.setHeader(Response.java:582) ~[jetty-server-9.4.20.v20190813.jar!/:9.4.20.v20190813]
	at javax.servlet.http.HttpServletResponseWrapper.setHeader(HttpServletResponseWrapper.java:203) ~[javax.servlet-api-3.1.0.jar!/:3.1.0]
	at org.springframework.security.web.firewall.FirewalledResponse.setHeader(FirewalledResponse.java:49) ~[spring-security-web-5.2.2.RELEASE.jar!/:5.2.2.RELEASE]
	at org.springframework.security.web.header.writers.frameoptions.XFrameOptionsHeaderWriter.writeHeaders(XFrameOptionsHeaderWriter.java:99) ~[spring-security-web-5.2.2.RELEASE.jar!/:5.2.2.RELEASE]
   ....

Looking at the jetty HttpFields implementation, these NPEs seem to be caused by a corrupted internal _fields state containing nulls.

With jetty 9.4.20 these exceptions cause the connection to be aborted, but with newer version of jetty (at least 9.4.27) these corrupted HttpFields put the jetty HttpChannel into a busyloop that crashes the entire server: https://github.com/eclipse/jetty.project/issues/4860

Investigating the underlying NPE issue together with the jetty maintainers we were unable to find any direct calls to the jetty APIs that would introduce null values in the HttpFields state, but looking at the implementation of the jetty HttpFields structure used in Response.setHeaders/addHeaders etc, it is suspectible to internal state corruption if called concurrently from multiple threads, and is documented as non-threadsafe:

* <p>This class is not synchronized as it is expected that modifications will only be performed by a
* single thread.

This kind of corruption caused by concurrent multi-threaded accesses also matches up with our observation of very rarely occuring state corruption/crashes. And indeed, using jdb to trace calls to these header-related methods for a single HTTP request, I can see calls from multiple different threads!

Some of the calls are from the jetty server thread, via the servlet filter chain, ~which is the correct usage~:

  [1] org.eclipse.jetty.server.Response.addHeader (Response.java:627)
  [2] javax.servlet.http.HttpServletResponseWrapper.addHeader (HttpServletResponseWrapper.java:212)
  [3] org.springframework.security.web.firewall.FirewalledResponse.addHeader (FirewalledResponse.java:55)
  [4] javax.servlet.http.HttpServletResponseWrapper.addHeader (HttpServletResponseWrapper.java:212)
  [5] org.springframework.security.web.util.OnCommittedResponseWrapper.addHeader (OnCommittedResponseWrapper.java:63)
  [6] org.springframework.web.cors.DefaultCorsProcessor.processRequest (DefaultCorsProcessor.java:63)
  [7] org.springframework.web.servlet.handler.AbstractHandlerMapping$CorsInterceptor.preHandle (AbstractHandlerMapping.java:582)
  [8] org.springframework.web.servlet.HandlerExecutionChain.applyPreHandle (HandlerExecutionChain.java:141)
  [9] org.springframework.web.servlet.DispatcherServlet.doDispatch (DispatcherServlet.java:1,035)
  [10] org.springframework.web.servlet.DispatcherServlet.doService (DispatcherServlet.java:943)
  [11] org.springframework.web.servlet.FrameworkServlet.processRequest (FrameworkServlet.java:1,006)
  [12] org.springframework.web.servlet.FrameworkServlet.doGet (FrameworkServlet.java:898)
  [13] javax.servlet.http.HttpServlet.service (HttpServlet.java:687)
  [14] org.springframework.web.servlet.FrameworkServlet.service (FrameworkServlet.java:883)
  [15] javax.servlet.http.HttpServlet.service (HttpServlet.java:790)
  [16] org.eclipse.jetty.servlet.ServletHolder.handle (ServletHolder.java:755)
  [17] org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter (ServletHandler.java:1,617)
  [18] com.example.common.audit.MDCLogFilter.doFilter (MDCLogFilter.java:67)
  [19] org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter (ServletHandler.java:1,604)
  [20] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:320)
  [21] org.springframework.security.web.access.intercept.FilterSecurityInterceptor.invoke (FilterSecurityInterceptor.java:126)
  [22] org.springframework.security.web.access.intercept.FilterSecurityInterceptor.doFilter (FilterSecurityInterceptor.java:90)
  [23] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [24] org.springframework.security.web.access.ExceptionTranslationFilter.doFilter (ExceptionTranslationFilter.java:118)
  [25] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [26] org.springframework.security.web.session.SessionManagementFilter.doFilter (SessionManagementFilter.java:137)
  [27] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [28] org.springframework.security.web.authentication.AnonymousAuthenticationFilter.doFilter (AnonymousAuthenticationFilter.java:111)
  [29] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [30] org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter.doFilter (SecurityContextHolderAwareRequestFilter.java:158)
  [31] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [32] com.example.common.auth.WhitelistIpAuthenticationFilter.doFilter (WhitelistIpAuthenticationFilter.java:37)
  [33] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [34] org.springframework.security.web.authentication.www.BasicAuthenticationFilter.doFilterInternal (BasicAuthenticationFilter.java:204)
  [35] org.springframework.web.filter.OncePerRequestFilter.doFilter (OncePerRequestFilter.java:119)
  [36] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [37] com.example.common.auth.StatelessAuthenticationFilter.doFilter (StatelessAuthenticationFilter.java:49)
  [38] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [39] org.springframework.security.web.header.HeaderWriterFilter.doHeadersAfter (HeaderWriterFilter.java:92)
  [40] org.springframework.security.web.header.HeaderWriterFilter.doFilterInternal (HeaderWriterFilter.java:77)
  [41] org.springframework.web.filter.OncePerRequestFilter.doFilter (OncePerRequestFilter.java:119)
  [42] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [43] org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter (SecurityContextPersistenceFilter.java:105)
  [44] org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter (FilterChainProxy.java:334)
  [45] org.springframework.security.web.FilterChainProxy.doFilterInternal (FilterChainProxy.java:215)
  [46] org.springframework.security.web.FilterChainProxy.doFilter (FilterChainProxy.java:178)
  [47] org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate (DelegatingFilterProxy.java:358)
  [48] org.springframework.web.filter.DelegatingFilterProxy.doFilter (DelegatingFilterProxy.java:271)
  [49] org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter (ServletHandler.java:1,604)
  [50] org.springframework.web.filter.RequestContextFilter.doFilterInternal (RequestContextFilter.java:100)
  [51] org.springframework.web.filter.OncePerRequestFilter.doFilter (OncePerRequestFilter.java:119)
  [52] org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter (ServletHandler.java:1,604)
  [53] org.springframework.web.filter.FormContentFilter.doFilterInternal (FormContentFilter.java:93)
  [54] org.springframework.web.filter.OncePerRequestFilter.doFilter (OncePerRequestFilter.java:119)
  [55] org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter (ServletHandler.java:1,604)
  [56] org.springframework.boot.actuate.metrics.web.servlet.WebMvcMetricsFilter.doFilterInternal (WebMvcMetricsFilter.java:109)
  [57] org.springframework.web.filter.OncePerRequestFilter.doFilter (OncePerRequestFilter.java:119)
  [58] org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter (ServletHandler.java:1,604)
  [59] org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal (CharacterEncodingFilter.java:201)
  [60] org.springframework.web.filter.OncePerRequestFilter.doFilter (OncePerRequestFilter.java:119)
  [61] org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter (ServletHandler.java:1,604)
  [62] org.eclipse.jetty.servlet.ServletHandler.doHandle (ServletHandler.java:545)
  [63] org.eclipse.jetty.server.handler.ScopedHandler.handle (ScopedHandler.java:143)
  [64] org.eclipse.jetty.security.SecurityHandler.handle (SecurityHandler.java:590)
  [65] org.eclipse.jetty.server.handler.HandlerWrapper.handle (HandlerWrapper.java:127)
  [66] org.eclipse.jetty.server.handler.ScopedHandler.nextHandle (ScopedHandler.java:235)
  [67] org.eclipse.jetty.server.session.SessionHandler.doHandle (SessionHandler.java:1,610)
  [68] org.eclipse.jetty.server.handler.ScopedHandler.nextHandle (ScopedHandler.java:233)
  [69] org.eclipse.jetty.server.handler.ContextHandler.doHandle (ContextHandler.java:1,300)
  [70] org.eclipse.jetty.server.handler.ScopedHandler.nextScope (ScopedHandler.java:188)
  [71] org.eclipse.jetty.servlet.ServletHandler.doScope (ServletHandler.java:485)
  [72] org.eclipse.jetty.server.session.SessionHandler.doScope (SessionHandler.java:1,580)
  [73] org.eclipse.jetty.server.handler.ScopedHandler.nextScope (ScopedHandler.java:186)
  [74] org.eclipse.jetty.server.handler.ContextHandler.doScope (ContextHandler.java:1,215)
  [75] org.eclipse.jetty.server.handler.ScopedHandler.handle (ScopedHandler.java:141)
  [76] org.eclipse.jetty.server.handler.StatisticsHandler.handle (StatisticsHandler.java:173)
  [77] org.eclipse.jetty.server.handler.HandlerWrapper.handle (HandlerWrapper.java:127)
  [78] org.eclipse.jetty.server.Server.handle (Server.java:500)
  [79] org.eclipse.jetty.server.HttpChannel.lambda$handle$1 (HttpChannel.java:383)
  [80] org.eclipse.jetty.server.HttpChannel$$Lambda$1167.1412565883.dispatch (null)
  [81] org.eclipse.jetty.server.HttpChannel.dispatch (HttpChannel.java:547)
  [82] org.eclipse.jetty.server.HttpChannel.handle (HttpChannel.java:375)
  [83] org.eclipse.jetty.server.HttpConnection.onFillable (HttpConnection.java:273)
  [84] org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded (AbstractConnection.java:311)
  [85] org.eclipse.jetty.io.FillInterest.fillable (FillInterest.java:103)
  [86] org.eclipse.jetty.io.ChannelEndPoint$2.run (ChannelEndPoint.java:117)
  [87] org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask (EatWhatYouKill.java:336)
  [88] org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce (EatWhatYouKill.java:313)
  [89] org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce (EatWhatYouKill.java:171)
  [90] org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.produce (EatWhatYouKill.java:135)
  [91] org.eclipse.jetty.io.ManagedSelector$$Lambda$1140.431966204.run (null)
  [92] org.eclipse.jetty.util.thread.QueuedThreadPool.runJob (QueuedThreadPool.java:806)
  [93] org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run (QueuedThreadPool.java:938)
  [94] java.lang.Thread.run (Thread.java:834)

However, there are also calls to addHeader from the ThreadPoolExecutor used in the spring framework ResponseBodyEmitterReturnValueHandler implementation, via the org.springframework.web.servlet.mvc.method.annotation.ReactiveTypeHandler$SseEmitterSubscriber:

  [1] org.eclipse.jetty.server.Response.addHeader (Response.java:627)
  [2] javax.servlet.http.HttpServletResponseWrapper.addHeader (HttpServletResponseWrapper.java:212)
  [3] org.springframework.security.web.firewall.FirewalledResponse.addHeader (FirewalledResponse.java:55)
  [4] javax.servlet.http.HttpServletResponseWrapper.addHeader (HttpServletResponseWrapper.java:212)
  [5] org.springframework.security.web.util.OnCommittedResponseWrapper.addHeader (OnCommittedResponseWrapper.java:63)
  [6] org.springframework.http.server.ServletServerHttpResponse.lambda$writeHeaders$0 (ServletServerHttpResponse.java:104)
  [7] org.springframework.http.server.ServletServerHttpResponse$$Lambda$1241.488706756.accept (null)
  [8] java.util.Map.forEach (Map.java:661)
  [9] org.springframework.http.server.ServletServerHttpResponse.writeHeaders (ServletServerHttpResponse.java:102)
  [10] org.springframework.http.server.ServletServerHttpResponse.getBody (ServletServerHttpResponse.java:83)
  [11] org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitterReturnValueHandler$StreamingServletServerHttpResponse.getBody (ResponseBodyEmitterReturnValueHandler.java:278)
  [12] org.springframework.http.converter.StringHttpMessageConverter.writeInternal (StringHttpMessageConverter.java:122)
  [13] org.springframework.http.converter.StringHttpMessageConverter.writeInternal (StringHttpMessageConverter.java:44)
  [14] org.springframework.http.converter.AbstractHttpMessageConverter.write (AbstractHttpMessageConverter.java:227)
  [15] org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitterReturnValueHandler$HttpMessageConvertingHandler.sendInternal (ResponseBodyEmitterReturnValueHandler.java:210)
  [16] org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitterReturnValueHandler$HttpMessageConvertingHandler.send (ResponseBodyEmitterReturnValueHandler.java:203)
  [17] org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitter.sendInternal (ResponseBodyEmitter.java:189)
  [18] org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitter.send (ResponseBodyEmitter.java:183)
  [19] org.springframework.web.servlet.mvc.method.annotation.SseEmitter.send (SseEmitter.java:126)
  [20] org.springframework.web.servlet.mvc.method.annotation.ReactiveTypeHandler$SseEmitterSubscriber.send (ReactiveTypeHandler.java:365)
  [21] org.springframework.web.servlet.mvc.method.annotation.ReactiveTypeHandler$AbstractEmitterSubscriber.run (ReactiveTypeHandler.java:308)
  [22] java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1,128)
  [23] java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:628)
  [24] java.lang.Thread.run (Thread.java:834)

~Looking at the code, the ResponseBodyEmitter created by the ResponseBodyEmitterReturnValueHandler/ReactiveTypeHandler should not be writing any HTTP response headers outside of the extendResponse called in the servlet thread, but the ReactiveTypeHandler subscription’s SseEmitter.send call in the TaskExecutor thread is also invoking ServletServerHttpResponse.writeHeaders via the HttpMessageConvertingHandler.sendInternal.~ EDIT: see comment below on spring-security HeaderWriterFilter behavior instead

Complete error log and jdb traces atttached:

EDIT: edited stack traces to censor custom package names; use the first task-9 thread stack from the jdb traces as the example

About this issue

Original URL
State: open
Created 4 years ago
Reactions: 3
Comments: 17 (5 by maintainers)

Commits related to this issue

fix sporadic spring-security issue 9175: https://github.com/spring-projects/spring-security/issues/9175#issuecomment-661879599 — committed to mschweyen/sapl-server by mschweyen 8 months ago
fix sporadic spring-security issue 9175: https://github.com/spring-projects/spring-security/issues/9175#issuecomment-661879599 — committed to mschweyen/sapl-server by mschweyen 8 months ago

Most upvoted comments

@rstoyanchev It looks like @SpComb analysis is correct. Spring Security’s OnComittedResponseWrapper is detecting that the response is about to be committed, so it then tries to write the headers before allowing the operation that commits the response.

I’m curious what happens if you set HeaderWriterFilter.shouldWriteHeadersEagerly = true using something like this:

@Override
protected void configure(HttpSecurity http) throws Exception {
	http
		.headers(headers -> headers
			.withObjectPostProcessor(new ObjectPostProcessor<HeaderWriterFilter>() {
				@Override
				public HeaderWriterFilter postProcess(HeaderWriterFilter headerWriterFilter) {
					headerWriterFilter.setShouldWriteHeadersEagerly(true);
					return headerWriterFilter;
				}
			})
		)
...

This ensures the headers are written immediately. However, this can cause problems with cache control. In particular if someone overrides the cache control header to anything but Cache-Control: no-cache, no-store, max-age=0, must-revalidate Pragma: no-cache it can cause issues since Spring Security will also attempt to set other cache related headers like Expires: 0. The problem is that servlet spec allows the value of a header to be overridden, but not removed (which is what should happen).

rwinch on Jul 21, 2020

My team has seen sporadic test failures for all async endpoints that use StreamingResponseBody or, more generally, any async endpoints that use the WebAsyncManager.startCallableProcessing code path. All of these failures are due to ConcurrentModificationException in Spring Security’s HeaderWriterFilter.writeHeaders. Some occur in the original dispatch thread, while unwinding the filter chain; the rest occur in the async dispatch thread when our code begins writing to the response.

The crux of the problem seems to be that OnCommittedResponseWrapper is used in multiple threads, but isn’t thread-safe. One simple solution would be to make it thread-safe. Its boolean disableOnCommitted field that’s used here:

    private void doOnResponseCommitted() {
        if (!this.disableOnCommitted) {
            onResponseCommitted();
            disableOnResponseCommitted();
        }
    }

…could be changed to final AtomicBoolean and used thus:

    private void doOnResponseCommitted() {
        if (!this.disableOnCommitted.getAndSet(true)) {
            onResponseCommitted();
        }
    }

An even simpler solution would be for HeaderWriterFilter to stop trying to write to the response from two different threads. Application code normally commits any given HTTP response from exactly one thread. If HeaderWriterFilter just trusted OnCommittedResponseWrapper to do its thing, there would be no race condition. That would mean deleting the finally block below:

    try {
        filterChain.doFilter(headerWriterRequest, headerWriterResponse);
    }
    finally {
        headerWriterResponse.writeHeaders();
    }

Any reason not to?

For the time being, the workaround I’ve adopted is adding a response.flushBuffer(); call just before the return statement in each affected async endpoint. That call commits the response synchronously in the main dispatch thread before async processing begins—before the race condition can develop.

2is10 on Oct 9, 2023

At a minimum, the FirewalledResponse should be aware of the value on HttpServletResponse.isCommitted() not attempt to do anything if that returns true (indicating the response is committed and no header manipulation is now possible)

joakime on Oct 2, 2020