NPE in HttpSessionSecurityContextRepository.isTransientAuthentication

[windmueller]

Describe the bug
On our systems we see a NullPointerException for cached static resources with spring-security-web-5.3.3:

java.lang.NullPointerException: null
	at org.springframework.security.web.context.HttpSessionSecurityContextRepository.isTransientAuthentication(HttpSessionSecurityContextRepository.java:444) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.context.HttpSessionSecurityContextRepository.access$300(HttpSessionSecurityContextRepository.java:82) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.context.HttpSessionSecurityContextRepository$SaveToSessionResponseWrapper.createNewSessionIfAllowed(HttpSessionSecurityContextRepository.java:389) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.context.HttpSessionSecurityContextRepository$SaveToSessionResponseWrapper.saveContext(HttpSessionSecurityContextRepository.java:363) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.context.SaveContextOnUpdateOrErrorResponseWrapper.onResponseCommitted(SaveContextOnUpdateOrErrorResponseWrapper.java:85) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.util.OnCommittedResponseWrapper.doOnResponseCommitted(OnCommittedResponseWrapper.java:252) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.util.OnCommittedResponseWrapper.checkContentLength(OnCommittedResponseWrapper.java:242) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.util.OnCommittedResponseWrapper.access$200(OnCommittedResponseWrapper.java:34) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.util.OnCommittedResponseWrapper$SaveContextServletOutputStream.write(OnCommittedResponseWrapper.java:644) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.security.web.util.OnCommittedResponseWrapper$SaveContextServletOutputStream.write(OnCommittedResponseWrapper.java:645) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	at org.springframework.util.FastByteArrayOutputStream.writeTo(FastByteArrayOutputStream.java:249) ~[spring-core-5.2.7.RELEASE.jar:5.2.7.RELEASE]
	at org.springframework.web.util.ContentCachingResponseWrapper.copyBodyToResponse(ContentCachingResponseWrapper.java:215) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
	at org.springframework.web.util.ContentCachingResponseWrapper.copyBodyToResponse(ContentCachingResponseWrapper.java:199) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
	at org.springframework.web.filter.ShallowEtagHeaderFilter.updateResponse(ShallowEtagHeaderFilter.java:132) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
	at org.springframework.web.filter.ShallowEtagHeaderFilter.doFilterInternal(ShallowEtagHeaderFilter.java:109) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:119) ~[spring-web-5.2.7.RELEASE.jar:5.2.7.RELEASE]
	at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1618) ~[jetty-servlet-9.4.28.v20200408.jar:9.4.28.v20200408]
	at org.springframework.security.web.authentication.preauth.AbstractPreAuthenticatedProcessingFilter.doFilter(AbstractPreAuthenticatedProcessingFilter.java:124) ~[spring-security-web-5.3.3.RELEASE.jar:5.3.3.RELEASE]
	[...]

This seems to happen because the authentication returned by SecurityContextHolder.getContext() is null.

To Reproduce
Unknown, because the error does not happen often.

Expected behavior
I am not sure if the authentication may be null at all, but instead of a NullPointerException resulting in an "Internal Server Error" I would expect a plain "Unauthorized".

[jzheaux]

@stovocor, I agree that a NullPointerException in this case is not desirable.

Since isTransientAuthentication cannot determine whether or not the authentication is transient in this case, I think it's reasonable to have it return false.

[marcusdacoregio]

@stovocor, thank you for pointing this out.

I was looking into the code and the authentication is checked for null before the method is called:

if (authentication == null || trustResolver.isAnonymous(authentication)) { // authentication is being checked for null
	if (logger.isDebugEnabled()) {
		logger.debug("SecurityContext is empty or contents are anonymous - context will not be stored in HttpSession.");
	}

	if (httpSession != null && authBeforeExecution != null) {
		// SEC-1587 A non-anonymous context may still be in the session
		// SEC-1735 remove if the contextBeforeExecution was not anonymous
		httpSession.removeAttribute(springSecurityContextKey);
	}
	return;
}

if (httpSession == null) {
	httpSession = createNewSessionIfAllowed(context); // the method that is throwing NPE call
}

So in the scenario where the authentication is null it shouldreturn from this method and not reach the createNewSessionIfAllowed.

It seems to be a concurrency issue where the authentication is getting removed after the authentication == null test and before the createNewSessionIfAllowed method call.

To narrow the problem down, could you provide a sample that replicates the behavior?

[windmueller]

To narrow the problem down, could you provide a sample that replicates the behavior?

I am very sorry, but as I mentioned in the original bugreport, I am not able to reproduce this problem and it occurs rarely.

[marcusdacoregio]

@stovocor I see, thanks for the feedback. We are planning to work on a more general solution that would address more scenarios. @jzheaux do we have already an issue open for that?

[jzheaux]

Yes, #9634

[marcusdacoregio]

Hi @stovocor, we have now a proposed PR #9813 to address this problem.

[marcusdacoregio]

Hello @stovocor, the PR is now merged and is aimed at the milestone 5.6.0-M1. Thanks for the report and feel free to discuss here if you want anything else.