From: Joel Johnson Date: Fri, 26 Jun 2015 22:10:54 +0000 (-0600) Subject: prevent session fixation for external authentication X-Git-Tag: v1.7.0~1^2~18^2 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=refs%2Fpull%2F908%2Fhead;p=gitblit.git prevent session fixation for external authentication + use request instead of session to flag authentication status and user, for external authentication types --- 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);