8282730: LdapLoginModule throw NPE from logout method after login fai…

…lure

Backport-of: 554f44ecb1134acff3eaf02e2e1c0e01158ab7e5

src/java.base/share/classes/javax/security/auth/Subject.java

@@ -144,8 +144,8 @@ public final class Subject implements java.io.Serializable {
      * has been set read-only before permitting subsequent modifications.
      * The newly created Sets also prevent illegal modifications
      * by ensuring that callers have sufficient permissions.  These Sets
-     * also prohibit null elements, and attempts to add or query a null
-     * element will result in a {@code NullPointerException}.
+     * also prohibit null elements, and attempts to add, query, or remove
+     * a null element will result in a {@code NullPointerException}.
      *
      * <p> To modify the Principals Set, the caller must have
      * {@code AuthPermission("modifyPrincipals")}.
@@ -174,8 +174,8 @@ public Subject() {
      * has been set read-only before permitting subsequent modifications.
      * The newly created Sets also prevent illegal modifications
      * by ensuring that callers have sufficient permissions.  These Sets
-     * also prohibit null elements, and attempts to add or query a null
-     * element will result in a {@code NullPointerException}.
+     * also prohibit null elements, and attempts to add, query, or remove
+     * a null element will result in a {@code NullPointerException}.
      *
      * <p> To modify the Principals Set, the caller must have
      * {@code AuthPermission("modifyPrincipals")}.

src/java.base/share/classes/javax/security/auth/login/LoginContext.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -691,13 +691,13 @@ private void invoke(String methodName) throws LoginException {
         // - this can only be non-zero if methodName is LOGIN_METHOD
 
         for (int i = moduleIndex; i < moduleStack.length; i++, moduleIndex++) {
+            String name = moduleStack[i].entry.getLoginModuleName();
             try {
 
                 if (moduleStack[i].module == null) {
 
                     // locate and instantiate the LoginModule
                     //
-                    String name = moduleStack[i].entry.getLoginModuleName();
                     Set<Provider<LoginModule>> lmProviders;
                     synchronized(providersCache){
                         lmProviders = providersCache.get(contextClassLoader);
@@ -780,16 +780,16 @@ private void invoke(String methodName) throws LoginException {
                         clearState();
 
                         if (debug != null)
-                            debug.println(methodName + " SUFFICIENT success");
+                            debug.println(name + " " + methodName + " SUFFICIENT success");
                         return;
                     }
 
                     if (debug != null)
-                        debug.println(methodName + " success");
+                        debug.println(name + " " + methodName + " success");
                     success = true;
                 } else {
                     if (debug != null)
-                        debug.println(methodName + " ignored");
+                        debug.println(name + " " + methodName + " ignored");
                 }
             } catch (Exception ite) {
 
@@ -854,7 +854,7 @@ private void invoke(String methodName) throws LoginException {
                     AppConfigurationEntry.LoginModuleControlFlag.REQUISITE) {
 
                     if (debug != null)
-                        debug.println(methodName + " REQUISITE failure");
+                        debug.println(name + " " + methodName + " REQUISITE failure");
 
                     // if REQUISITE, then immediately throw an exception
                     if (methodName.equals(ABORT_METHOD) ||
@@ -869,7 +869,7 @@ private void invoke(String methodName) throws LoginException {
                     AppConfigurationEntry.LoginModuleControlFlag.REQUIRED) {
 
                     if (debug != null)
-                        debug.println(methodName + " REQUIRED failure");
+                        debug.println(name + " " + methodName + " REQUIRED failure");
 
                     // mark down that a REQUIRED module failed
                     if (firstRequiredError == null)
@@ -878,7 +878,7 @@ private void invoke(String methodName) throws LoginException {
                 } else {
 
                     if (debug != null)
-                        debug.println(methodName + " OPTIONAL failure");
+                        debug.println(name + " " + methodName + " OPTIONAL failure");
 
                     // mark down that an OPTIONAL module failed
                     if (firstError == null)

src/java.base/share/classes/javax/security/auth/spi/LoginModule.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,7 +26,6 @@
 package javax.security.auth.spi;
 
 import javax.security.auth.Subject;
-import javax.security.auth.AuthPermission;
 import javax.security.auth.callback.*;
 import javax.security.auth.login.*;
 import java.util.Map;
@@ -50,13 +49,13 @@
  * a {@code Subject}, a {@code CallbackHandler}, shared
  * {@code LoginModule} state, and LoginModule-specific options.
  *
- * The {@code Subject} represents the
+ * <p> The {@code Subject} represents the
  * {@code Subject} currently being authenticated and is updated
  * with relevant Credentials if authentication succeeds.
  * LoginModules use the {@code CallbackHandler} to
  * communicate with users.  The {@code CallbackHandler} may be
  * used to prompt for usernames and passwords, for example.
- * Note that the {@code CallbackHandler} may be null.  LoginModules
+ * Note that the {@code CallbackHandler} may be {@code null}.  LoginModules
  * which absolutely require a {@code CallbackHandler} to authenticate
  * the {@code Subject} may throw a {@code LoginException}.
  * LoginModules optionally use the shared state to share information
@@ -129,7 +128,7 @@
 public interface LoginModule {
 
     /**
-     * Initialize this LoginModule.
+     * Initialize this {@code LoginModule}.
      *
      * <p> This method is called by the {@code LoginContext}
      * after this {@code LoginModule} has been instantiated.
@@ -163,12 +162,12 @@ void initialize(Subject subject, CallbackHandler callbackHandler,
      * {@code Subject} information such
      * as a username and password and then attempt to verify the password.
      * This method saves the result of the authentication attempt
-     * as private state within the LoginModule.
+     * as private state within the {@code LoginModule}.
      *
      * @exception LoginException if the authentication fails
      *
-     * @return true if the authentication succeeded, or false if this
-     *                  {@code LoginModule} should be ignored.
+     * @return {@code true} if the authentication succeeded, or {@code false}
+     *                  if this {@code LoginModule} should be ignored.
      */
     boolean login() throws LoginException;
 
@@ -190,8 +189,8 @@ void initialize(Subject subject, CallbackHandler callbackHandler,
      *
      * @exception LoginException if the commit fails
      *
-     * @return true if this method succeeded, or false if this
-     *                  {@code LoginModule} should be ignored.
+     * @return {@code true} if this method succeeded, or {@code false}
+     *                  if this {@code LoginModule} should be ignored.
      */
     boolean commit() throws LoginException;
 
@@ -210,8 +209,8 @@ void initialize(Subject subject, CallbackHandler callbackHandler,
      *
      * @exception LoginException if the abort fails
      *
-     * @return true if this method succeeded, or false if this
-     *                  {@code LoginModule} should be ignored.
+     * @return {@code true} if this method succeeded, or {@code false}
+     *                  if this {@code LoginModule} should be ignored.
      */
     boolean abort() throws LoginException;
 
@@ -223,8 +222,15 @@ void initialize(Subject subject, CallbackHandler callbackHandler,
      *
      * @exception LoginException if the logout fails
      *
-     * @return true if this method succeeded, or false if this
-     *                  {@code LoginModule} should be ignored.
+     * @return {@code true} if this method succeeded, or {@code false}
+     *                  if this {@code LoginModule} should be ignored.
+     *
+     * @implSpec Implementations should check if a variable is {@code null}
+     *      before removing it from the Principals or Credentials set
+     *      of a {@code Subject}, otherwise a {@code NullPointerException}
+     *      will be thrown as these sets {@linkplain Subject#Subject()
+     *      prohibit null elements}. This is especially important if
+     *      this method is called after a login failure.
      */
     boolean logout() throws LoginException;
 }

src/java.management/share/classes/com/sun/jmx/remote/security/FileLoginModule.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2004, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -422,7 +422,9 @@ public boolean logout() throws LoginException {
             cleanState();
             throw new LoginException ("Subject is read-only");
         }
-        subject.getPrincipals().remove(user);
+        if (user != null) {
+            subject.getPrincipals().remove(user);
+        }
 
         // clean out state
         cleanState();

src/jdk.security.auth/share/classes/com/sun/security/auth/module/JndiLoginModule.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -469,11 +469,18 @@ public boolean logout() throws LoginException {
             cleanState();
             throw new LoginException ("Subject is Readonly");
         }
-        subject.getPrincipals().remove(userPrincipal);
-        subject.getPrincipals().remove(UIDPrincipal);
-        subject.getPrincipals().remove(GIDPrincipal);
-        for (int i = 0; i < supplementaryGroups.size(); i++) {
-            subject.getPrincipals().remove(supplementaryGroups.get(i));
+        if (userPrincipal != null) {
+            subject.getPrincipals().remove(userPrincipal);
+        }
+        if (UIDPrincipal != null) {
+            subject.getPrincipals().remove(UIDPrincipal);
+        }
+        if (GIDPrincipal != null) {
+            subject.getPrincipals().remove(GIDPrincipal);
+        }
+        for (UnixNumericGroupPrincipal gp : supplementaryGroups) {
+            // gp is never null
+            subject.getPrincipals().remove(gp);
         }
 
 

src/jdk.security.auth/share/classes/com/sun/security/auth/module/KeyStoreLoginModule.java

@@ -862,23 +862,25 @@ private void logoutInternal() throws LoginException {
             certP = null;
             status = INITIALIZED;
             // destroy the private credential
-            Iterator<Object> it = subject.getPrivateCredentials().iterator();
-            while (it.hasNext()) {
-                Object obj = it.next();
-                if (privateCredential.equals(obj)) {
-                    privateCredential = null;
-                    try {
-                        ((Destroyable)obj).destroy();
-                        if (debug)
-                            debugPrint("Destroyed private credential, " +
-                                       obj.getClass().getName());
-                        break;
-                    } catch (DestroyFailedException dfe) {
-                        LoginException le = new LoginException
-                            ("Unable to destroy private credential, "
-                             + obj.getClass().getName());
-                        le.initCause(dfe);
-                        throw le;
+            if (privateCredential != null) {
+                Iterator<Object> it = subject.getPrivateCredentials().iterator();
+                while (it.hasNext()) {
+                    Object obj = it.next();
+                    if (privateCredential.equals(obj)) {
+                        privateCredential = null;
+                        try {
+                            ((Destroyable) obj).destroy();
+                            if (debug)
+                                debugPrint("Destroyed private credential, " +
+                                        obj.getClass().getName());
+                            break;
+                        } catch (DestroyFailedException dfe) {
+                            LoginException le = new LoginException
+                                    ("Unable to destroy private credential, "
+                                            + obj.getClass().getName());
+                            le.initCause(dfe);
+                            throw le;
+                        }
                     }
                 }
             }

src/jdk.security.auth/share/classes/com/sun/security/auth/module/Krb5LoginModule.java

@@ -1206,8 +1206,10 @@ public boolean logout() throws LoginException {
             throw new LoginException("Subject is Readonly");
         }
 
-        subject.getPrincipals().remove(kerbClientPrinc);
-           // Let us remove all Kerberos credentials stored in the Subject
+        if (kerbClientPrinc != null) {
+            subject.getPrincipals().remove(kerbClientPrinc);
+        }
+        // Let us remove all Kerberos credentials stored in the Subject
         Iterator<Object> it = subject.getPrivateCredentials().iterator();
         while (it.hasNext()) {
             Object o = it.next();

src/jdk.security.auth/share/classes/com/sun/security/auth/module/LdapLoginModule.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2005, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2005, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -696,8 +696,12 @@ public boolean logout() throws LoginException {
             throw new LoginException ("Subject is read-only");
         }
         Set<Principal> principals = subject.getPrincipals();
-        principals.remove(ldapPrincipal);
-        principals.remove(userPrincipal);
+        if (ldapPrincipal != null) {
+            principals.remove(ldapPrincipal);
+        }
+        if (userPrincipal != null) {
+            principals.remove(userPrincipal);
+        }
         if (authzIdentity != null) {
             principals.remove(authzPrincipal);
         }

src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -349,29 +349,30 @@ public boolean logout() throws LoginException {
             throw new LoginException ("Subject is ReadOnly");
         }
         Set<Principal> principals = subject.getPrincipals();
-        if (principals.contains(userPrincipal)) {
+        if (userPrincipal != null && principals.contains(userPrincipal)) {
             principals.remove(userPrincipal);
         }
-        if (principals.contains(userSID)) {
+        if (userSID != null && principals.contains(userSID)) {
             principals.remove(userSID);
         }
-        if (principals.contains(userDomain)) {
+        if (userDomain != null && principals.contains(userDomain)) {
             principals.remove(userDomain);
         }
-        if (principals.contains(domainSID)) {
+        if (domainSID != null && principals.contains(domainSID)) {
             principals.remove(domainSID);
         }
-        if (principals.contains(primaryGroup)) {
+        if (primaryGroup != null && principals.contains(primaryGroup)) {
             principals.remove(primaryGroup);
         }
-        for (int i = 0; groups != null && i < groups.length; i++) {
-            if (principals.contains(groups[i])) {
-                principals.remove(groups[i]);
+        if (groups != null) {
+            for (NTSidGroupPrincipal gp : groups) {
+                // gp is never null
+                principals.remove(gp);
             }
         }
 
         Set<Object> pubCreds = subject.getPublicCredentials();
-        if (pubCreds.contains(iToken)) {
+        if (iToken != null && pubCreds.contains(iToken)) {
             pubCreds.remove(iToken);
         }
 

src/jdk.security.auth/share/classes/com/sun/security/auth/module/UnixLoginModule.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2022, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -277,11 +277,18 @@ public boolean logout() throws LoginException {
                     ("logout Failed: Subject is Readonly");
             }
         // remove the added Principals from the Subject
-        subject.getPrincipals().remove(userPrincipal);
-        subject.getPrincipals().remove(UIDPrincipal);
-        subject.getPrincipals().remove(GIDPrincipal);
-        for (int i = 0; i < supplementaryGroups.size(); i++) {
-            subject.getPrincipals().remove(supplementaryGroups.get(i));
+        if (userPrincipal != null) {
+            subject.getPrincipals().remove(userPrincipal);
+        }
+        if (UIDPrincipal != null) {
+            subject.getPrincipals().remove(UIDPrincipal);
+        }
+        if (GIDPrincipal != null) {
+            subject.getPrincipals().remove(GIDPrincipal);
+        }
+        for (UnixNumericGroupPrincipal gp : supplementaryGroups) {
+            // gp is never null
+            subject.getPrincipals().remove(gp);
         }
 
         // clean out state