From f7b794bf684a51539b240b23275bf887bbf9cae4 Mon Sep 17 00:00:00 2001 From: Nanne Baars Date: Sun, 3 Nov 2019 18:14:15 +0100 Subject: [PATCH] Race condition in counting number of attempts #567 (#697) Add version to Hibernate mapping so we get optimistic locking this solves number of parallel calls trying to update/guess and mess with the lesson counter --- .../owasp/webgoat/users/LessonTracker.java | 2 + .../resources/db/container/V2__version.sql | 1 + .../org/owasp/webgoat/IntegrationTest.java | 39 +++++++++-------- .../webgoat/ProgressRaceConditionTest.java | 43 +++++++++++++++++++ 4 files changed, 66 insertions(+), 19 deletions(-) create mode 100644 webgoat-container/src/main/resources/db/container/V2__version.sql create mode 100644 webgoat-integration-tests/src/test/java/org/owasp/webgoat/ProgressRaceConditionTest.java diff --git a/webgoat-container/src/main/java/org/owasp/webgoat/users/LessonTracker.java b/webgoat-container/src/main/java/org/owasp/webgoat/users/LessonTracker.java index e6beedf5c..31f9be00a 100644 --- a/webgoat-container/src/main/java/org/owasp/webgoat/users/LessonTracker.java +++ b/webgoat-container/src/main/java/org/owasp/webgoat/users/LessonTracker.java @@ -54,6 +54,8 @@ public class LessonTracker { private final Set allAssignments = new HashSet<>(); @Getter private int numberOfAttempts = 0; + @Version + private Integer version; private LessonTracker() { //JPA diff --git a/webgoat-container/src/main/resources/db/container/V2__version.sql b/webgoat-container/src/main/resources/db/container/V2__version.sql new file mode 100644 index 000000000..3d7a8908a --- /dev/null +++ b/webgoat-container/src/main/resources/db/container/V2__version.sql @@ -0,0 +1 @@ +ALTER TABLE CONTAINER.LESSON_TRACKER ADD VERSION INTEGER; diff --git a/webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java b/webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java index 8807b21ce..4d1c41bd5 100644 --- a/webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java +++ b/webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java @@ -37,9 +37,9 @@ public abstract class IntegrationTest { @BeforeClass public static void beforeAll() { -if (WG_SSL) { - WEBGOAT_URL = WEBGOAT_URL.replace("http:","https:"); - } + if (WG_SSL) { + WEBGOAT_URL = WEBGOAT_URL.replace("http:", "https:"); + } if (!started) { started = true; if (!isAlreadyRunning(WG_PORT)) { @@ -84,7 +84,7 @@ if (WG_SSL) { .relaxedHTTPSValidation() .formParam("username", webgoatUser) .formParam("password", "password") - .post(url("login")).then() + .post(url("login")).then() .cookie("JSESSIONID") .statusCode(302) .extract().header("Location"); @@ -236,7 +236,8 @@ if (WG_SSL) { .statusCode(200) .extract().path("lessonCompleted"), CoreMatchers.is(expectedResult)); } -public void checkAssignmentWithGet(String url, Map params, boolean expectedResult) { + + public void checkAssignmentWithGet(String url, Map params, boolean expectedResult) { Assert.assertThat( RestAssured.given() .when() @@ -251,28 +252,28 @@ public void checkAssignmentWithGet(String url, Map params, boolean ex public String getWebGoatServerPath() throws IOException { - //read path from server + //read path from server String result = RestAssured.given() - .when() - .relaxedHTTPSValidation() - .cookie("JSESSIONID", getWebGoatCookie()) - .get(url("/WebGoat/xxe/tmpdir")) - .then() - .extract().response().getBody().asString(); + .when() + .relaxedHTTPSValidation() + .cookie("JSESSIONID", getWebGoatCookie()) + .get(url("/WebGoat/xxe/tmpdir")) + .then() + .extract().response().getBody().asString(); result = result.replace("%20", " "); return result; } public String getWebWolfServerPath() throws IOException { - //read path from server + //read path from server String result = RestAssured.given() - .when() - .relaxedHTTPSValidation() - .cookie("WEBWOLFSESSION", getWebWolfCookie()) - .get(webWolfUrl("/tmpdir")) - .then() - .extract().response().getBody().asString(); + .when() + .relaxedHTTPSValidation() + .cookie("WEBWOLFSESSION", getWebWolfCookie()) + .get(webWolfUrl("/tmpdir")) + .then() + .extract().response().getBody().asString(); result = result.replace("%20", " "); return result; } diff --git a/webgoat-integration-tests/src/test/java/org/owasp/webgoat/ProgressRaceConditionTest.java b/webgoat-integration-tests/src/test/java/org/owasp/webgoat/ProgressRaceConditionTest.java new file mode 100644 index 000000000..467fef655 --- /dev/null +++ b/webgoat-integration-tests/src/test/java/org/owasp/webgoat/ProgressRaceConditionTest.java @@ -0,0 +1,43 @@ +package org.owasp.webgoat; + +import io.restassured.RestAssured; +import io.restassured.response.Response; +import org.assertj.core.api.Assertions; +import org.junit.Test; + +import java.util.List; +import java.util.Map; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.stream.Collectors; +import java.util.stream.IntStream; + +public class ProgressRaceConditionTest extends IntegrationTest { + + @Test + public void runTests() throws InterruptedException { + startLesson("Challenge1"); + + Callable call = () -> + RestAssured.given() + .when() + .relaxedHTTPSValidation() + .cookie("JSESSIONID", getWebGoatCookie()) + .formParams(Map.of("flag", "test")) + .post(url("/challenge/flag/")); + ExecutorService executorService = Executors.newFixedThreadPool(20); + List> flagCalls = IntStream.range(0, 20).mapToObj(i -> call).collect(Collectors.toList()); + var responses = executorService.invokeAll(flagCalls); + + //A certain amount of parallel calls should fail as optimistic locking in DB is applied + Assertions.assertThat(responses.stream().filter(r -> { + try { + return r.get().getStatusCode() == 500; + } catch (InterruptedException | ExecutionException e) { + throw new IllegalStateException(e); + } + }).count()).isGreaterThan(10); + } +}