8282730: LdapLoginModule throw NPE from logout method after login failure

Reviewed-by: mullan

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
+     * also prohibit null elements, and attempts to add, query, or remove
-     * element will result in a {@code NullPointerException}.
+     * 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 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
+     * also prohibit null elements, and attempts to add, query, or remove
-     * element will result in a {@code NullPointerException}.
+     * 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 @@ public class LoginContext {
         // - 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 @@ public class LoginContext {
                         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 @@ public class LoginContext {
                     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 @@ public class LoginContext {
                     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 @@ public class LoginContext {
                 } 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 @@ import java.util.Map;
  * 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 @@ import java.util.Map;
 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 @@ public interface LoginModule {
      * {@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
+     * @return {@code true} if the authentication succeeded, or {@code false}
-     *                  {@code LoginModule} should be ignored.
+     *                  if this {@code LoginModule} should be ignored.
      */
     boolean login() throws LoginException;
 
@@ -190,8 +189,8 @@ public interface LoginModule {
      *
      * @exception LoginException if the commit fails
      *
-     * @return true if this method succeeded, or false if this
+     * @return {@code true} if this method succeeded, or {@code false}
-     *                  {@code LoginModule} should be ignored.
+     *                  if this {@code LoginModule} should be ignored.
      */
     boolean commit() throws LoginException;
 
@@ -210,8 +209,8 @@ public interface LoginModule {
      *
      * @exception LoginException if the abort fails
      *
-     * @return true if this method succeeded, or false if this
+     * @return {@code true} if this method succeeded, or {@code false}
-     *                  {@code LoginModule} should be ignored.
+     *                  if this {@code LoginModule} should be ignored.
      */
     boolean abort() throws LoginException;
 
@@ -223,8 +222,15 @@ public interface LoginModule {
      *
      * @exception LoginException if the logout fails
      *
-     * @return true if this method succeeded, or false if this
+     * @return {@code true} if this method succeeded, or {@code false}
-     *                  {@code LoginModule} should be ignored.
+     *                  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 class FileLoginModule implements LoginModule {
             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
@@ -471,11 +471,18 @@ public class JndiLoginModule implements LoginModule {
             cleanState();
             throw new LoginException ("Subject is Readonly");
         }
-        subject.getPrincipals().remove(userPrincipal);
+        if (userPrincipal != null) {
-        subject.getPrincipals().remove(UIDPrincipal);
+            subject.getPrincipals().remove(userPrincipal);
-        subject.getPrincipals().remove(GIDPrincipal);
+        }
-        for (int i = 0; i < supplementaryGroups.size(); i++) {
+        if (UIDPrincipal != null) {
-            subject.getPrincipals().remove(supplementaryGroups.get(i));
+            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

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

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

@@ -1202,8 +1202,10 @@ public class Krb5LoginModule implements LoginModule {
             throw new LoginException("Subject is Readonly");
         }
 
-        subject.getPrincipals().remove(kerbClientPrinc);
+        if (kerbClientPrinc != null) {
-           // Let us remove all Kerberos credentials stored in the Subject
+            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 class LdapLoginModule implements LoginModule {
             throw new LoginException ("Subject is read-only");
         }
         Set<Principal> principals = subject.getPrincipals();
-        principals.remove(ldapPrincipal);
+        if (ldapPrincipal != null) {
-        principals.remove(userPrincipal);
+            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 class NTLoginModule implements LoginModule {
             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 (groups != null) {
-            if (principals.contains(groups[i])) {
+            for (NTSidGroupPrincipal gp : groups) {
-                principals.remove(groups[i]);
+                // 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 class UnixLoginModule implements LoginModule {
                     ("logout Failed: Subject is Readonly");
             }
         // remove the added Principals from the Subject
-        subject.getPrincipals().remove(userPrincipal);
+        if (userPrincipal != null) {
-        subject.getPrincipals().remove(UIDPrincipal);
+            subject.getPrincipals().remove(userPrincipal);
-        subject.getPrincipals().remove(GIDPrincipal);
+        }
-        for (int i = 0; i < supplementaryGroups.size(); i++) {
+        if (UIDPrincipal != null) {
-            subject.getPrincipals().remove(supplementaryGroups.get(i));
+            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

test/jdk/javax/security/auth/login/modules/SafeLogout.java

@@ -0,0 +1,118 @@
+/*
+ * Copyright (c) 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import javax.security.auth.Subject;
+import javax.security.auth.login.AppConfigurationEntry;
+import javax.security.auth.login.Configuration;
+import javax.security.auth.login.LoginContext;
+import javax.security.auth.login.LoginException;
+import java.util.*;
+
+import static javax.security.auth.login.AppConfigurationEntry.LoginModuleControlFlag.*;
+
+/*
+ * @test
+ * @bug 8282730
+ * @key randomness
+ * @summary Check that all LoginModule implementations don't throw NPE
+ *          from logout method after login failure
+ * @modules jdk.security.auth
+ *          java.management
+ */
+public class SafeLogout {
+
+    static Random r = new Random();
+
+    public static void main(String[] args) throws Exception {
+        for (int i = 0; i < 100; i++) {
+            test(i);
+        }
+    }
+
+    static void test(int pos) throws Exception {
+        // The content of the principals and credentials sets depends on
+        // combinations of (possibly multiple) login modules configurations,
+        // and it is difficult to find only a "typical" subset to test on,
+        // Therefore we use a random number to choose login module names,
+        // flag for each, and whether to perform a login at the beginning.
+        // Each config is printed out so that any failure can be reproduced.
+        boolean login = r.nextBoolean();
+        Map<String, ?> empty = Collections.emptyMap();
+        AppConfigurationEntry[] result = new AppConfigurationEntry[r.nextInt(4) + 1];
+        for (int i = 0; i < result.length; i++) {
+            result[i] = new AppConfigurationEntry(randomModule(), randomControl(), empty);
+        }
+
+        System.out.println(pos + " " + login);
+        Arrays.stream(result)
+                .forEach(a -> System.out.println(a.getLoginModuleName() + ":" + a.getControlFlag()));
+
+        LoginContext lc = new LoginContext("a", new Subject(), null, new Configuration() {
+            @Override
+            public AppConfigurationEntry[] getAppConfigurationEntry(String name) {
+                return result;
+            }
+        });
+
+        try {
+            if (login) {
+                lc.login();
+            }
+        } catch (LoginException e) {
+            // Don't care
+        } finally {
+            try {
+                lc.logout();
+            } catch (LoginException le) {
+                if (!le.getMessage().contains("all modules ignored")) {
+                    throw le;
+                }
+            }
+        }
+    }
+
+    static AppConfigurationEntry.LoginModuleControlFlag[] allControls = {
+            REQUIRED,
+            REQUISITE,
+            SUFFICIENT,
+            OPTIONAL
+    };
+
+    static AppConfigurationEntry.LoginModuleControlFlag randomControl() {
+        return allControls[r.nextInt(allControls.length)];
+    }
+
+    static String[] allModules = {
+            "com.sun.security.auth.module.Krb5LoginModule",
+            "com.sun.security.auth.module.UnixLoginModule",
+            "com.sun.security.auth.module.JndiLoginModule",
+            "com.sun.security.auth.module.KeyStoreLoginModule",
+            "com.sun.security.auth.module.NTLoginModule",
+            "com.sun.security.auth.module.LdapLoginModule",
+            "com.sun.jmx.remote.security.FileLoginModule"
+    };
+
+    static String randomModule() {
+        return allModules[r.nextInt(allModules.length)];
+    }
+}