]> source.dussan.org Git - gitblit.git/commitdiff
Fix authentication security hole with external providers 35/35/1
authorJames Moger <james.moger@gitblit.com>
Wed, 19 Mar 2014 01:10:48 +0000 (21:10 -0400)
committerJames Moger <james.moger@gitblit.com>
Wed, 19 Mar 2014 01:10:48 +0000 (21:10 -0400)
src/main/java/com/gitblit/manager/AuthenticationManager.java
src/test/java/com/gitblit/tests/HtpasswdAuthenticationTest.java
src/test/java/com/gitblit/tests/LdapAuthenticationTest.java
src/test/java/com/gitblit/tests/RedmineAuthenticationTest.java

index ad4a9851adaccde3ffe4ce8f8fdca155951ffd40..4f3e652c58f6a82880177b3c1a3bc5abf7675af5 100644 (file)
@@ -159,6 +159,10 @@ public class AuthenticationManager implements IAuthenticationManager {
                }
                return this;
        }
+       
+       public void addAuthenticationProvider(AuthenticationProvider prov) {
+               authenticationProviders.add(prov);
+       }
 
        /**
         * Authenticate a user based on HTTP request parameters.
@@ -341,41 +345,52 @@ public class AuthenticationManager implements IAuthenticationManager {
 
                // try local authentication
                if (user != null && user.isLocalAccount()) {
-                       UserModel returnedUser = null;
-                       if (user.password.startsWith(StringUtils.MD5_TYPE)) {
-                               // password digest
-                               String md5 = StringUtils.MD5_TYPE + StringUtils.getMD5(new String(password));
-                               if (user.password.equalsIgnoreCase(md5)) {
-                                       returnedUser = user;
-                               }
-                       } else if (user.password.startsWith(StringUtils.COMBINED_MD5_TYPE)) {
-                               // username+password digest
-                               String md5 = StringUtils.COMBINED_MD5_TYPE
-                                               + StringUtils.getMD5(username.toLowerCase() + new String(password));
-                               if (user.password.equalsIgnoreCase(md5)) {
-                                       returnedUser = user;
-                               }
-                       } else if (user.password.equals(new String(password))) {
-                               // plain-text password
-                               returnedUser = user;
-                       }
-                       return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS);
+                       return authenticateLocal(user, password);
                }
 
                // try registered external authentication providers
-               if (user == null) {
-                       for (AuthenticationProvider provider : authenticationProviders) {
-                               if (provider instanceof UsernamePasswordAuthenticationProvider) {
-                                       user = provider.authenticate(usernameDecoded, password);
-                                       if (user != null) {
-                                               // user authenticated
-                                               user.accountType = provider.getAccountType();
-                                               return validateAuthentication(user, AuthenticationType.CREDENTIALS);
-                                       }
+               for (AuthenticationProvider provider : authenticationProviders) {
+                       if (provider instanceof UsernamePasswordAuthenticationProvider) {
+                               UserModel returnedUser = provider.authenticate(usernameDecoded, password);
+                               if (returnedUser != null) {
+                                       // user authenticated
+                                       returnedUser.accountType = provider.getAccountType();
+                                       return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS);
                                }
                        }
                }
-               return validateAuthentication(user, AuthenticationType.CREDENTIALS);
+               
+               // could not authenticate locally or with a provider
+               return null;
+       }
+       
+       /**
+        * Returns a UserModel if local authentication succeeds.
+        * 
+        * @param user
+        * @param password
+        * @return a UserModel if local authentication succeeds, null otherwise
+        */
+       protected UserModel authenticateLocal(UserModel user, char [] password) {
+               UserModel returnedUser = null;
+               if (user.password.startsWith(StringUtils.MD5_TYPE)) {
+                       // password digest
+                       String md5 = StringUtils.MD5_TYPE + StringUtils.getMD5(new String(password));
+                       if (user.password.equalsIgnoreCase(md5)) {
+                               returnedUser = user;
+                       }
+               } else if (user.password.startsWith(StringUtils.COMBINED_MD5_TYPE)) {
+                       // username+password digest
+                       String md5 = StringUtils.COMBINED_MD5_TYPE
+                                       + StringUtils.getMD5(user.username.toLowerCase() + new String(password));
+                       if (user.password.equalsIgnoreCase(md5)) {
+                               returnedUser = user;
+                       }
+               } else if (user.password.equals(new String(password))) {
+                       // plain-text password
+                       returnedUser = user;
+               }
+               return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS);
        }
 
        /**
index 3b1d51e12da59c18f952d5c6c491593b84edc565..4e1c3ac154d1ec4891f4226fde93881078a3a017 100644 (file)
@@ -27,6 +27,7 @@ import org.junit.Test;
 
 import com.gitblit.IStoredSettings;
 import com.gitblit.auth.HtpasswdAuthProvider;
+import com.gitblit.manager.AuthenticationManager;
 import com.gitblit.manager.RuntimeManager;
 import com.gitblit.manager.UserManager;
 import com.gitblit.models.UserModel;
@@ -47,6 +48,7 @@ public class HtpasswdAuthenticationTest extends GitblitUnitTest {
 
     private HtpasswdAuthProvider htpasswd;
 
+       private AuthenticationManager auth;
 
     private MemorySettings getSettings(String userfile, String groupfile, Boolean overrideLA)
     {
@@ -68,6 +70,7 @@ public class HtpasswdAuthenticationTest extends GitblitUnitTest {
     private void setupUS()
     {
         htpasswd = newHtpasswdAuthentication(getSettings());
+        auth = newAuthenticationManager(getSettings());
     }
 
     private HtpasswdAuthProvider newHtpasswdAuthentication(IStoredSettings settings) {
@@ -77,6 +80,16 @@ public class HtpasswdAuthenticationTest extends GitblitUnitTest {
        htpasswd.setup(runtime, users);
        return htpasswd;
     }
+    
+    private AuthenticationManager newAuthenticationManager(IStoredSettings settings) {
+       RuntimeManager runtime = new RuntimeManager(settings, GitBlitSuite.BASEFOLDER).start();
+       UserManager users = new UserManager(runtime).start();
+       HtpasswdAuthProvider htpasswd = new HtpasswdAuthProvider();
+       htpasswd.setup(runtime, users);
+       AuthenticationManager auth = new AuthenticationManager(runtime, users);
+       auth.addAuthenticationProvider(htpasswd);
+       return auth;
+    }
 
 
     private void copyInFiles() throws IOException
@@ -178,6 +191,52 @@ public class HtpasswdAuthenticationTest extends GitblitUnitTest {
         assertEquals("leading", user.username);
     }
 
+    
+    @Test
+    public void testAuthenticationManager()
+    {
+        MS.put(KEY_SUPPORT_PLAINTEXT_PWD, "true");
+        UserModel user = auth.authenticate("user1", "pass1".toCharArray());
+        assertNotNull(user);
+        assertEquals("user1", user.username);
+
+        user = auth.authenticate("user2", "pass2".toCharArray());
+        assertNotNull(user);
+        assertEquals("user2", user.username);
+
+        // Test different encryptions
+        user = auth.authenticate("plain", "passWord".toCharArray());
+        assertNotNull(user);
+        assertEquals("plain", user.username);
+
+        MS.put(KEY_SUPPORT_PLAINTEXT_PWD, "false");
+        user = auth.authenticate("crypt", "password".toCharArray());
+        assertNotNull(user);
+        assertEquals("crypt", user.username);
+
+        user = auth.authenticate("md5", "password".toCharArray());
+        assertNotNull(user);
+        assertEquals("md5", user.username);
+
+        user = auth.authenticate("sha", "password".toCharArray());
+        assertNotNull(user);
+        assertEquals("sha", user.username);
+
+
+        // Test leading and trailing whitespace
+        user = auth.authenticate("trailing", "whitespace".toCharArray());
+        assertNotNull(user);
+        assertEquals("trailing", user.username);
+
+        user = auth.authenticate("tabbed", "frontAndBack".toCharArray());
+        assertNotNull(user);
+        assertEquals("tabbed", user.username);
+
+        user = auth.authenticate("leading", "whitespace".toCharArray());
+        assertNotNull(user);
+        assertEquals("leading", user.username);
+    }
+
 
     @Test
     public void testAttributes()
@@ -255,6 +314,63 @@ public class HtpasswdAuthenticationTest extends GitblitUnitTest {
     }
 
 
+    @Test
+    public void testAuthenticationMangerDenied()
+    {
+        UserModel user = null;
+        MS.put(KEY_SUPPORT_PLAINTEXT_PWD, "true");
+        user = auth.authenticate("user1", "".toCharArray());
+        assertNull("User 'user1' falsely authenticated.", user);
+
+        user = auth.authenticate("user1", "pass2".toCharArray());
+        assertNull("User 'user1' falsely authenticated.", user);
+
+        user = auth.authenticate("user2", "lalala".toCharArray());
+        assertNull("User 'user2' falsely authenticated.", user);
+
+
+        user = auth.authenticate("user3", "disabled".toCharArray());
+        assertNull("User 'user3' falsely authenticated.", user);
+
+        user = auth.authenticate("user4", "disabled".toCharArray());
+        assertNull("User 'user4' falsely authenticated.", user);
+
+
+        user = auth.authenticate("plain", "text".toCharArray());
+        assertNull("User 'plain' falsely authenticated.", user);
+
+        user = auth.authenticate("plain", "password".toCharArray());
+        assertNull("User 'plain' falsely authenticated.", user);
+
+
+        MS.put(KEY_SUPPORT_PLAINTEXT_PWD, "false");
+
+        user = auth.authenticate("crypt", "".toCharArray());
+        assertNull("User 'cyrpt' falsely authenticated.", user);
+
+        user = auth.authenticate("crypt", "passwd".toCharArray());
+        assertNull("User 'crypt' falsely authenticated.", user);
+
+        user = auth.authenticate("md5", "".toCharArray());
+        assertNull("User 'md5' falsely authenticated.", user);
+
+        user = auth.authenticate("md5", "pwd".toCharArray());
+        assertNull("User 'md5' falsely authenticated.", user);
+
+        user = auth.authenticate("sha", "".toCharArray());
+        assertNull("User 'sha' falsely authenticated.", user);
+
+        user = auth.authenticate("sha", "letmein".toCharArray());
+        assertNull("User 'sha' falsely authenticated.", user);
+
+
+        user = auth.authenticate("  tabbed", "frontAndBack".toCharArray());
+        assertNull("User 'tabbed' falsely authenticated.", user);
+
+        user = auth.authenticate("    leading", "whitespace".toCharArray());
+        assertNull("User 'leading' falsely authenticated.", user);
+    }
+
     @Test
     public void testCleartextIntrusion()
     {
index e4dc2db2036e7ed3d5dc83d69f20c5dab30a8e56..b037754c8902d3caa25b639120c8ef7b60114904 100644 (file)
@@ -32,6 +32,7 @@ import com.gitblit.Constants.AccountType;
 import com.gitblit.IStoredSettings;
 import com.gitblit.Keys;
 import com.gitblit.auth.LdapAuthProvider;
+import com.gitblit.manager.AuthenticationManager;
 import com.gitblit.manager.IUserManager;
 import com.gitblit.manager.RuntimeManager;
 import com.gitblit.manager.UserManager;
@@ -67,6 +68,8 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
        private static InMemoryDirectoryServer ds;
 
        private IUserManager userManager;
+       
+       private AuthenticationManager auth;
 
        private MemorySettings settings;
 
@@ -89,6 +92,7 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
                FileUtils.copyFile(new File(RESOURCE_DIR + "users.conf"), usersConf);
                settings = getSettings();
                ldap = newLdapAuthentication(settings);
+               auth = newAuthenticationManager(settings);
        }
 
        private LdapAuthProvider newLdapAuthentication(IStoredSettings settings) {
@@ -98,6 +102,13 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
                ldap.setup(runtime, userManager);
                return ldap;
        }
+       
+       private AuthenticationManager newAuthenticationManager(IStoredSettings settings) {
+               RuntimeManager runtime = new RuntimeManager(settings, GitBlitSuite.BASEFOLDER).start();
+               AuthenticationManager auth = new AuthenticationManager(runtime, userManager);
+               auth.addAuthenticationProvider(newLdapAuthentication(settings));
+               return auth;
+       }
 
        private MemorySettings getSettings() {
                Map<String, Object> backingMap = new HashMap<String, Object>();
@@ -223,6 +234,31 @@ public class LdapAuthenticationTest extends GitblitUnitTest {
                assertEquals("Number of ldap groups in gitblit team model", 1, countLdapTeamsInUserManager());
        }
 
+       @Test
+       public void testAuthenticationManager() {
+               UserModel userOneModel = auth.authenticate("UserOne", "userOnePassword".toCharArray());
+               assertNotNull(userOneModel);
+               assertNotNull(userOneModel.getTeam("git_admins"));
+               assertNotNull(userOneModel.getTeam("git_users"));
+               assertTrue(userOneModel.canAdmin);
+
+               UserModel userOneModelFailedAuth = auth.authenticate("UserOne", "userTwoPassword".toCharArray());
+               assertNull(userOneModelFailedAuth);
+
+               UserModel userTwoModel = auth.authenticate("UserTwo", "userTwoPassword".toCharArray());
+               assertNotNull(userTwoModel);
+               assertNotNull(userTwoModel.getTeam("git_users"));
+               assertNull(userTwoModel.getTeam("git_admins"));
+               assertNotNull(userTwoModel.getTeam("git admins"));
+               assertTrue(userTwoModel.canAdmin);
+
+               UserModel userThreeModel = auth.authenticate("UserThree", "userThreePassword".toCharArray());
+               assertNotNull(userThreeModel);
+               assertNotNull(userThreeModel.getTeam("git_users"));
+               assertNull(userThreeModel.getTeam("git_admins"));
+               assertTrue(userThreeModel.canAdmin);
+       }
+
        private int countLdapUsersInUserManager() {
                int ldapAccountCount = 0;
                for (UserModel userModel : userManager.getAllUsers()) {
index 1fe8459fe52ed1d70e984e8ed790efe4b07a83c6..6ede8313d5d24c51ea1e85b8a363da93ccccfc0b 100644 (file)
@@ -8,6 +8,7 @@ import org.junit.Test;
 
 import com.gitblit.IStoredSettings;
 import com.gitblit.auth.RedmineAuthProvider;
+import com.gitblit.manager.AuthenticationManager;
 import com.gitblit.manager.RuntimeManager;
 import com.gitblit.manager.UserManager;
 import com.gitblit.models.UserModel;
@@ -19,10 +20,6 @@ public class RedmineAuthenticationTest extends GitblitUnitTest {
         + "\"last_login_on\":\"2012-09-06T23:59:26Z\",\"firstname\":\"baz\","
         + "\"id\":4,\"login\":\"RedmineUserId\",\"mail\":\"baz@example.com\"}}";
 
-    private static final String NOT_ADMIN_JSON = "{\"user\":{\"lastname\":\"foo\","
-        + "\"last_login_on\":\"2012-09-08T13:59:01Z\",\"created_on\":\"2009-03-17T14:25:50Z\","
-        + "\"mail\":\"baz@example.com\",\"id\":5,\"firstname\":\"baz\"}}";
-
     MemorySettings getSettings() {
        return new MemorySettings(new HashMap<String, Object>());
     }
@@ -38,6 +35,17 @@ public class RedmineAuthenticationTest extends GitblitUnitTest {
     RedmineAuthProvider newRedmineAuthentication() {
        return newRedmineAuthentication(getSettings());
     }
+    
+    AuthenticationManager newAuthenticationManager() {
+       RuntimeManager runtime = new RuntimeManager(getSettings(), GitBlitSuite.BASEFOLDER).start();
+       UserManager users = new UserManager(runtime).start();
+       RedmineAuthProvider redmine = new RedmineAuthProvider();
+       redmine.setup(runtime, users);
+        redmine.setTestingCurrentUserAsJson(JSON);
+       AuthenticationManager auth = new AuthenticationManager(runtime, users);
+       auth.addAuthenticationProvider(redmine);
+       return auth;
+    }
 
     @Test
     public void testAuthenticate() throws Exception {
@@ -48,18 +56,15 @@ public class RedmineAuthenticationTest extends GitblitUnitTest {
         assertThat(userModel.getDisplayName(), is("baz foo"));
         assertThat(userModel.emailAddress, is("baz@example.com"));
         assertNotNull(userModel.cookie);
-        assertThat(userModel.canAdmin, is(true));
     }
 
     @Test
-    public void testAuthenticateNotAdminUser() throws Exception {
-       RedmineAuthProvider redmine = newRedmineAuthentication();
-        redmine.setTestingCurrentUserAsJson(NOT_ADMIN_JSON);
-        UserModel userModel = redmine.authenticate("RedmineUserId", "RedmineAPIKey".toCharArray());
-        assertThat(userModel.getName(), is("redmineuserid"));
+    public void testAuthenticationManager() throws Exception {
+       AuthenticationManager auth = newAuthenticationManager();
+        UserModel userModel = auth.authenticate("RedmineAdminId", "RedmineAPIKey".toCharArray());
+        assertThat(userModel.getName(), is("redmineadminid"));
         assertThat(userModel.getDisplayName(), is("baz foo"));
         assertThat(userModel.emailAddress, is("baz@example.com"));
         assertNotNull(userModel.cookie);
-        assertThat(userModel.canAdmin, is(false));
     }
 }