Repository navigation
SEC-423: CLONE -SEC 356 Introduces New Bug: Changes to Authentication leak into synchronous requests when using HttpSessionContextIntegrationFilter #684
Description
Christos Kokotsis(Migrated from SEC-423) said:
HttpSessionContextIntegrationFilter will read an existing SecurityContext object from the session and attach it to the Http request thread by calling: SecurityContextHolder#setContext. This means that simultaneous requests get the same SecurityContext object. If one of those threads changes the authentication attached to the context (for example, to enable some “Run As” functionality such as in org.acegisecurity.intercept.AbstractSecurityInterceptor) that authentication will be seen to change in all the request threads and may enable those threads to be able to gain access that they shouldn’t have.
Activity
spring-projects-issues commentedon Jan 4, 2007
Christos Kokotsis said:
The assertion of the contextFromHttpSession is cloneable throws an illegal argument exception where the cloneable context object is null.
Moving the fix to after the contextFromSessionObject != null check will resolve this issue.
Here is a working doFilter method with the fixed applied:
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { if ((request != null) && (request.getAttribute(FILTER_APPLIED) != null)) { // ensure that filter is only applied once per request chain.doFilter(request, response); } else { if (request != null) { request.setAttribute(FILTER_APPLIED, Boolean.TRUE); }```
try { httpSession = ((HttpServletRequest) request).getSession(forceEagerSessionCreation); } catch (IllegalStateException ignored) {} if (httpSession != null) { httpSessionExistedAtStartOfRequest = true; Object contextFromSessionObject = httpSession.getAttribute(ACEGI_SECURITY_CONTEXT_KEY); if (contextFromSessionObject != null) { // move of cloning codeto after check for null (see SEC-423) // Clone if required (see SEC-356) if (cloneFromHttpSession) { Assert.isInstanceOf(Cloneable.class, contextFromSessionObject, “Context must implement Clonable and provide a Object.clone() method”); try { Method m = contextFromSessionObject.getClass().getMethod(“clone”, new Class[] {}); if (!m.isAccessible()) { m.setAccessible(true); } contextFromSessionObject = m.invoke(contextFromSessionObject, new Object[] {}); } catch (Exception ex) { ReflectionUtils.handleReflectionException(ex); } } if (contextFromSessionObject instanceof SecurityContext) { if (logger.isDebugEnabled()) { logger.debug( “Obtained from ACEGI_SECURITY_CONTEXT a valid SecurityContext and set to SecurityContextHolder: ’” + contextFromSessionObject + “’”); } SecurityContextHolder.setContext((SecurityContext) contextFromSessionObject); } else { if (logger.isWarnEnabled()) { logger.warn(“ACEGI_SECURITY_CONTEXT did not contain a SecurityContext but contained: ’” + contextFromSessionObject + “’; are you improperly modifying the HttpSession directly (you should always use SecurityContextHolder) or using the HttpSession attribute reserved for this class? – new SecurityContext instance associated with SecurityContextHolder”); } SecurityContextHolder.setContext(generateNewContext()); } } else { if (logger.isDebugEnabled()) { logger.debug( “HttpSession returned null object for ACEGI_SECURITY_CONTEXT – new SecurityContext instance associated with SecurityContextHolder”); } SecurityContextHolder.setContext(generateNewContext()); } } else { if (logger.isDebugEnabled()) { logger.debug( “No HttpSession currently exists – new SecurityContext instance associated with SecurityContextHolder”); } SecurityContextHolder.setContext(generateNewContext()); } // Make the HttpSession null, as we want to ensure we don’t keep // a reference to the HttpSession laying around in case the // chain.doFilter() invalidates it. httpSession = null; // Proceed with chain int contextWhenChainProceeded = SecurityContextHolder.getContext().hashCode(); try { chain.doFilter(request, response); } catch (IOException ioe) { throw ioe; } catch (ServletException se) { throw se; } finally { // do clean up, even if there was an exception // Store context back to HttpSession try { httpSession = ((HttpServletRequest) request).getSession(false); } catch (IllegalStateException ignored) {} if ((httpSession == null) && httpSessionExistedAtStartOfRequest) { if (logger.isDebugEnabled()) { logger.debug( “HttpSession is now null, but was not null at start of request; session was invalidated, so do not create a new session”); } } // Generate a HttpSession only if we need to if ((httpSession == null) && !httpSessionExistedAtStartOfRequest) { if (!allowSessionCreation) { if (logger.isDebugEnabled()) { logger.debug( “The HttpSession is currently null, and the HttpSessionContextIntegrationFilter is prohibited from creating a HttpSession (because the allowSessionCreation property is false) – SecurityContext thus not stored for next request”); } } else if (!contextObject.equals(SecurityContextHolder.getContext())) { if (logger.isDebugEnabled()) { logger.debug(“HttpSession being created as SecurityContextHolder contents are non-default”); } try { httpSession = ((HttpServletRequest) request).getSession(true); } catch (IllegalStateException ignored) {} } else { if (logger.isDebugEnabled()) { logger.debug( “HttpSession is null, but SecurityContextHolder has not changed from default: ’ " + SecurityContextHolder.getContext() + “’; not creating HttpSession or storing SecurityContextHolder contents”); } } } // If HttpSession exists, store current SecurityContextHolder // contents // but only if SecurityContext has actually changed (see JIRA // SEC-37) if ((httpSession != null) && (SecurityContextHolder.getContext().hashCode() != contextWhenChainProceeded)) { httpSession.setAttribute(ACEGI_SECURITY_CONTEXT_KEY, SecurityContextHolder.getContext()); if (logger.isDebugEnabled()) { logger.debug(“SecurityContext stored to HttpSession: ’” + SecurityContextHolder.getContext() + “’”); } } // Remove SecurityContextHolder contents SecurityContextHolder.clearContext(); if (logger.isDebugEnabled()) { logger.debug(“SecurityContextHolder set to new context, as request processing completed”); } } }HttpSession httpSession = null;
boolean httpSessionExistedAtStartOfRequest = false;
```
}spring-projects-issues commentedon Apr 26, 2007
Vishal Puri said:
Reasonable fix. I have made a change and closed the issue.