[jboss-jira] [JBoss JIRA] Commented: (SECURITY-339) ClientLoginModule improperly handles SecurityAssociation stack in abort()

[Marco Schulze (JIRA)]

    [ https://jira.jboss.org/jira/browse/SECURITY-339?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12478967#action_12478967 ] 

Marco Schulze commented on SECURITY-339:
----------------------------------------

I just took a look at the test cases in http://fisheye.jboss.org/browse/JBossAS/projects/security/security-jboss-sx/branches/Branch_2_0/jbosssx/src/test/java/org/jboss/test/authentication/jaas/ClientLoginModuleUnitTestCase.java?r=91978 and I'm pretty sure it does *NOT* test a situation in which the bug described by this issue can be reproduced.

If I see it correctly, it tests only once for abort. But this test does not have multiple stacked logins and thus the wrong behaviour of the abort method can not be demonstrated.

Let me explain in a small example what goes wrong. Imagine we have two login processes happening and thus 2 principals are to be stacked. The first to authenticate is user1 and the second is user2.

If both logins succeed, it looks like this:

A1) ClientLoginModule.login() called for user1. No change on the stack yet!
A2) ClientLoginModule.commit() called for user1. user1 is now the only item on the stack.
A3) ClientLoginModule.login() called for user2. No change on the stack yet! user1 is still the only item on the stack.
A4) ClientLoginModule.commit() called for user2. user2 is now the last item on the stack.

Now the logouts would come in reverse order:

A5) ClientLoginModule.logout() called for user2. user2 is popped from the stack. user1 is the only item on the stack, now.
A6) ClientLoginModule.logout() called for user1. user1 is popped from the stack. The stack is empty now.


If the first login succeeds and the second login fails, however, things are completely different:

B1) ClientLoginModule.login() called for user1. No change on the stack yet!
B2) ClientLoginModule.commit() called for user1. user1 is now the only item on the stack.
B3) ClientLoginModule.login() called for user2. No change on the stack yet! user1 is still the only item on the stack.
B4) Some other login module failed during login, thus ClientLoginModule.abort() is called for user2. The current wrong code wants to pop user2 from the stack, but since user2 is not on the stack, it instead pops user1.

I hope that it is now clear why the code is wrong.

> ClientLoginModule improperly handles SecurityAssociation stack in abort()
> -------------------------------------------------------------------------
>
>                 Key: SECURITY-339
>                 URL: https://jira.jboss.org/jira/browse/SECURITY-339
>             Project: JBoss Security and Identity Management
>          Issue Type: Bug
>      Security Level: Public(Everyone can see) 
>          Components: JBossSX
>            Reporter: Marco Schulze
>            Assignee: Anil Saldhana
>             Fix For: JBossSecurity_2.0.4
>
>
> The abort() method calls SecurityAssociationActions.popPrincipalInfo() even though the corresponding push happens in commit() [via SecurityAssociationActions.setPrincipalInfo(loginPrincipal, loginCredential, subject)]. That means, whenever a login fails, the commit is not called (thus nothing pushed), but the abort pops out an element from the stack. This should not be done. IMHO the abort() method should look like this:
>   public boolean abort() throws LoginException
>    {
>       if( trace )
>          log.trace("abort");
>       if( restoreLoginIdentity == false )
>       {
>          // Clear the entire security association stack
>          SecurityAssociationActions.clear();         
>       }
>       return true;
>    }

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira