SEC-1741: Modify ContextPropagatingRemoteInvocation to pass a simple combination of principal/credentials as Strings, rather than serializing the whole SecurityContext object from the client.

remoting/src/main/java/org/springframework/security/remoting/rmi/ContextPropagatingRemoteInvocation.java

@@ -15,33 +15,31 @@
 
 package org.springframework.security.remoting.rmi;
 
-import org.springframework.security.core.SpringSecurityCoreVersion;
-import org.springframework.security.core.context.SecurityContext;
-import org.springframework.security.core.context.SecurityContextHolder;
-
 import org.aopalliance.intercept.MethodInvocation;
-
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
 import org.springframework.remoting.support.RemoteInvocation;
+import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.SpringSecurityCoreVersion;
+import org.springframework.security.core.context.SecurityContextHolder;
 
 import java.lang.reflect.InvocationTargetException;
 
 
 /**
- * The actual <code>RemoteInvocation</code> that is passed from the client to the server, which contains the
- * contents of {@link SecurityContextHolder}, being a {@link SecurityContext} object.
+ * The actual {@code RemoteInvocation} that is passed from the client to the server.
  * <p>
- * When constructed on the client via {@link ContextPropagatingRemoteInvocationFactory}, the contents of the
- * <code>SecurityContext</code> are stored inside the object. The object is then passed to the server that is
- * processing the remote invocation. Upon the server invoking the remote invocation, it will retrieve the passed
- * contents of the <code>SecurityContextHolder</code> and set them on the server-side
- * <code>SecurityContextHolder</code> while the target object is invoked. When the target invocation has been
- * completed, the security context will be cleared using a call to {@link SecurityContextHolder#clearContext()}.
+ * The principal and credentials information will be extracted from the current
+ * security context and passed to the server as part of the invocation object.
+ * <p>
+ * To avoid potential serialization-based attacks, this implementation interprets the values as {@code String}s
+ * and creates a {@code UsernamePasswordAuthenticationToken} on the server side to hold them. If a different
+ * token type is required you can override the {@code createAuthenticationRequest} method.
  *
  * @author James Monaghan
  * @author Ben Alex
+ * @author Luke Taylor
  */
 public class ContextPropagatingRemoteInvocation extends RemoteInvocation {
 
@@ -51,34 +49,40 @@ public class ContextPropagatingRemoteInvocation extends RemoteInvocation {
 
     //~ Instance fields ================================================================================================
 
-    private final SecurityContext securityContext;
+    private final String principal;
+    private final String credentials;
 
     //~ Constructors ===================================================================================================
 
     /**
-     * Constructs the object, storing the value of the client-side
-     * <code>SecurityContextHolder</code> inside the object.
+     * Constructs the object, storing the principal and credentials extracted from the client-side
+     * security context.
      *
      * @param methodInvocation the method to invoke
      */
     public ContextPropagatingRemoteInvocation(MethodInvocation methodInvocation) {
         super(methodInvocation);
-        securityContext = SecurityContextHolder.getContext();
+        Authentication currentUser = SecurityContextHolder.getContext().getAuthentication();
+
+        if (currentUser != null) {
+            principal = currentUser.getPrincipal().toString();
+            credentials = currentUser.getCredentials().toString();
+        } else {
+            principal = credentials = null;
+        }
 
         if (logger.isDebugEnabled()) {
-            logger.debug("RemoteInvocation now has SecurityContext: " + securityContext);
+            logger.debug("RemoteInvocation now has principal: " + principal);
         }
     }
 
     //~ Methods ========================================================================================================
 
     /**
-     * Invoked on the server-side as described in the class JavaDocs.
+     * Invoked on the server-side.
      * <p>
-     * Invocations will always have their {@link org.springframework.security.core.Authentication#setAuthenticated(boolean)}
-     * set to <code>false</code>, which is guaranteed to always be accepted by <code>Authentication</code>
-     * implementations. This ensures that even remotely authenticated <code>Authentication</code>s will be untrusted by
-     * the server-side, which is an appropriate security measure.
+     * The transmitted principal and credentials will be used to create an unauthenticated {@code Authentication}
+     * instance for processing by the {@code AuthenticationManager}.
      *
      * @param targetObject the target object to apply the invocation to
      *
@@ -90,15 +94,15 @@ public class ContextPropagatingRemoteInvocation extends RemoteInvocation {
      */
     public Object invoke(Object targetObject)
             throws NoSuchMethodException, IllegalAccessException, InvocationTargetException {
-        SecurityContextHolder.setContext(securityContext);
 
-        if ((SecurityContextHolder.getContext() != null)
-            && (SecurityContextHolder.getContext().getAuthentication() != null)) {
-            SecurityContextHolder.getContext().getAuthentication().setAuthenticated(false);
-        }
+        if (principal != null) {
+            Authentication request = createAuthenticationRequest(principal, credentials);
+            request.setAuthenticated(false);
+            SecurityContextHolder.getContext().setAuthentication(request);
 
-        if (logger.isDebugEnabled()) {
-            logger.debug("Set SecurityContextHolder to contain: " + securityContext);
+            if (logger.isDebugEnabled()) {
+                logger.debug("Set SecurityContextHolder to contain: " + request);
+            }
         }
 
         try {
@@ -111,4 +115,11 @@ public class ContextPropagatingRemoteInvocation extends RemoteInvocation {
             }
         }
     }
+
+    /**
+     * Creates the server-side authentication request object.
+     */
+    protected Authentication createAuthenticationRequest(String principal, String credentials) {
+        return new UsernamePasswordAuthenticationToken(principal, credentials);
+    }
 }

remoting/src/test/java/org/springframework/security/remoting/rmi/ContextPropagatingRemoteInvocationTests.java

@@ -16,20 +16,13 @@
 package org.springframework.security.remoting.rmi;
 
 import junit.framework.TestCase;
-
+import org.aopalliance.intercept.MethodInvocation;
 import org.springframework.security.TargetObject;
-
 import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolder;
-
-import org.springframework.security.remoting.rmi.ContextPropagatingRemoteInvocation;
-import org.springframework.security.remoting.rmi.ContextPropagatingRemoteInvocationFactory;
-
 import org.springframework.security.util.SimpleMethodInvocation;
 
-import org.aopalliance.intercept.MethodInvocation;
-
 import java.lang.reflect.Method;
 
 
@@ -57,8 +50,7 @@ public class ContextPropagatingRemoteInvocationTests extends TestCase {
         return (ContextPropagatingRemoteInvocation) factory.createRemoteInvocation(mi);
     }
 
-    public void testContextIsResetEvenIfExceptionOccurs()
-        throws Exception {
+    public void testContextIsResetEvenIfExceptionOccurs() throws Exception {
         // Setup client-side context
         Authentication clientSideAuthentication = new UsernamePasswordAuthenticationToken("rod", "koala");
         SecurityContextHolder.getContext().setAuthentication(clientSideAuthentication);
@@ -96,10 +88,10 @@ public class ContextPropagatingRemoteInvocationTests extends TestCase {
     }
 
     public void testNullContextHolderDoesNotCauseInvocationProblems() throws Exception {
-        SecurityContextHolder.getContext().setAuthentication(null); // just to be explicit
+        SecurityContextHolder.clearContext(); // just to be explicit
 
         ContextPropagatingRemoteInvocation remoteInvocation = getRemoteInvocation();
-        SecurityContextHolder.getContext().setAuthentication(null); // unnecessary, but for explicitness
+        SecurityContextHolder.clearContext(); // unnecessary, but for explicitness
 
         assertEquals("some_string Authentication empty", remoteInvocation.invoke(new TargetObject()));
     }