diff --git a/samples/users/src/main/webapp/index.jsp b/samples/users/src/main/webapp/index.jsp index d50b5fd..9fc5184 100644 --- a/samples/users/src/main/webapp/index.jsp +++ b/samples/users/src/main/webapp/index.jsp @@ -41,7 +41,10 @@
  • -
  • + + + +
  • diff --git a/spring-session/src/main/java/org/springframework/session/web/http/CookieHttpSessionStrategy.java b/spring-session/src/main/java/org/springframework/session/web/http/CookieHttpSessionStrategy.java index a7fb799..94aa954 100644 --- a/spring-session/src/main/java/org/springframework/session/web/http/CookieHttpSessionStrategy.java +++ b/spring-session/src/main/java/org/springframework/session/web/http/CookieHttpSessionStrategy.java @@ -15,9 +15,12 @@ */ package org.springframework.session.web.http; +import java.io.UnsupportedEncodingException; +import java.net.URLEncoder; import java.util.LinkedHashMap; import java.util.Map; import java.util.StringTokenizer; +import java.util.regex.Pattern; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; @@ -151,6 +154,8 @@ public final class CookieHttpSessionStrategy implements MultiHttpSessionStrategy static final String DEFAULT_SESSION_ALIAS_PARAM_NAME = "_s"; + private Pattern ALIAS_PATTERN = Pattern.compile("^[\\w-]{1,50}$"); + private String cookieName = "SESSION"; private String sessionParam = DEFAULT_SESSION_ALIAS_PARAM_NAME; @@ -169,6 +174,9 @@ public final class CookieHttpSessionStrategy implements MultiHttpSessionStrategy if(u == null) { return DEFAULT_ALIAS; } + if(!ALIAS_PATTERN.matcher(u).matches()) { + return DEFAULT_ALIAS; + } return u; } @@ -324,23 +332,32 @@ public final class CookieHttpSessionStrategy implements MultiHttpSessionStrategy } public String encodeURL(String url, String sessionAlias) { + String encodedSessionAlias = urlEncode(sessionAlias); int queryStart = url.indexOf("?"); - boolean isDefaultAlias = DEFAULT_ALIAS.equals(sessionAlias); + boolean isDefaultAlias = DEFAULT_ALIAS.equals(encodedSessionAlias); if(queryStart < 0) { - return isDefaultAlias ? url : url + "?" + sessionParam + "=" + sessionAlias; + return isDefaultAlias ? url : url + "?" + sessionParam + "=" + encodedSessionAlias; } String path = url.substring(0, queryStart); String query = url.substring(queryStart + 1, url.length()); - String replacement = isDefaultAlias ? "" : "$1"+sessionAlias; + String replacement = isDefaultAlias ? "" : "$1"+encodedSessionAlias; query = query.replaceFirst( "((^|&)" + sessionParam + "=)([^&]+)?", replacement); if(!isDefaultAlias && url.endsWith(query)) { // no existing alias if(!(query.endsWith("&") || query.length() == 0)) { query += "&"; } - query += sessionParam + "=" + sessionAlias; + query += sessionParam + "=" + encodedSessionAlias; } return path + "?" + query; } + + private String urlEncode(String value) { + try { + return URLEncoder.encode(value, "UTF-8"); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + } } \ No newline at end of file diff --git a/spring-session/src/test/java/org/springframework/session/web/http/CookieHttpSessionStrategyTests.java b/spring-session/src/test/java/org/springframework/session/web/http/CookieHttpSessionStrategyTests.java index 1418cf0..7c8f19f 100644 --- a/spring-session/src/test/java/org/springframework/session/web/http/CookieHttpSessionStrategyTests.java +++ b/spring-session/src/test/java/org/springframework/session/web/http/CookieHttpSessionStrategyTests.java @@ -210,6 +210,11 @@ public class CookieHttpSessionStrategyTests { assertThat(strategy.encodeURL("/url?a=b&_s=1", "0")).isEqualTo("/url?a=b"); } + @Test + public void encodeURLMaliciousAlias() { + assertThat(strategy.encodeURL("/url?a=b&_s=1", "\"> ")).isEqualTo("/url?a=b&_s=%22%3E+%3Cscript%3Ealert%28%27hi%27%29%3C%2Fscript%3E"); + } + // --- getCurrentSessionAlias @Test @@ -225,6 +230,60 @@ public class CookieHttpSessionStrategyTests { assertThat(strategy.getCurrentSessionAlias(request)).isEqualTo(CookieHttpSessionStrategy.DEFAULT_ALIAS); } + // protect against malicious users + @Test + public void getCurrentSessionAliasContainsQuote() { + request.setParameter(CookieHttpSessionStrategy.DEFAULT_SESSION_ALIAS_PARAM_NAME, "here\"this"); + + assertThat(strategy.getCurrentSessionAlias(request)).isEqualTo(CookieHttpSessionStrategy.DEFAULT_ALIAS); + } + + @Test + public void getCurrentSessionAliasContainsSingleQuote() { + request.setParameter(CookieHttpSessionStrategy.DEFAULT_SESSION_ALIAS_PARAM_NAME, "here'this"); + + assertThat(strategy.getCurrentSessionAlias(request)).isEqualTo(CookieHttpSessionStrategy.DEFAULT_ALIAS); + } + + @Test + public void getCurrentSessionAliasContainsSpace() { + request.setParameter(CookieHttpSessionStrategy.DEFAULT_SESSION_ALIAS_PARAM_NAME, "here this"); + + assertThat(strategy.getCurrentSessionAlias(request)).isEqualTo(CookieHttpSessionStrategy.DEFAULT_ALIAS); + } + + @Test + public void getCurrentSessionAliasContainsLt() { + request.setParameter(CookieHttpSessionStrategy.DEFAULT_SESSION_ALIAS_PARAM_NAME, "herethis"); + + assertThat(strategy.getCurrentSessionAlias(request)).isEqualTo(CookieHttpSessionStrategy.DEFAULT_ALIAS); + } + + @Test + public void getCurrentSessionAliasTooLong() { + request.setParameter(CookieHttpSessionStrategy.DEFAULT_SESSION_ALIAS_PARAM_NAME, "012345678901234567890123456789012345678901234567890"); + + assertThat(strategy.getCurrentSessionAlias(request)).isEqualTo(CookieHttpSessionStrategy.DEFAULT_ALIAS); + } + + // We want some sort of length restrictions, but want to ensure some sort of length Technically no hard limit, but chose 50 + @Test + public void getCurrentSessionAliasAllows50() { + request.setParameter(CookieHttpSessionStrategy.DEFAULT_SESSION_ALIAS_PARAM_NAME, "01234567890123456789012345678901234567890123456789"); + + assertThat(strategy.getCurrentSessionAlias(request)).isEqualTo("01234567890123456789012345678901234567890123456789"); + } + + + @Test public void getCurrentSession() { String expectedAlias = "1";