CVE-2024-23679: Session Fixation (CWE-384)
value: Enonic XP versions less than 7.7.4 are vulnerable to a session fixation issue. An remote and unauthenticated attacker can use prior sessions due to the lack of invalidating session attributes.


CWE description: Authenticating a user, or otherwise establishing a new user session, without invalidating any existing session identifier gives an attacker the opportunity to steal authenticated sessions.
CVSS v3 base score: 9.8
Published: 2024-01-19T21:15Z
Repository: https://github.com/enonic/xp
Language: Java
Files changed: modules/lib/lib-auth/src/main/java/com/enonic/xp/lib/auth/LoginHandler.java, modules/lib/lib-auth/src/test/java/com/enonic/xp/lib/auth/LoginHandlerTest.java
Commit date: 2021-12-06 14:37:33 +0100
Commit message: Invalidate old session after login #9253

Diff: diff --git a/modules/lib/lib-auth/src/main/java/com/enonic/xp/lib/auth/LoginHandler.java b/modules/lib/lib-auth/src/main/java/com/enonic/xp/lib/auth/LoginHandler.java
index ff92bf4c5b..624858588e 100644
--- a/modules/lib/lib-auth/src/main/java/com/enonic/xp/lib/auth/LoginHandler.java
+++ b/modules/lib/lib-auth/src/main/java/com/enonic/xp/lib/auth/LoginHandler.java
@@ -7,10 +7,11 @@ import java.util.stream.Collectors;
 
 import javax.servlet.http.HttpSession;
 
 import com.enonic.xp.context.Context;
 import com.enonic.xp.context.ContextBuilder;
+import com.enonic.xp.context.LocalScope;
 import com.enonic.xp.portal.PortalRequest;
 import com.enonic.xp.script.bean.BeanContext;
 import com.enonic.xp.script.bean.ScriptBean;
 import com.enonic.xp.security.IdProvider;
 import com.enonic.xp.security.IdProviderKey;
@@ -28,15 +29,10 @@ import com.enonic.xp.security.auth.VerifiedUsernameAuthToken;
 import com.enonic.xp.session.Session;
 
 public final class LoginHandler
     implements ScriptBean
 {
-    private enum Scope
-    {
-        SESSION, REQUEST, NONE
-    }
-
     private String user;
 
     private String password;
 
     private boolean skipAuth;
@@ -111,19 +107,30 @@ public final class LoginHandler
         }
     }
 
     private void createSession( final AuthenticationInfo authInfo )
     {
-        final Session session = this.context.get().getLocalScope().getSession();
+        final LocalScope localScope = this.context.get().getLocalScope();
+        final Session session = localScope.getSession();
+
         if ( session != null )
         {
-            session.setAttribute( authInfo );
-        }
+            final var attributes = session.getAttributes();
+            session.invalidate();
 
-        if ( this.sessionTimeout != null )
-        {
-            setSessionTimeout();
+            final Session newSession = localScope.getSession();
+
+            if ( newSession != null )
+            {
+                attributes.forEach( newSession::setAttribute );
+                session.setAttribute( authInfo );
+
+                if ( this.sessionTimeout != null )
+                {
+                    setSessionTimeout();
+                }
+            }
         }
     }
 
     private boolean noIdProviderSpecified()
     {
@@ -147,13 +154,12 @@ public final class LoginHandler
     }
 
     private IdProviders getSortedIdProviders()
     {
         IdProviders idProviders = securityService.get().getIdProviders();
-        return IdProviders.from( idProviders.stream().
-            sorted( Comparator.comparing( u -> u.getKey().toString() ) ).
-            collect( Collectors.toList() ) );
+        return IdProviders.from(
+            idProviders.stream().sorted( Comparator.comparing( u -> u.getKey().toString() ) ).collect( Collectors.toList() ) );
     }
 
     private AuthenticationInfo attemptLogin()
     {
 
@@ -219,15 +225,16 @@ public final class LoginHandler
     }
 
     private <T> T runAsAuthenticated( Callable<T> runnable )
     {
         final AuthenticationInfo authInfo = AuthenticationInfo.create().principals( RoleKeys.AUTHENTICATED ).user( User.ANONYMOUS ).build();
-        return ContextBuilder.from( this.context.get() ).
-            authInfo( authInfo ).
-            repositoryId( SystemConstants.SYSTEM_REPO_ID ).
-            branch( SecurityConstants.BRANCH_SECURITY ).build().
-            callWith( runnable );
+        return ContextBuilder.from( this.context.get() )
+            .authInfo( authInfo )
+            .repositoryId( SystemConstants.SYSTEM_REPO_ID )
+            .branch( SecurityConstants.BRANCH_SECURITY )
+            .build()
+            .callWith( runnable );
     }
 
     private boolean isValidEmail( final String value )
     {
         return value != null && value.chars().filter( ch -> ch == '@' ).count() == 1;
@@ -251,6 +258,11 @@ public final class LoginHandler
     {
         this.securityService = context.getService( SecurityService.class );
         this.context = context.getBinding( Context.class );
         this.portalRequestSupplier = context.getBinding( PortalRequest.class );
     }
+
+    private enum Scope
+    {
+        SESSION, REQUEST, NONE
+    }
 }
diff --git a/modules/lib/lib-auth/src/test/java/com/enonic/xp/lib/auth/LoginHandlerTest.java b/modules/lib/lib-auth/src/test/java/com/enonic/xp/lib/auth/LoginHandlerTest.java
index 863598bf38..493e3377d2 100644
--- a/modules/lib/lib-auth/src/test/java/com/enonic/xp/lib/auth/LoginHandlerTest.java
+++ b/modules/lib/lib-auth/src/test/java/com/enonic/xp/lib/auth/LoginHandlerTest.java
@@ -21,10 +21,12 @@ import com.enonic.xp.session.Session;
 import com.enonic.xp.session.SessionMock;
 import com.enonic.xp.testing.ScriptTestSupport;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 
 public class LoginHandlerTest
     extends ScriptTestSupport
 {
     private SecurityService securityService;
@@ -190,10 +192,29 @@ public class LoginHandlerTest
         assertEquals( "idprovider1", matcher.loginIdProviderAttempts.get( 0 ).toString() );
         assertEquals( "idprovider2", matcher.loginIdProviderAttempts.get( 1 ).toString() );
         assertEquals( "idprovider3", matcher.loginIdProviderAttempts.get( 2 ).toString() );
     }
 
+    @Test
+    public void testSessionInvalidatedOnLogin()
+    {
+        final AuthenticationInfo authInfo = TestDataFixtures.createAuthenticationInfo();
+
+        final IdProviders idProviders =
+            IdProviders.from( IdProvider.create().displayName( "system" ).key( IdProviderKey.from( "system" ) ).build() );
+
+        Mockito.when( this.securityService.authenticate( Mockito.any() ) ).thenReturn( authInfo );
+        Mockito.when( this.securityService.getIdProviders() ).thenReturn( idProviders );
+
+        final SessionMock session = Mockito.spy( new SessionMock() );
+        ContextAccessor.current().getLocalScope().setSession( session );
+
+        runScript( "/lib/xp/examples/auth/login.js" );
+
+        verify( session, times( 5 ) ).invalidate();
+    }
+
     private static class AuthTokenMatcher
         implements ArgumentMatcher<AuthenticationToken>
     {
         EmailPasswordAuthToken thisObject;
 

Vulnerable code:     private enum Scope
    {
        SESSION, REQUEST, NONE
    }

        final Session session = this.context.get().getLocalScope().getSession();
            session.setAttribute( authInfo );
        }
        if ( this.sessionTimeout != null )
        {
            setSessionTimeout();
        return IdProviders.from( idProviders.stream().
            sorted( Comparator.comparing( u -> u.getKey().toString() ) ).
            collect( Collectors.toList() ) );
        return ContextBuilder.from( this.context.get() ).
            authInfo( authInfo ).
            repositoryId( SystemConstants.SYSTEM_REPO_ID ).
            branch( SecurityConstants.BRANCH_SECURITY ).build().
            callWith( runnable );
Fixed code: import com.enonic.xp.context.LocalScope;
        final LocalScope localScope = this.context.get().getLocalScope();
        final Session session = localScope.getSession();

            final var attributes = session.getAttributes();
            session.invalidate();
            final Session newSession = localScope.getSession();

            if ( newSession != null )
            {
                attributes.forEach( newSession::setAttribute );
                session.setAttribute( authInfo );

                if ( this.sessionTimeout != null )
                {
                    setSessionTimeout();
                }
            }
        return IdProviders.from(
            idProviders.stream().sorted( Comparator.comparing( u -> u.getKey().toString() ) ).collect( Collectors.toList() ) );
        return ContextBuilder.from( this.context.get() )
            .authInfo( authInfo )
            .repositoryId( SystemConstants.SYSTEM_REPO_ID )
            .branch( SecurityConstants.BRANCH_SECURITY )
            .build()
            .callWith( runnable );

    private enum Scope
    {
        SESSION, REQUEST, NONE
    }
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
    @Test
    public void testSessionInvalidatedOnLogin()
    {
        final AuthenticationInfo authInfo = TestDataFixtures.createAuthenticationInfo();

        final IdProviders idProviders =
            IdProviders.from( IdProvider.create().displayName( "system" ).key( IdProviderKey.from( "system" ) ).build() );

        Mockito.when( this.securityService.authenticate( Mockito.any() ) ).thenReturn( authInfo );
        Mockito.when( this.securityService.getIdProviders() ).thenReturn( idProviders );

        final SessionMock session = Mockito.spy( new SessionMock() );
        ContextAccessor.current().getLocalScope().setSession( session );

        runScript( "/lib/xp/examples/auth/login.js" );

        verify( session, times( 5 ) ).invalidate();
    }