From b4a63aad7f56486c164a15ae2477bcd251b0bb1b Mon Sep 17 00:00:00 2001
From: James Moger <james.moger@gitblit.com>
Date: Tue, 18 Mar 2014 21:10:48 -0400
Subject: Fix authentication security hole with external providers
---
.../com/gitblit/manager/AuthenticationManager.java | 73 +++++++------
.../gitblit/tests/HtpasswdAuthenticationTest.java | 116 +++++++++++++++++++++
.../com/gitblit/tests/LdapAuthenticationTest.java | 36 +++++++
.../gitblit/tests/RedmineAuthenticationTest.java | 27 +++--
4 files changed, 212 insertions(+), 40 deletions(-)
(limited to 'src')
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<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()) {
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<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));
}
}
--
cgit v1.2.3