Skip to content

Commit

Permalink
On every form load, reset CSRF cookie to prevent re-use.
Browse files Browse the repository at this point in the history 
[#141240857] https://www.pivotaltracker.com/story/show/141240857

Signed-off-by: Jeremy Coffield <jcoffield@pivotal.io>
  • Loading branch information
@Priyata25
Bharath Sekar authored and Priyata25 committed Mar 16, 2017
1 parent f09db0d commit 4342544
Show file tree
Hide file tree
Showing 7 changed files with 104 additions and 88 deletions.
15 changes: 10 additions & 5 deletions .../main/java/org/cloudfoundry/identity/uaa/security/web/CookieBasedCsrfTokenRepository.java
View file
Expand Up @@ -16,6 +16,7 @@
package org.cloudfoundry.identity.uaa.security.web;

import org.springframework.security.oauth2.common.util.RandomValueStringGenerator;
import org.springframework.security.web.csrf.CsrfFilter;
import org.springframework.security.web.csrf.CsrfToken;
import org.springframework.security.web.csrf.CsrfTokenRepository;
import org.springframework.security.web.csrf.DefaultCsrfToken;
Expand Down Expand Up @@ -96,11 +97,15 @@ public void saveToken(CsrfToken token, HttpServletRequest request, HttpServletRe

@Override
public CsrfToken loadToken(HttpServletRequest request) {
Cookie[] cookies = request.getCookies();
if (cookies!=null) {
for (Cookie cookie : request.getCookies()) {
if (getParameterName().equals(cookie.getName())) {
return new DefaultCsrfToken(getHeaderName(), getParameterName(), cookie.getValue());
boolean requiresCsrfProtection = CsrfFilter.DEFAULT_CSRF_MATCHER.matches(request);

if(requiresCsrfProtection) {
Cookie[] cookies = request.getCookies();
if (cookies != null) {
for (Cookie cookie : request.getCookies()) {
if (getParameterName().equals(cookie.getName())) {
return new DefaultCsrfToken(getHeaderName(), getParameterName(), cookie.getValue());
}
}
}
}
Expand Down
31 changes: 0 additions & 31 deletions server/src/main/java/org/cloudfoundry/identity/uaa/security/web/CsrfResetFilter.java
View file

This file was deleted.

5 changes: 0 additions & 5 deletions server/src/main/resources/spring/login-ui.xml
View file
Expand Up @@ -277,10 +277,6 @@
<property name="logoutRequestMatcher" ref="uiLogoutRequestMatcher"/>
</bean>

<bean id="csrfResetFilter" class = "org.cloudfoundry.identity.uaa.security.web.CsrfResetFilter">
<constructor-arg ref="loginCookieCsrfRepository"/>
</bean>

<bean name="forcePasswordChangeController" class="org.cloudfoundry.identity.uaa.login.ForcePasswordChangeController">
<property name="resourcePropertySource" ref="messagePropertiesSource"/>
</bean>
Expand Down Expand Up @@ -318,7 +314,6 @@
<!--<logout logout-url="/logout.do" success-handler-ref="logoutHandler" invalidate-session="true"/>-->
<custom-filter ref="clientRedirectStateCache" before="FORM_LOGIN_FILTER"/>
<custom-filter ref="logoutFilter" after="LOGOUT_FILTER"/>
<custom-filter ref="csrfResetFilter" after="CSRF_FILTER"/>
<custom-filter ref="samlLogoutFilter" before="LOGOUT_FILTER"/>
<csrf disabled="false"
token-repository-ref="loginCookieCsrfRepository"
Expand Down
16 changes: 16 additions & 0 deletions .../src/test/java/org/cloudfoundry/identity/uaa/web/CookieBasedCsrfTokenRepositoryTests.java
View file
Expand Up @@ -17,16 +17,20 @@

import org.cloudfoundry.identity.uaa.security.web.CookieBasedCsrfTokenRepository;
import org.junit.Test;
import org.springframework.http.HttpMethod;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.security.oauth2.common.util.RandomValueStringGenerator;
import org.springframework.security.web.csrf.CsrfToken;
import org.springframework.security.web.csrf.DefaultCsrfToken;

import javax.servlet.http.Cookie;

import static org.hamcrest.Matchers.nullValue;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;

public class CookieBasedCsrfTokenRepositoryTests {
Expand Down Expand Up @@ -78,6 +82,18 @@ public void testSave_and_Load_Token() throws Exception {
assertEquals(token.getParameterName(), saved.getParameterName());
}

@Test
public void testLoad_Token_During_Get() {
MockHttpServletRequest request = new MockHttpServletRequest();
request.setMethod(HttpMethod.GET.name());
request.setCookies(new Cookie(CookieBasedCsrfTokenRepository.DEFAULT_CSRF_COOKIE_NAME, "should-be-removed"));

CookieBasedCsrfTokenRepository repo = new CookieBasedCsrfTokenRepository();

CsrfToken csrfToken = repo.loadToken(request);
assertThat(csrfToken, nullValue());
}

@Test
public void csrfCookie_alwaysHttpOnly() throws Exception {
Cookie cookie = getCookie(false);
Expand Down
29 changes: 16 additions & 13 deletions ...st/java/org/cloudfoundry/identity/uaa/integration/CheckTokenEndpointIntegrationTests.java
View file
Expand Up @@ -12,6 +12,8 @@
*******************************************************************************/
package org.cloudfoundry.identity.uaa.integration;

import org.apache.http.impl.client.BasicCookieStore;
import org.apache.http.impl.cookie.BasicClientCookie;
import org.cloudfoundry.identity.uaa.ServerRunning;
import org.cloudfoundry.identity.uaa.integration.util.IntegrationTestUtils;
import org.cloudfoundry.identity.uaa.test.TestAccountSetup;
Expand All @@ -36,6 +38,7 @@
import java.util.Arrays;
import java.util.Map;

import static org.cloudfoundry.identity.uaa.integration.util.IntegrationTestUtils.getHeaders;
import static org.cloudfoundry.identity.uaa.security.web.CookieBasedCsrfTokenRepository.DEFAULT_CSRF_COOKIE_NAME;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
Expand All @@ -59,32 +62,31 @@ public class CheckTokenEndpointIntegrationTests {
public void testDecodeToken() throws Exception {

// TODO Fix to use json API rather than HTML
HttpHeaders headers = new HttpHeaders();
// TODO: should be able to handle just TEXT_HTML
headers.setAccept(Arrays.asList(MediaType.TEXT_HTML, MediaType.ALL));

AuthorizationCodeResourceDetails resource = testAccounts.getDefaultAuthorizationCodeResource();
BasicCookieStore cookies = new BasicCookieStore();

URI uri = serverRunning.buildUri("/oauth/authorize").queryParam("response_type", "code")
.queryParam("state", "mystateid").queryParam("client_id", resource.getClientId())
.queryParam("redirect_uri", resource.getPreEstablishedRedirectUri()).build();
ResponseEntity<Void> result = serverRunning.getForResponse(uri.toString(), headers);
ResponseEntity<Void> result = serverRunning.getForResponse(uri.toString(), getHeaders(cookies));
assertEquals(HttpStatus.FOUND, result.getStatusCode());
String location = result.getHeaders().getLocation().toString();

if (result.getHeaders().containsKey("Set-Cookie")) {
for (String cookie : result.getHeaders().get("Set-Cookie")) {
assertNotNull("Expected cookie in " + result.getHeaders(), cookie);
headers.add("Cookie", cookie);
int nameLength = cookie.indexOf('=');
cookies.addCookie(new BasicClientCookie(cookie.substring(0, nameLength), cookie.substring(nameLength+1)));
}
}

ResponseEntity<String> response = serverRunning.getForString(location, headers);
ResponseEntity<String> response = serverRunning.getForString(location, getHeaders(cookies));

if (response.getHeaders().containsKey("Set-Cookie")) {
for (String cookie : response.getHeaders().get("Set-Cookie")) {
assertNotNull("Expected cookie in " + result.getHeaders(), cookie);
headers.add("Cookie", cookie);
int nameLength = cookie.indexOf('=');
cookies.addCookie(new BasicClientCookie(cookie.substring(0, nameLength), cookie.substring(nameLength+1)));
}
}
// should be directed to the login screen...
Expand All @@ -99,25 +101,25 @@ public void testDecodeToken() throws Exception {
formData.add(DEFAULT_CSRF_COOKIE_NAME, csrf);

// Should be redirected to the original URL, but now authenticated
result = serverRunning.postForResponse("/login.do", headers, formData);
result = serverRunning.postForResponse("/login.do", getHeaders(cookies), formData);
assertEquals(HttpStatus.FOUND, result.getStatusCode());

if (result.getHeaders().containsKey("Set-Cookie")) {
for (String cookie : result.getHeaders().get("Set-Cookie")) {
assertNotNull("Expected cookie in " + result.getHeaders(), cookie);
headers.add("Cookie", cookie);
int nameLength = cookie.indexOf('=');
cookies.addCookie(new BasicClientCookie(cookie.substring(0, nameLength), cookie.substring(nameLength+1)));
}
}

response = serverRunning.getForString(result.getHeaders().getLocation().toString(), headers);
response = serverRunning.getForString(result.getHeaders().getLocation().toString(), getHeaders(cookies));
if (response.getStatusCode() == HttpStatus.OK) {
// The grant access page should be returned
assertTrue(response.getBody().contains("<h1>Application Authorization</h1>"));

formData.clear();
formData.add(DEFAULT_CSRF_COOKIE_NAME, IntegrationTestUtils.extractCookieCsrf(response.getBody()));
formData.add(USER_OAUTH_APPROVAL, "true");
result = serverRunning.postForResponse("/oauth/authorize", headers, formData);
result = serverRunning.postForResponse("/oauth/authorize", getHeaders(cookies), formData);
assertEquals(HttpStatus.FOUND, result.getStatusCode());
location = result.getHeaders().getLocation().toString();
}
Expand All @@ -144,6 +146,7 @@ public void testDecodeToken() throws Exception {
@SuppressWarnings("unchecked")
OAuth2AccessToken accessToken = DefaultOAuth2AccessToken.valueOf(tokenResponse.getBody());

HttpHeaders headers = new HttpHeaders();
formData = new LinkedMultiValueMap<String, String>();
headers.set("Authorization",
testAccounts.getAuthorizationHeader(resource.getClientId(), resource.getClientSecret()));
Expand Down
38 changes: 21 additions & 17 deletions uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/AutologinIT.java
View file
Expand Up @@ -12,6 +12,8 @@
*******************************************************************************/
package org.cloudfoundry.identity.uaa.integration.feature;

import org.apache.http.impl.client.BasicCookieStore;
import org.apache.http.impl.cookie.BasicClientCookie;
import org.cloudfoundry.identity.uaa.integration.util.IntegrationTestUtils;
import org.cloudfoundry.identity.uaa.test.UaaTestAccounts;
import org.junit.After;
Expand Down Expand Up @@ -45,6 +47,7 @@
import java.util.List;
import java.util.Map;

import static org.cloudfoundry.identity.uaa.integration.util.IntegrationTestUtils.getHeaders;
import static org.cloudfoundry.identity.uaa.security.web.CookieBasedCsrfTokenRepository.DEFAULT_CSRF_COOKIE_NAME;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasItem;
Expand Down Expand Up @@ -225,43 +228,35 @@ public void testSimpleAutologinFlow() throws Exception {
//here we must reset our state. we do that by following the logout flow.
headers.clear();

headers.set(HttpHeaders.ACCEPT, MediaType.TEXT_HTML_VALUE);
BasicCookieStore cookieStore = new BasicCookieStore();
ResponseEntity<String> loginResponse = template.exchange(baseUrl + "/login",
HttpMethod.GET,
new HttpEntity<>(null, headers),
new HttpEntity<>(null, getHeaders(cookieStore)),
String.class);

if (loginResponse.getHeaders().containsKey("Set-Cookie")) {
for (String cookie : loginResponse.getHeaders().get("Set-Cookie")) {
headers.add("Cookie", cookie);
}
}
setCookiesFromResponse(cookieStore, loginResponse);
String csrf = IntegrationTestUtils.extractCookieCsrf(loginResponse.getBody());
requestBody.add(DEFAULT_CSRF_COOKIE_NAME, csrf);

headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED);
loginResponse = restOperations.exchange(baseUrl + "/login.do",
HttpMethod.POST,
new HttpEntity<>(requestBody, headers),
new HttpEntity<>(requestBody, getHeaders(cookieStore)),
String.class);
cookies = loginResponse.getHeaders().get("Set-Cookie");
assertThat(cookies, hasItem(startsWith("JSESSIONID")));
assertThat(cookies, hasItem(startsWith("X-Uaa-Csrf")));
assertThat(cookies, hasItem(startsWith("Saved-Account-")));
assertThat(cookies, hasItem(startsWith("Current-User")));
headers.clear();
for (String cookie : loginResponse.getHeaders().get("Set-Cookie")) {
if (!cookie.contains("1970")) { //deleted cookie
headers.add("Cookie", cookie);
}
}
cookieStore.clear();
setCookiesFromResponse(cookieStore, loginResponse);
headers.add(HttpHeaders.ACCEPT, MediaType.TEXT_HTML_VALUE);
ResponseEntity<String> profilePage =
restOperations.exchange(baseUrl + "/profile",
HttpMethod.GET,
new HttpEntity<>(null, headers), String.class);

new HttpEntity<>(null, getHeaders(cookieStore)), String.class);

setCookiesFromResponse(cookieStore, profilePage);
String revokeApprovalsUrl = UriComponentsBuilder.fromHttpUrl(baseUrl)
.path("/profile")
.build().toUriString();
Expand All @@ -271,11 +266,20 @@ public void testSimpleAutologinFlow() throws Exception {
requestBody.add(DEFAULT_CSRF_COOKIE_NAME, IntegrationTestUtils.extractCookieCsrf(profilePage.getBody()));
ResponseEntity<Void> revokeResponse = template.exchange(revokeApprovalsUrl,
HttpMethod.POST,
new HttpEntity<>(requestBody, headers),
new HttpEntity<>(requestBody, getHeaders(cookieStore)),
Void.class);
assertEquals(HttpStatus.FOUND, revokeResponse.getStatusCode());
}

private void setCookiesFromResponse(BasicCookieStore cookieStore, ResponseEntity<String> loginResponse) {
if (loginResponse.getHeaders().containsKey("Set-Cookie")) {
for (String cookie : loginResponse.getHeaders().get("Set-Cookie")) {
int nameLength = cookie.indexOf('=');
cookieStore.addCookie(new BasicClientCookie(cookie.substring(0, nameLength), cookie.substring(nameLength+1)));
}
}
}

@Test
public void testFormEncodedAutologinRequest() throws Exception {
HttpHeaders headers = getAppBasicAuthHttpHeaders();
Expand Down

0 comments on commit 4342544

Please sign in to comment.