From b4a63aad7f56486c164a15ae2477bcd251b0bb1b Mon Sep 17 00:00:00 2001 From: James Moger Date: Tue, 18 Mar 2014 21:10:48 -0400 Subject: [PATCH] Fix authentication security hole with external providers --- .../manager/AuthenticationManager.java | 73 ++++++----- .../tests/HtpasswdAuthenticationTest.java | 116 ++++++++++++++++++ .../gitblit/tests/LdapAuthenticationTest.java | 36 ++++++ .../tests/RedmineAuthenticationTest.java | 27 ++-- 4 files changed, 212 insertions(+), 40 deletions(-) diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index ad4a9851..4f3e652c 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -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); } /** diff --git a/src/test/java/com/gitblit/tests/HtpasswdAuthenticationTest.java b/src/test/java/com/gitblit/tests/HtpasswdAuthenticationTest.java index 3b1d51e1..4e1c3ac1 100644 --- a/src/test/java/com/gitblit/tests/HtpasswdAuthenticationTest.java +++ b/src/test/java/com/gitblit/tests/HtpasswdAuthenticationTest.java @@ -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() { diff --git a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java index e4dc2db2..b037754c 100644 --- a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java +++ b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java @@ -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 backingMap = new HashMap(); @@ -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()) { diff --git a/src/test/java/com/gitblit/tests/RedmineAuthenticationTest.java b/src/test/java/com/gitblit/tests/RedmineAuthenticationTest.java index 1fe8459f..6ede8313 100644 --- a/src/test/java/com/gitblit/tests/RedmineAuthenticationTest.java +++ b/src/test/java/com/gitblit/tests/RedmineAuthenticationTest.java @@ -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()); } @@ -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)); } } -- 2.39.5