[jboss-jira] [JBoss JIRA] (SECURITY-659) WebJASPIAuthenticator ignores GroupPrincipalCallback but requires PasswordValidationCallback

[arjan tijms (JIRA)]

arjan tijms created SECURITY-659:
------------------------------------

             Summary: WebJASPIAuthenticator ignores GroupPrincipalCallback but requires PasswordValidationCallback
                 Key: SECURITY-659
                 URL: https://issues.jboss.org/browse/SECURITY-659
             Project: PicketBox (JBoss Security and Identity Management)
          Issue Type: Bug
      Security Level: Public (Everyone can see)
          Components: PicketBox
    Affects Versions: PicketBox_v4_0_7
            Reporter: arjan tijms
            Assignee: Anil Saldhana


In JBoss AS 7.1.1, if a user provided {{ServerAuthModule}} provides a {{GroupPrincipalCallback}}, then this is ignored by {{WebJASPIAuthenticator}}. The provided handler copies the {{GroupPrincipalCallback}}, but the authenticator then does nothing with it. Simulteanously, if the {{ServerAuthModule}} does not provide a {{PasswordValidationCallback}} to the handler, then this will result in a null pointer exception in the authenticator.

Regarding the ignored {{GroupPrincipalCallback}}, the problem seems to be in the following code:

{code:title=WebJASPIAuthenticator#authenticate}
// ...
       if (result) {
            PasswordValidationCallback pvc = cbh.getPasswordValidationCallback();
            CallerPrincipalCallback cpc = cbh.getCallerPrincipalCallback();

            // get the client principal from the callback.
            Principal clientPrincipal = cpc.getPrincipal();
            if (clientPrincipal == null) {
                clientPrincipal = new SimplePrincipal(cpc.getName());
            }

            // if the client principal is not a jboss generic principal, we need to build one before registering.
            if (!(clientPrincipal instanceof JBossGenericPrincipal))
                clientPrincipal = this.buildJBossPrincipal(clientSubject, clientPrincipal);
{code}

{{buildJBossPrincipal()}} looks at the "Roles" group in the Subject, but this hasn't been set by either the handler or other code based on what the GroupPrincipalCallback contains.

I wonder if changing this into the following would be more correct:

{code:title=WebJASPIAuthenticator#authenticate}
// ...
       if (result) {
            PasswordValidationCallback pvc = cbh.getPasswordValidationCallback();
            CallerPrincipalCallback cpc = cbh.getCallerPrincipalCallback();
            GroupPrincipalCallback gpc = cbh.getGroupPrincipalCallback(); // ADDED

            // get the client principal from the callback.
            Principal clientPrincipal = cpc.getPrincipal();
            if (clientPrincipal == null) {
                clientPrincipal = new SimplePrincipal(cpc.getName());
            }

            // if the client principal is not a jboss generic principal, we need to build one before registering.
            if (!(clientPrincipal instanceof JBossGenericPrincipal))
                clientPrincipal = this.buildJBossPrincipal(clientSubject, clientPrincipal, gpc); // ADDED gpc PARAMETER
{code}

With {{buildJBossPrincipal()}} implemented as:

{code:title=WebJASPIAuthenticator#buildJBossPrincipal}
    protected Principal buildJBossPrincipal(Subject subject, Principal principal, GroupPrincipalCallback groupPrincipalCallback) {

        List<String> roles = new ArrayList<String>();
        // look for roles in the subject first.
        for (Principal p : subject.getPrincipals()) {
            if (p instanceof Group && p.getName().equals("Roles")) {
                Enumeration<? extends Principal> members = ((Group) p).members();
                while (members.hasMoreElements())
                    roles.add(members.nextElement().getName());
            }
        }
        
        // START ADDED
        if (groupPrincipalCallback != null && groupPrincipalCallback.getGroups() != null) {
            for (String group : groupPrincipalCallback.getGroups()) {
                roles.add(group);
            }
        }
        // END ADDED

        // if the subject didn't contain any roles, look for the roles declared in the deployment descriptor.
        JBossWebRealm realm = (JBossWebRealm) this.getContainer().getRealm();
        Set<String> descriptorRoles = realm.getPrincipalVersusRolesMap().get(principal.getName());
        if (roles.isEmpty() && descriptorRoles != null)
            roles.addAll(descriptorRoles);

        // build and return the JBossGenericPrincipal.
        return new JBossGenericPrincipal(realm, principal.getName(), null, roles, principal, null, null, null, subject);

    }
{code}

As for the PasswordValidationCallback, WebJASPIAuthenticator now contains the following code in {{authenticate()}}:

{code:title=WebJASPIAuthenticator#authenticate}
this.register(request, response, clientPrincipal, authMethod, pvc.getUsername(),
    new String(pvc.getPassword()));
{code}

The {{register()}} method considers both username and password as optional, but because there's no null check on {{pvc}}, the above line will throw a NPE in case no PasswordValidationCallback is provided. This could perhaps be changed into something like the following:

{code:title=WebJASPIAuthenticator#authenticate}
this.register(request, response, clientPrincipal, authMethod, pvc != null ? pvc.getUsername() : null,
    pvc != null ? new String(pvc.getPassword()) : null);
{code}


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.jboss.org/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira