Skip to content

Commit

Permalink
Merge pull request #908 from mrjoel/mrjoel-authrequestnotsession
Browse files Browse the repository at this point in the history 
prevent session fixation for external authentication
  • Loading branch information
@gitblit
gitblit committed Sep 18, 2015
2 parents 7a19abd + 62e0259 commit 0322351
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 43 deletions.
4 changes: 3 additions & 1 deletion src/main/java/com/gitblit/Constants.java
View file
Expand Up @@ -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();
Expand Down
35 changes: 25 additions & 10 deletions src/main/java/com/gitblit/manager/AuthenticationManager.java
View file
Expand Up @@ -194,6 +194,14 @@ public UserModel authenticate(HttpServletRequest httpRequest) {
*/
@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();
Expand All @@ -204,7 +212,7 @@ public UserModel authenticate(HttpServletRequest httpRequest, boolean requiresCe
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);
Expand Down Expand Up @@ -239,7 +247,7 @@ public UserModel authenticate(HttpServletRequest httpRequest, boolean requiresCe
}

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);
Expand All @@ -260,7 +268,7 @@ public UserModel authenticate(HttpServletRequest httpRequest, boolean requiresCe
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);
Expand All @@ -282,7 +290,7 @@ public UserModel authenticate(HttpServletRequest httpRequest, boolean requiresCe
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);
Expand All @@ -304,7 +312,7 @@ public UserModel authenticate(HttpServletRequest httpRequest, boolean requiresCe
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);
Expand Down Expand Up @@ -423,8 +431,9 @@ protected UserModel validateAuthentication(UserModel user, AuthenticationType ty
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);
}

/**
Expand Down Expand Up @@ -545,9 +554,15 @@ public void setCookie(HttpServletResponse response, UserModel user) {
@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;
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/com/gitblit/wicket/pages/RootPage.java
View file
Expand Up @@ -279,7 +279,7 @@ private void loginUser(UserModel user) {

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);
Expand Down Expand Up @@ -608,8 +608,8 @@ protected void onInitialize() {
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));
Expand Down
70 changes: 41 additions & 29 deletions src/main/java/com/gitblit/wicket/pages/SessionPage.java
View file
Expand Up @@ -56,56 +56,68 @@ private void login() {
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
UserModel user = app().authentication().authenticate(request);

// 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.
// It avoid double change of session, that some authentication method
// 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);

Expand Down

0 comments on commit 0322351

Please sign in to comment.