prevent session fixation for external authentication

  + use request instead of session to flag authentication status
    and user, for external authentication types

4 files changed, 72 insertions, 43 deletions

diff --git a/src/main/java/com/gitblit/Constants.java b/src/main/java/com/gitblit/Constants.java
index 8b05f895..787d7267 100644
--- a/src/main/java/com/gitblit/Constants.java
+++ b/src/main/java/com/gitblit/Constants.java
@@ -137,7 +137,9 @@ public class Constants {
 

 	public static final String DEVELOP = "develop";

 

-	public static final String AUTHENTICATION_TYPE = "authentication-type";

+	public static final String ATTRIB_AUTHTYPE = NAME + ":authentication-type";

+

+	public static final String ATTRIB_AUTHUSER = NAME + ":authenticated-user";

 

 	public static String getVersion() {

 		String v = Constants.class.getPackage().getImplementationVersion();

diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java
index 38e45a6f..51aa2213 100644
--- a/src/main/java/com/gitblit/manager/AuthenticationManager.java
+++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java
@@ -194,6 +194,14 @@ public class AuthenticationManager implements IAuthenticationManager {
 	 */
 	@Override
 	public UserModel authenticate(HttpServletRequest httpRequest, boolean requiresCertificate) {
+
+		// Check if this request has already been authenticated, and trust that instead of re-processing
+		String reqAuthUser = (String) httpRequest.getAttribute(Constants.ATTRIB_AUTHUSER);
+		if (!StringUtils.isEmpty(reqAuthUser)) {
+			logger.warn("Called servlet authenticate when request is already authenticated.");
+			return userManager.getUserModel(reqAuthUser);
+		}
+
 		// try to authenticate by servlet container principal
 		if (!requiresCertificate) {
 			Principal principal = httpRequest.getUserPrincipal();
@@ -204,7 +212,7 @@ public class AuthenticationManager implements IAuthenticationManager {
 					UserModel user = userManager.getUserModel(username);
 					if (user != null) {
 						// existing user
-						flagSession(httpRequest, AuthenticationType.CONTAINER);
+						flagRequest(httpRequest, AuthenticationType.CONTAINER, user.username);
 						logger.debug(MessageFormat.format("{0} authenticated by servlet container principal from {1}",
 								user.username, httpRequest.getRemoteAddr()));
 						return validateAuthentication(user, AuthenticationType.CONTAINER);
@@ -239,7 +247,7 @@ public class AuthenticationManager implements IAuthenticationManager {
 						}
 						
 						userManager.updateUserModel(user);
-						flagSession(httpRequest, AuthenticationType.CONTAINER);
+						flagRequest(httpRequest, AuthenticationType.CONTAINER, user.username);
 						logger.debug(MessageFormat.format("{0} authenticated and created by servlet container principal from {1}",
 								user.username, httpRequest.getRemoteAddr()));
 						return validateAuthentication(user, AuthenticationType.CONTAINER);
@@ -260,7 +268,7 @@ public class AuthenticationManager implements IAuthenticationManager {
 			UserModel user = userManager.getUserModel(model.username);
 			X509Metadata metadata = HttpUtils.getCertificateMetadata(httpRequest);
 			if (user != null) {
-				flagSession(httpRequest, AuthenticationType.CERTIFICATE);
+				flagRequest(httpRequest, AuthenticationType.CERTIFICATE, user.username);
 				logger.debug(MessageFormat.format("{0} authenticated by client certificate {1} from {2}",
 						user.username, metadata.serialNumber, httpRequest.getRemoteAddr()));
 				return validateAuthentication(user, AuthenticationType.CERTIFICATE);
@@ -282,7 +290,7 @@ public class AuthenticationManager implements IAuthenticationManager {
 		if (!StringUtils.isEmpty(cookie)) {
 			user = userManager.getUserModel(cookie.toCharArray());
 			if (user != null) {
-				flagSession(httpRequest, AuthenticationType.COOKIE);
+				flagRequest(httpRequest, AuthenticationType.COOKIE, user.username);
 				logger.debug(MessageFormat.format("{0} authenticated by cookie from {1}",
 					user.username, httpRequest.getRemoteAddr()));
 				return validateAuthentication(user, AuthenticationType.COOKIE);
@@ -304,7 +312,7 @@ public class AuthenticationManager implements IAuthenticationManager {
 				char[] password = values[1].toCharArray();
 				user = authenticate(username, password);
 				if (user != null) {
-					flagSession(httpRequest, AuthenticationType.CREDENTIALS);
+					flagRequest(httpRequest, AuthenticationType.CREDENTIALS, user.username);
 					logger.debug(MessageFormat.format("{0} authenticated by BASIC request header from {1}",
 							user.username, httpRequest.getRemoteAddr()));
 					return validateAuthentication(user, AuthenticationType.CREDENTIALS);
@@ -423,8 +431,9 @@ public class AuthenticationManager implements IAuthenticationManager {
 		return user;
 	}
 
-	protected void flagSession(HttpServletRequest httpRequest, AuthenticationType authenticationType) {
-		httpRequest.getSession().setAttribute(Constants.AUTHENTICATION_TYPE, authenticationType);
+	protected void flagRequest(HttpServletRequest httpRequest, AuthenticationType authenticationType, String authedUsername) {
+		httpRequest.setAttribute(Constants.ATTRIB_AUTHUSER,  authedUsername);
+		httpRequest.setAttribute(Constants.ATTRIB_AUTHTYPE,  authenticationType);
 	}
 
 	/**
@@ -545,9 +554,15 @@ public class AuthenticationManager implements IAuthenticationManager {
 	@Override
 	public void setCookie(HttpServletRequest request, HttpServletResponse response, UserModel user) {
 		if (settings.getBoolean(Keys.web.allowCookieAuthentication, true)) {
-			HttpSession session = request.getSession();
-			AuthenticationType authenticationType = (AuthenticationType) session.getAttribute(Constants.AUTHENTICATION_TYPE);
-			boolean standardLogin = authenticationType.isStandard();
+			boolean standardLogin = true;
+
+			if (null != request) {
+				// Pull the auth type from the request, it is set there if container managed
+				AuthenticationType authenticationType = (AuthenticationType) request.getAttribute(Constants.ATTRIB_AUTHTYPE);
+
+				if (null != authenticationType)
+					standardLogin = authenticationType.isStandard();
+			}
 
 			if (standardLogin) {
 				Cookie userCookie;
diff --git a/src/main/java/com/gitblit/wicket/pages/RootPage.java b/src/main/java/com/gitblit/wicket/pages/RootPage.java
index 6d7ebd85..61d7b759 100644
--- a/src/main/java/com/gitblit/wicket/pages/RootPage.java
+++ b/src/main/java/com/gitblit/wicket/pages/RootPage.java
@@ -279,7 +279,7 @@ public abstract class RootPage extends BasePage {
 
 			request = ((WebRequest) getRequest()).getHttpServletRequest();
 			response = ((WebResponse) getResponse()).getHttpServletResponse();
-			request.getSession().setAttribute(Constants.AUTHENTICATION_TYPE, AuthenticationType.CREDENTIALS);
+			request.getSession().setAttribute(Constants.ATTRIB_AUTHTYPE, AuthenticationType.CREDENTIALS);
 
 			// Set Cookie
 			app().authentication().setCookie(request, response, user);
@@ -608,8 +608,8 @@ public abstract class RootPage extends BasePage {
 			UserModel user = session.getUser();
 			boolean editCredentials = app().authentication().supportsCredentialChanges(user);
 			HttpServletRequest request = ((WebRequest) getRequest()).getHttpServletRequest();
-			AuthenticationType authenticationType = (AuthenticationType) request.getSession().getAttribute(Constants.AUTHENTICATION_TYPE);
-			boolean standardLogin = authenticationType.isStandard();
+			AuthenticationType authenticationType = (AuthenticationType) request.getAttribute(Constants.ATTRIB_AUTHTYPE);
+			boolean standardLogin = (null != authenticationType) ? authenticationType.isStandard() : true;
 
 			if (app().settings().getBoolean(Keys.web.allowGravatar, true)) {
 				add(new AvatarImage("username", user, "navbarGravatar", 20, false));
diff --git a/src/main/java/com/gitblit/wicket/pages/SessionPage.java b/src/main/java/com/gitblit/wicket/pages/SessionPage.java
index af7f2115..bcf8e978 100644
--- a/src/main/java/com/gitblit/wicket/pages/SessionPage.java
+++ b/src/main/java/com/gitblit/wicket/pages/SessionPage.java
@@ -56,34 +56,50 @@ public abstract class SessionPage extends WebPage {
 		HttpServletRequest request = ((WebRequest) getRequest()).getHttpServletRequest();
 		HttpServletResponse response = ((WebResponse) getResponse()).getHttpServletResponse();
 
-		if (session.isLoggedIn() && !session.isSessionInvalidated()) {
-			// already have a session, refresh usermodel to pick up
-			// any changes to permissions or roles (issue-186)
-			UserModel user = app().users().getUserModel(session.getUser().username);
-
-			if (user == null || user.disabled) {
-				// user was deleted/disabled during session
-				app().authentication().logout(request, response, user);
-				session.setUser(null);
-				session.invalidateNow();
-				return;
+		// If using container/external servlet authentication, use request attribute
+		String authedUser = (String) request.getAttribute(Constants.ATTRIB_AUTHUSER);
+
+		// Default to trusting session authentication if not set in request by external processing
+		if (StringUtils.isEmpty(authedUser) && session.isLoggedIn()) {
+			authedUser = session.getUsername();
+		}
+
+		if (!StringUtils.isEmpty(authedUser)) {
+			// Avoid session fixation for non-session authentication
+			// If the authenticated user is different from the session user, discard
+			// the old session entirely, without trusting any session values
+			if (!authedUser.equals(session.getUsername())) {
+				session.replaceSession();
 			}
 
-			// validate cookie during session (issue-361)
-			if (user != null && app().settings().getBoolean(Keys.web.allowCookieAuthentication, true)) {
-				String requestCookie = app().authentication().getCookie(request);
-				if (!StringUtils.isEmpty(requestCookie) && !StringUtils.isEmpty(user.cookie)) {
-					if (!requestCookie.equals(user.cookie)) {
-						// cookie was changed during our session
-						app().authentication().logout(request, response, user);
-						session.setUser(null);
-						session.invalidateNow();
-						return;
+			if (!session.isSessionInvalidated()) {
+				// Refresh usermodel to pick up any changes to permissions or roles (issue-186)
+				UserModel user = app().users().getUserModel(authedUser);
+
+				if (user == null || user.disabled) {
+					// user was deleted/disabled during session
+					app().authentication().logout(request, response, user);
+					session.setUser(null);
+					session.invalidateNow();
+					return;
+				}
+
+				// validate cookie during session (issue-361)
+				if (app().settings().getBoolean(Keys.web.allowCookieAuthentication, true)) {
+					String requestCookie = app().authentication().getCookie(request);
+					if (!StringUtils.isEmpty(requestCookie) && !StringUtils.isEmpty(user.cookie)) {
+						if (!requestCookie.equals(user.cookie)) {
+							// cookie was changed during our session
+							app().authentication().logout(request, response, user);
+							session.setUser(null);
+							session.invalidateNow();
+							return;
+						}
 					}
 				}
+				session.setUser(user);
+				return;
 			}
-			session.setUser(user);
-			return;
 		}
 
 		// try to authenticate by servlet request
@@ -91,9 +107,7 @@ public abstract class SessionPage extends WebPage {
 
 		// Login the user
 		if (user != null) {
-			// preserve the authentication type across session replacement
-			AuthenticationType authenticationType = (AuthenticationType) request.getSession()
-					.getAttribute(Constants.AUTHENTICATION_TYPE);
+			AuthenticationType authenticationType = (AuthenticationType) request.getAttribute(Constants.ATTRIB_AUTHTYPE);
 
 			// issue 62: fix session fixation vulnerability
 			// but only if authentication was done in the container.
@@ -101,11 +115,9 @@ public abstract class SessionPage extends WebPage {
 			// don't like
 			if (AuthenticationType.CONTAINER != authenticationType) {
 				session.replaceSession();
-			}			
+			}
 			session.setUser(user);
 
-			request.getSession().setAttribute(Constants.AUTHENTICATION_TYPE, authenticationType);
-
 			// Set Cookie
 			app().authentication().setCookie(request, response, user);