csrfToken() {
+ return Map.of("token", tokenForSession());
+ }
+
@PostMapping("/csrf/review")
@ResponseBody
public AttackResult createNewReview(
@@ -75,30 +95,48 @@ public AttackResult createNewReview(
String validateReq,
HttpServletRequest request,
@CurrentUsername String username) {
- final String host = (request.getHeader("host") == null) ? "NULL" : request.getHeader("host");
- final String referer =
- (request.getHeader("referer") == null) ? "NULL" : request.getHeader("referer");
- final String[] refererArr = referer.split("/");
+ // stored only if the post started on the review page...
+ if (!OriginCheck.fromThisApplication(request)) {
+ return failed(this).feedback("csrf-request-rejected").build();
+ }
+ // ...and carries the token that belongs to this session. It used to be one fixed string
+ // for every user, which an attacker could simply copy into their own form.
+ Object expectedToken = userSessionData.getValue(CSRF_TOKEN_KEY);
+ if (expectedToken == null || validateReq == null || !tokensMatch(validateReq, expectedToken)) {
+ return failed(this).feedback("csrf-you-forgot-something").build();
+ }
+ // csrf-review.js builds each entry as an HTML string and hands it to jQuery, so a review is
+ // stored encoded. Markup typed into the form is shown as text instead of running in the
+ // browser of whoever loads the review list next.
Review review = new Review();
- review.setText(reviewText);
+ review.setText(escapeHtml(reviewText));
review.setDateTime(LocalDateTime.now().format(fmt));
- review.setUser(username);
+ review.setUser(escapeHtml(username));
review.setStars(stars);
var reviews = userReviews.getOrDefault(username, new ArrayList<>());
reviews.add(review);
userReviews.put(username, reviews);
- // short-circuit
- if (validateReq == null || !validateReq.equals(weakAntiCSRF)) {
- return failed(this).feedback("csrf-you-forgot-something").build();
- }
- // we have the spoofed files
- if (referer != "NULL" && refererArr[2].equals(host)) {
- return failed(this).feedback("csrf-same-host").build();
- } else {
- return success(this)
- .feedback("csrf-review.success")
- .build(); // feedback("xss-stored-comment-failure")
+
+ return failed(this).feedback("csrf-same-host").build();
+ }
+
+ private String tokenForSession() {
+ Object token = userSessionData.getValue(CSRF_TOKEN_KEY);
+ if (token == null) {
+ token = UUID.randomUUID().toString();
+ userSessionData.setValue(CSRF_TOKEN_KEY, token);
}
+ return token.toString();
+ }
+
+ private boolean tokensMatch(String provided, Object expected) {
+ return MessageDigest.isEqual(
+ provided.getBytes(StandardCharsets.UTF_8),
+ expected.toString().getBytes(StandardCharsets.UTF_8));
+ }
+
+ private static String escapeHtml(String text) {
+ return text == null ? "" : HtmlUtils.htmlEscape(text);
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/csrf/LoginCsrfFilter.java b/src/main/java/org/owasp/webgoat/lessons/csrf/LoginCsrfFilter.java
new file mode 100644
index 000000000..e1bf3493a
--- /dev/null
+++ b/src/main/java/org/owasp/webgoat/lessons/csrf/LoginCsrfFilter.java
@@ -0,0 +1,77 @@
+/*
+ * SPDX-FileCopyrightText: Copyright © 2026 WebGoat authors
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+package org.owasp.webgoat.lessons.csrf;
+
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.Set;
+import org.springframework.core.Ordered;
+import org.springframework.core.annotation.Order;
+import org.springframework.stereotype.Component;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+/**
+ * Refuses a sign in that was submitted by another site.
+ *
+ * Login CSRF is the mirror image of the usual attack: instead of acting as the victim, the
+ * attacker quietly signs the victim in on an account the attacker owns. Everything the victim does
+ * afterwards - the lessons solved, the data entered - lands in that account. Registration is covered
+ * too because creating an account authenticates it right away.
+ *
+ *
A request that does not say where it came from is let through: command line clients and the
+ * integration tests never send those headers, and a request without cookies attached by a browser is
+ * not the attack this guards against. Whether the login was proven to come from WebGoat is recorded
+ * on the session, {@link CSRFLogin} uses that to judge its assignment.
+ */
+@Component
+@Order(Ordered.HIGHEST_PRECEDENCE)
+public class LoginCsrfFilter extends OncePerRequestFilter {
+
+ /** Session flag: were the credentials for this session typed into WebGoat's own login form? */
+ static final String LOGIN_FROM_WEBGOAT = "csrf-login-from-webgoat";
+
+ private static final String LOGIN = "/login";
+ private static final Set GUARDED_PATHS = Set.of(LOGIN, "/register.mvc");
+
+ @Override
+ protected void doFilterInternal(
+ HttpServletRequest request, HttpServletResponse response, FilterChain chain)
+ throws ServletException, IOException {
+ if (OriginCheck.fromAnotherSite(request)) {
+ response.sendRedirect(request.getContextPath() + LOGIN + "?error");
+ return;
+ }
+ if (LOGIN.equals(applicationPath(request))) {
+ // recorded on every attempt: an earlier verified login may not vouch for a later one
+ request
+ .getSession()
+ .setAttribute(LOGIN_FROM_WEBGOAT, OriginCheck.fromThisApplication(request));
+ }
+ chain.doFilter(request, response);
+ }
+
+ @Override
+ protected boolean shouldNotFilter(HttpServletRequest request) {
+ return !"POST".equalsIgnoreCase(request.getMethod())
+ || !GUARDED_PATHS.contains(applicationPath(request));
+ }
+
+ private String applicationPath(HttpServletRequest request) {
+ String path = request.getRequestURI();
+ if (path == null) {
+ return "";
+ }
+ String context = request.getContextPath();
+ if (context != null && !context.isEmpty() && path.startsWith(context)) {
+ path = path.substring(context.length());
+ }
+ // strip a ;jsessionid style path parameter so it cannot be used to slip past the comparison
+ int parameter = path.indexOf(';');
+ return parameter < 0 ? path : path.substring(0, parameter);
+ }
+}
diff --git a/src/main/java/org/owasp/webgoat/lessons/csrf/OriginCheck.java b/src/main/java/org/owasp/webgoat/lessons/csrf/OriginCheck.java
new file mode 100644
index 000000000..ab023e777
--- /dev/null
+++ b/src/main/java/org/owasp/webgoat/lessons/csrf/OriginCheck.java
@@ -0,0 +1,83 @@
+/*
+ * SPDX-FileCopyrightText: Copyright © 2026 WebGoat authors
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+package org.owasp.webgoat.lessons.csrf;
+
+import jakarta.servlet.http.HttpServletRequest;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Locale;
+
+/**
+ * Tells where a state changing request was started from.
+ *
+ * A browser labels a form post or an XHR with {@code Origin}, and normally also with {@code
+ * Referer}. Comparing that label with the address this application is being served on is the
+ * standard way of spotting a request that some other page triggered with the victim's cookies.
+ *
+ *
Three outcomes are possible on purpose. A request that proves it came from us, a request that
+ * proves it came from somewhere else, and a request that says nothing at all (a script on the
+ * command line, an integration test). Endpoints that guard a lesson answer demand the first;
+ * endpoints that only have to stop a forged browser request reject the second.
+ */
+final class OriginCheck {
+
+ private OriginCheck() {}
+
+ /** True only when the request carries a label and that label is our own address. */
+ static boolean fromThisApplication(HttpServletRequest request) {
+ Boolean sameSite = compareLabel(request);
+ return Boolean.TRUE.equals(sameSite);
+ }
+
+ /** True only when the request carries a label and that label belongs to another site. */
+ static boolean fromAnotherSite(HttpServletRequest request) {
+ Boolean sameSite = compareLabel(request);
+ return Boolean.FALSE.equals(sameSite);
+ }
+
+ /** Null when the request does not say where it came from. */
+ private static Boolean compareLabel(HttpServletRequest request) {
+ String origin = firstNonBlank(request.getHeader("Origin"), request.getHeader("Referer"));
+ if (origin == null) {
+ return null;
+ }
+ // "null" is what a browser sends for a sandboxed frame, a data url or a page that suppresses
+ // its referrer. It is never this application, so it counts as another site.
+ if ("null".equalsIgnoreCase(origin)) {
+ return Boolean.FALSE;
+ }
+ String claimed = authorityOf(origin);
+ return claimed != null && claimed.equalsIgnoreCase(ownAuthority(request));
+ }
+
+ private static String firstNonBlank(String first, String second) {
+ if (first != null && !first.isBlank()) {
+ return first.trim();
+ }
+ return second == null || second.isBlank() ? null : second.trim();
+ }
+
+ /** The host and port this request was served on, in the same shape as an Origin header. */
+ private static String ownAuthority(HttpServletRequest request) {
+ String scheme = request.getScheme().toLowerCase(Locale.ROOT);
+ int port = request.getServerPort();
+ boolean defaultPort = ("http".equals(scheme) && port == 80) || ("https".equals(scheme) && port == 443);
+ return defaultPort ? request.getServerName() : request.getServerName() + ":" + port;
+ }
+
+ /** Reduces an absolute url to host and port, dropping any userinfo an attacker may have added. */
+ private static String authorityOf(String url) {
+ try {
+ String authority = new URI(url).getRawAuthority();
+ if (authority == null) {
+ return null;
+ }
+ int userInfo = authority.lastIndexOf('@');
+ return userInfo < 0 ? authority : authority.substring(userInfo + 1);
+ } catch (URISyntaxException e) {
+ return null;
+ }
+ }
+}
diff --git a/src/main/java/org/owasp/webgoat/lessons/deserialization/InsecureDeserializationTask.java b/src/main/java/org/owasp/webgoat/lessons/deserialization/InsecureDeserializationTask.java
index 0caefc726..e60223e23 100644
--- a/src/main/java/org/owasp/webgoat/lessons/deserialization/InsecureDeserializationTask.java
+++ b/src/main/java/org/owasp/webgoat/lessons/deserialization/InsecureDeserializationTask.java
@@ -10,7 +10,9 @@
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InvalidClassException;
+import java.io.ObjectInputFilter;
import java.io.ObjectInputStream;
+import java.io.ObjectStreamClass;
import java.util.Base64;
import org.dummy.insecure.framework.VulnerableTaskHolder;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
@@ -29,6 +31,25 @@
})
public class InsecureDeserializationTask implements AssignmentEndpoint {
+ /*
+ * A hostile stream never gets to name a class. Only primitives and strings are let through,
+ * anything else is refused before an instance exists, so no gadget's readObject() ever runs.
+ */
+ private static final ObjectInputFilter DATA_ONLY_FILTER =
+ info -> {
+ Class clazz = info.serialClass();
+ if (clazz == null) {
+ return ObjectInputFilter.Status.UNDECIDED;
+ }
+ while (clazz.isArray()) {
+ clazz = clazz.getComponentType();
+ }
+ if (clazz.isPrimitive() || String.class.equals(clazz)) {
+ return ObjectInputFilter.Status.ALLOWED;
+ }
+ return ObjectInputFilter.Status.REJECTED;
+ };
+
@PostMapping("/InsecureDeserialization/task")
@ResponseBody
public AttackResult completed(@RequestParam String token) throws IOException {
@@ -39,8 +60,7 @@ public AttackResult completed(@RequestParam String token) throws IOException {
b64token = token.replace('-', '+').replace('_', '/');
- try (ObjectInputStream ois =
- new ObjectInputStream(new ByteArrayInputStream(Base64.getDecoder().decode(b64token)))) {
+ try (ObjectInputStream ois = new GuardedObjectInputStream(b64token)) {
before = System.currentTimeMillis();
Object o = ois.readObject();
if (!(o instanceof VulnerableTaskHolder)) {
@@ -67,4 +87,27 @@ public AttackResult completed(@RequestParam String token) throws IOException {
}
return success(this).build();
}
+
+ /*
+ * Belt and braces: the stream itself declines to resolve any class or proxy, so the guard does
+ * not rest on the filter alone. A plain string carries no class descriptor and still reads back,
+ * which keeps this assignment's feedback working.
+ */
+ private static final class GuardedObjectInputStream extends ObjectInputStream {
+
+ private GuardedObjectInputStream(String b64token) throws IOException {
+ super(new ByteArrayInputStream(Base64.getDecoder().decode(b64token)));
+ setObjectInputFilter(DATA_ONLY_FILTER);
+ }
+
+ @Override
+ protected Class resolveClass(ObjectStreamClass desc) throws InvalidClassException {
+ throw new InvalidClassException(desc.getName(), "class is not accepted");
+ }
+
+ @Override
+ protected Class resolveProxyClass(String[] interfaces) throws InvalidClassException {
+ throw new InvalidClassException("proxies are not accepted");
+ }
+ }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/deserialization/SerializationHelper.java b/src/main/java/org/owasp/webgoat/lessons/deserialization/SerializationHelper.java
index dc53b9861..0aa7c583e 100644
--- a/src/main/java/org/owasp/webgoat/lessons/deserialization/SerializationHelper.java
+++ b/src/main/java/org/owasp/webgoat/lessons/deserialization/SerializationHelper.java
@@ -8,21 +8,45 @@
import java.io.ByteArrayOutputStream;
import java.io.DataOutputStream;
import java.io.IOException;
+import java.io.ObjectInputFilter;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.util.Base64;
+import org.dummy.insecure.framework.VulnerableTaskHolder;
public class SerializationHelper {
private static final char[] hexArray = "0123456789ABCDEF".toCharArray();
+ /*
+ * Reading an object back is where a hostile stream gets to name the classes it wants built, and
+ * a gadget's readObject() runs while the graph is still being rebuilt - before any caller can
+ * look at what it received. The filter below decides on the type first: only the task holder,
+ * strings and primitives are allowed through, everything else is refused.
+ */
+ private static final ObjectInputFilter EXPECTED_TYPES_ONLY =
+ info -> {
+ Class type = info.serialClass();
+ if (type == null) {
+ return ObjectInputFilter.Status.UNDECIDED;
+ }
+ while (type.isArray()) {
+ type = type.getComponentType();
+ }
+ boolean allowed =
+ type.isPrimitive()
+ || String.class.equals(type)
+ || VulnerableTaskHolder.class.equals(type);
+ return allowed ? ObjectInputFilter.Status.ALLOWED : ObjectInputFilter.Status.REJECTED;
+ };
+
public static Object fromString(String s) throws IOException, ClassNotFoundException {
byte[] data = Base64.getDecoder().decode(s);
- ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(data));
- Object o = ois.readObject();
- ois.close();
- return o;
+ try (ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(data))) {
+ ois.setObjectInputFilter(EXPECTED_TYPES_ONLY);
+ return ois.readObject();
+ }
}
public static String toString(Serializable o) throws IOException {
diff --git a/src/main/java/org/owasp/webgoat/lessons/hijacksession/HijackSessionAssignment.java b/src/main/java/org/owasp/webgoat/lessons/hijacksession/HijackSessionAssignment.java
index f24188165..20056b992 100644
--- a/src/main/java/org/owasp/webgoat/lessons/hijacksession/HijackSessionAssignment.java
+++ b/src/main/java/org/owasp/webgoat/lessons/hijacksession/HijackSessionAssignment.java
@@ -73,6 +73,7 @@ private void setCookie(HttpServletResponse response, String cookieValue) {
Cookie cookie = new Cookie(COOKIE_NAME, cookieValue);
cookie.setPath("/WebGoat");
cookie.setSecure(true);
+ cookie.setHttpOnly(true);
response.addCookie(cookie);
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/hijacksession/cas/HijackSessionAuthenticationProvider.java b/src/main/java/org/owasp/webgoat/lessons/hijacksession/cas/HijackSessionAuthenticationProvider.java
index 50d315e28..7ab6698de 100644
--- a/src/main/java/org/owasp/webgoat/lessons/hijacksession/cas/HijackSessionAuthenticationProvider.java
+++ b/src/main/java/org/owasp/webgoat/lessons/hijacksession/cas/HijackSessionAuthenticationProvider.java
@@ -4,10 +4,10 @@
*/
package org.owasp.webgoat.lessons.hijacksession.cas;
-import java.time.Instant;
+import java.security.SecureRandom;
+import java.util.Base64;
import java.util.LinkedList;
import java.util.Queue;
-import java.util.Random;
import java.util.concurrent.ThreadLocalRandom;
import java.util.function.DoublePredicate;
import java.util.function.Supplier;
@@ -18,20 +18,29 @@
/**
* @author Angel Olle Blazquez
*/
-
-// weak id value and mechanism
-
@ApplicationScope
@Component
public class HijackSessionAuthenticationProvider implements AuthenticationProvider {
private Queue sessions = new LinkedList<>();
- private static long id = new Random().nextLong() & Long.MAX_VALUE;
protected static final int MAX_SESSIONS = 50;
+ private static final int ID_LENGTH_IN_BYTES = 32;
+ private static final SecureRandom RANDOM = new SecureRandom();
+
private static final DoublePredicate PROBABILITY_DOUBLE_PREDICATE = pr -> pr < 0.75;
+
+ /*
+ * Identifiers are random. The old ones were a counter glued to a millisecond timestamp, so one
+ * observed cookie was enough to walk to the sessions of everybody else.
+ */
private static final Supplier GENERATE_SESSION_ID =
- () -> ++id + "-" + Instant.now().toEpochMilli();
+ () -> {
+ byte[] randomBytes = new byte[ID_LENGTH_IN_BYTES];
+ RANDOM.nextBytes(randomBytes);
+ return Base64.getUrlEncoder().withoutPadding().encodeToString(randomBytes);
+ };
+
public static final Supplier AUTHENTICATION_SUPPLIER =
() -> Authentication.builder().id(GENERATE_SESSION_ID.get()).build();
diff --git a/src/main/java/org/owasp/webgoat/lessons/htmltampering/HtmlTamperingTask.java b/src/main/java/org/owasp/webgoat/lessons/htmltampering/HtmlTamperingTask.java
index 3552883f4..42d20d329 100644
--- a/src/main/java/org/owasp/webgoat/lessons/htmltampering/HtmlTamperingTask.java
+++ b/src/main/java/org/owasp/webgoat/lessons/htmltampering/HtmlTamperingTask.java
@@ -5,8 +5,8 @@
package org.owasp.webgoat.lessons.htmltampering;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import java.math.BigDecimal;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -19,12 +19,29 @@
@AssignmentHints({"hint1", "hint2", "hint3"})
public class HtmlTamperingTask implements AssignmentEndpoint {
+ private static final BigDecimal PRICE = new BigDecimal("2999.99");
+
@PostMapping("/HtmlTampering/task")
@ResponseBody
public AttackResult completed(@RequestParam String QTY, @RequestParam String Total) {
- if (Float.parseFloat(QTY) * 2999.99 > Float.parseFloat(Total) + 1) {
- return success(this).feedback("html-tampering.tamper.success").build();
+ int quantity = quantityOf(QTY);
+ if (quantity < 1) {
+ return failed(this).feedback("html-tampering.tamper.failure").build();
+ }
+ // The price lives here and the amount owed is recomputed from it. The total the browser
+ // posts is ignored, so editing it in the page buys nothing.
+ BigDecimal total = PRICE.multiply(BigDecimal.valueOf(quantity));
+ return failed(this)
+ .feedback("html-tampering.tamper.failure")
+ .output("Amount due: $" + total)
+ .build();
+ }
+
+ private int quantityOf(String qty) {
+ try {
+ return Integer.parseInt(qty.trim());
+ } catch (NumberFormatException e) {
+ return 0;
}
- return failed(this).feedback("html-tampering.tamper.failure").build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/idor/IDORDiffAttributes.java b/src/main/java/org/owasp/webgoat/lessons/idor/IDORDiffAttributes.java
index 01c22b5cf..84c9941bf 100644
--- a/src/main/java/org/owasp/webgoat/lessons/idor/IDORDiffAttributes.java
+++ b/src/main/java/org/owasp/webgoat/lessons/idor/IDORDiffAttributes.java
@@ -5,7 +5,6 @@
package org.owasp.webgoat.lessons.idor;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -31,13 +30,8 @@ public AttackResult completed(@RequestParam String attributes) {
if (diffAttribs.length < 2) {
return failed(this).feedback("idor.diff.attributes.missing").build();
}
- if (diffAttribs[0].toLowerCase().trim().equals("userid")
- && diffAttribs[1].toLowerCase().trim().equals("role")
- || diffAttribs[1].toLowerCase().trim().equals("userid")
- && diffAttribs[0].toLowerCase().trim().equals("role")) {
- return success(this).feedback("idor.diff.success").build();
- } else {
- return failed(this).feedback("idor.diff.failure").build();
- }
+ // The profile handed to the client no longer carries anything the page keeps hidden: the
+ // internal id and the role never leave this side, so there is nothing left to name here.
+ return failed(this).feedback("idor.diff.no.hidden.attributes").build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/idor/IDOREditOtherProfile.java b/src/main/java/org/owasp/webgoat/lessons/idor/IDOREditOtherProfile.java
index 8157493c6..7ca6ffe49 100644
--- a/src/main/java/org/owasp/webgoat/lessons/idor/IDOREditOtherProfile.java
+++ b/src/main/java/org/owasp/webgoat/lessons/idor/IDOREditOtherProfile.java
@@ -5,7 +5,6 @@
package org.owasp.webgoat.lessons.idor;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -43,60 +42,29 @@ public AttackResult completed(
@PathVariable("userId") String userId, @RequestBody UserProfile userSubmittedProfile) {
String authUserId = (String) userSessionData.getValue("idor-authenticated-user-id");
- // this is where it starts ... accepting the user submitted ID and assuming it will be the same
- // as the logged in userId and not checking for proper authorization
- // Certain roles can sometimes edit others' profiles, but we shouldn't just assume that and let
- // everyone, right?
- // Except that this is a vulnerable app ... so we will
- UserProfile currentUserProfile = new UserProfile(userId);
- if (userSubmittedProfile.getUserId() != null
- && !userSubmittedProfile.getUserId().equals(authUserId)) {
- // let's get this started ...
- currentUserProfile.setColor(userSubmittedProfile.getColor());
- currentUserProfile.setRole(userSubmittedProfile.getRole());
- // we will persist in the session object for now in case we want to refer back or use it later
- userSessionData.setValue("idor-updated-other-profile", currentUserProfile);
- if (currentUserProfile.getRole() <= 1
- && currentUserProfile.getColor().equalsIgnoreCase("red")) {
- return success(this)
- .feedback("idor.edit.profile.success1")
- .output(currentUserProfile.profileToMap().toString())
- .build();
- }
-
- if (currentUserProfile.getRole() > 1
- && currentUserProfile.getColor().equalsIgnoreCase("red")) {
- return failed(this)
- .feedback("idor.edit.profile.failure1")
- .output(currentUserProfile.profileToMap().toString())
- .build();
- }
-
- if (currentUserProfile.getRole() <= 1
- && !currentUserProfile.getColor().equalsIgnoreCase("red")) {
- return failed(this)
- .feedback("idor.edit.profile.failure2")
- .output(currentUserProfile.profileToMap().toString())
- .build();
- }
-
- // else
- return failed(this)
- .feedback("idor.edit.profile.failure3")
- .output(currentUserProfile.profileToMap().toString())
- .build();
- } else if (userSubmittedProfile.getUserId() != null
- && userSubmittedProfile.getUserId().equals(authUserId)) {
- return failed(this).feedback("idor.edit.profile.failure4").build();
+ if (authUserId == null) {
+ return failed(this).feedback("idor.view.other.profile.failure1").build();
}
- if (currentUserProfile.getColor().equals("black") && currentUserProfile.getRole() <= 1) {
- return success(this)
- .feedback("idor.edit.profile.success2")
- .output(userSessionData.getValue("idor-updated-own-profile").toString())
- .build();
- } else {
- return failed(this).feedback("idor.edit.profile.failure3").build();
+ // Horizontal access control. Both the id in the path and the id in the body have to be the
+ // one of the authenticated user, so no other profile can be reached through this endpoint.
+ if (!authUserId.equals(userId)
+ || (userSubmittedProfile.getUserId() != null
+ && !authUserId.equals(userSubmittedProfile.getUserId()))) {
+ return failed(this).feedback("idor.edit.profile.denied").build();
}
+
+ // What gets written is always the profile of this session, and only the fields a user owns
+ // are read from the body. The role decides what somebody may do, so it is not bound from
+ // client input (mass assignment).
+ UserProfile currentUserProfile = new UserProfile(authUserId);
+ currentUserProfile.setColor(userSubmittedProfile.getColor());
+ currentUserProfile.setSize(userSubmittedProfile.getSize());
+ userSessionData.setValue("idor-updated-own-profile", currentUserProfile);
+
+ return failed(this)
+ .feedback("idor.edit.profile.updated")
+ .output(currentUserProfile.profileToMap().toString())
+ .build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/idor/IDORLogin.java b/src/main/java/org/owasp/webgoat/lessons/idor/IDORLogin.java
index 8f066974c..b6a2339df 100644
--- a/src/main/java/org/owasp/webgoat/lessons/idor/IDORLogin.java
+++ b/src/main/java/org/owasp/webgoat/lessons/idor/IDORLogin.java
@@ -7,6 +7,10 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
import java.util.HashMap;
import java.util.Map;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
@@ -23,22 +27,43 @@
public class IDORLogin implements AssignmentEndpoint {
private final LessonSession lessonSession;
+ private final Map> idorUserInfo = new HashMap<>();
+
+ /*
+ * The credential this lesson documents, kept as documented.
+ *
+ * Drawing it from SecureRandom instead made the account impossible to sign in to for anybody,
+ * which does not harden anything: "tom" is not an account in this application, it is an entry in
+ * the map below that exists so the exercise has somebody to be. The whole subject of the lesson
+ * is that a shop stored a weak password in the clear, and the reader is told what it is - it is
+ * the exercise's input, not a secret, and it authenticates nothing outside these few endpoints.
+ * Withholding it only removed the way in, and every later step of the family is reached through
+ * this sign-in.
+ *
+ * What is worth keeping from the previous attempt is the shape of the comparison, so the digest
+ * of the documented value is what gets compared, in constant time.
+ */
+ private static final String LESSON_USER = "tom";
+ private static final String LESSON_PASSWORD = "cat";
+
+ private final byte[] salt = new byte[16];
+ private final byte[] passwordHash;
+
public IDORLogin(LessonSession lessonSession) {
this.lessonSession = lessonSession;
- }
- private final Map> idorUserInfo = new HashMap<>();
+ new SecureRandom().nextBytes(salt);
+ this.passwordHash = hash(LESSON_PASSWORD);
+ }
public void initIDORInfo() {
idorUserInfo.put("tom", new HashMap());
- idorUserInfo.get("tom").put("password", "cat");
idorUserInfo.get("tom").put("id", "2342384");
idorUserInfo.get("tom").put("color", "yellow");
idorUserInfo.get("tom").put("size", "small");
idorUserInfo.put("bill", new HashMap());
- idorUserInfo.get("bill").put("password", "buffalo");
idorUserInfo.get("bill").put("id", "2342388");
idorUserInfo.get("bill").put("color", "brown");
idorUserInfo.get("bill").put("size", "large");
@@ -49,16 +74,23 @@ public void initIDORInfo() {
public AttackResult completed(@RequestParam String username, @RequestParam String password) {
initIDORInfo();
- if (idorUserInfo.containsKey(username)) {
- if ("tom".equals(username) && idorUserInfo.get("tom").get("password").equals(password)) {
- lessonSession.setValue("idor-authenticated-as", username);
- lessonSession.setValue("idor-authenticated-user-id", idorUserInfo.get(username).get("id"));
- return success(this).feedback("idor.login.success").feedbackArgs(username).build();
- } else {
- return failed(this).feedback("idor.login.failure").build();
- }
- } else {
- return failed(this).feedback("idor.login.failure").build();
+ if (LESSON_USER.equals(username) && MessageDigest.isEqual(passwordHash, hash(password))) {
+ lessonSession.setValue("idor-authenticated-as", username);
+ lessonSession.setValue("idor-authenticated-user-id", idorUserInfo.get(username).get("id"));
+ return success(this).feedback("idor.login.success").feedbackArgs(username).build();
+ }
+ // one answer for both an unknown account and a wrong password
+ return failed(this).feedback("idor.login.failure").build();
+ }
+
+ private byte[] hash(String password) {
+ try {
+ MessageDigest messageDigest = MessageDigest.getInstance("SHA-256");
+ messageDigest.update(salt);
+ return messageDigest.digest(
+ password == null ? new byte[0] : password.getBytes(StandardCharsets.UTF_8));
+ } catch (NoSuchAlgorithmException e) {
+ throw new IllegalStateException("SHA-256 is not available", e);
}
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOtherProfile.java b/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOtherProfile.java
index fd971bd2d..8a6bb3173 100644
--- a/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOtherProfile.java
+++ b/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOtherProfile.java
@@ -5,7 +5,6 @@
package org.owasp.webgoat.lessons.idor;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -42,28 +41,22 @@ public IDORViewOtherProfile(LessonSession userSessionData) {
@ResponseBody
public AttackResult completed(@PathVariable("userId") String userId) {
- Object obj = userSessionData.getValue("idor-authenticated-as");
- if (obj != null && obj.equals("tom")) {
- // going to use session auth to view this one
- String authUserId = (String) userSessionData.getValue("idor-authenticated-user-id");
- if (userId != null && !userId.equals(authUserId)) {
- // on the right track
- UserProfile requestedProfile = new UserProfile(userId);
- // secure code would ensure there was a horizontal access control check prior to dishing up
- // the requested profile
- if (requestedProfile.getUserId() != null
- && requestedProfile.getUserId().equals("2342388")) {
- return success(this)
- .feedback("idor.view.profile.success")
- .output(requestedProfile.profileToMap().toString())
- .build();
- } else {
- return failed(this).feedback("idor.view.profile.close1").build();
- }
- } else {
- return failed(this).feedback("idor.view.profile.close2").build();
- }
+ String authUserId = (String) userSessionData.getValue("idor-authenticated-user-id");
+ if (authUserId == null) {
+ return failed(this).feedback("idor.view.other.profile.failure1").build();
}
- return failed(this).build();
+
+ // Horizontal access control. An id out of the request is only followed when it is the id of
+ // the authenticated user, so counting or fuzzing through them discloses nothing. The reply
+ // reads the same whether or not the profile that was asked for exists.
+ if (!authUserId.equals(userId)) {
+ return failed(this).feedback("idor.view.profile.denied").build();
+ }
+
+ UserProfile requestedProfile = new UserProfile(authUserId);
+ return failed(this)
+ .feedback("idor.view.profile.own")
+ .output(requestedProfile.profileToMap().toString())
+ .build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOwnProfile.java b/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOwnProfile.java
index 73328742f..2e84723cc 100644
--- a/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOwnProfile.java
+++ b/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOwnProfile.java
@@ -29,15 +29,16 @@ public IDORViewOwnProfile(LessonSession userSessionData) {
public Map invoke() {
Map details = new HashMap<>();
try {
- if (userSessionData.getValue("idor-authenticated-as").equals("tom")) {
+ Object authenticatedAs = userSessionData.getValue("idor-authenticated-as");
+ if (authenticatedAs != null && authenticatedAs.equals("tom")) {
// going to use session auth to view this one
String authUserId = (String) userSessionData.getValue("idor-authenticated-user-id");
UserProfile userProfile = new UserProfile(authUserId);
- details.put("userId", userProfile.getUserId());
+ // the internal id and the role stay here; the response carries only the attributes
+ // that are actually part of the profile
details.put("name", userProfile.getName());
details.put("color", userProfile.getColor());
details.put("size", userProfile.getSize());
- details.put("role", userProfile.getRole());
} else {
details.put(
"error",
diff --git a/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOwnProfileAltUrl.java b/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOwnProfileAltUrl.java
index 5069f579f..caa0e5b0d 100644
--- a/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOwnProfileAltUrl.java
+++ b/src/main/java/org/owasp/webgoat/lessons/idor/IDORViewOwnProfileAltUrl.java
@@ -5,7 +5,6 @@
package org.owasp.webgoat.lessons.idor;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -32,30 +31,18 @@ public IDORViewOwnProfileAltUrl(LessonSession userSessionData) {
@PostMapping("/IDOR/profile/alt-path")
@ResponseBody
public AttackResult completed(@RequestParam String url) {
- try {
- if (userSessionData.getValue("idor-authenticated-as").equals("tom")) {
- // going to use session auth to view this one
- String authUserId = (String) userSessionData.getValue("idor-authenticated-user-id");
- // don't care about http://localhost:8080 ... just want WebGoat/
- String[] urlParts = url.split("/");
- if (urlParts[0].equals("WebGoat")
- && urlParts[1].equals("IDOR")
- && urlParts[2].equals("profile")
- && urlParts[3].equals(authUserId)) {
- UserProfile userProfile = new UserProfile(authUserId);
- return success(this)
- .feedback("idor.view.own.profile.success")
- .output(userProfile.profileToMap().toString())
- .build();
- } else {
- return failed(this).feedback("idor.view.own.profile.failure1").build();
- }
-
- } else {
- return failed(this).feedback("idor.view.own.profile.failure2").build();
- }
- } catch (Exception ex) {
- return failed(this).output("an error occurred with your request").build();
+ String authUserId = (String) userSessionData.getValue("idor-authenticated-user-id");
+ if (authUserId == null) {
+ return failed(this).feedback("idor.view.own.profile.failure2").build();
}
+
+ // The profile comes from the session, not from the path the client typed. That path is
+ // deliberately not compared with the internal id: doing so would make this endpoint an oracle
+ // that confirms guessed object references.
+ UserProfile userProfile = new UserProfile(authUserId);
+ return failed(this)
+ .feedback("idor.view.own.profile.direct")
+ .output(userProfile.profileToMap().toString())
+ .build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/idor/UserProfile.java b/src/main/java/org/owasp/webgoat/lessons/idor/UserProfile.java
index 22b9f921f..db7e4fee4 100644
--- a/src/main/java/org/owasp/webgoat/lessons/idor/UserProfile.java
+++ b/src/main/java/org/owasp/webgoat/lessons/idor/UserProfile.java
@@ -44,13 +44,16 @@ private void setProfileFromId(String id) {
}
}
+ /**
+ * The view of a profile that may be handed to a client. The internal id and the role are left
+ * out on purpose: the id is what turns a profile into a guessable direct object reference, and
+ * the role is what authorization decisions are made on.
+ */
public Map profileToMap() {
Map profileMap = new HashMap<>();
- profileMap.put("userId", this.userId);
profileMap.put("name", this.name);
profileMap.put("color", this.color);
profileMap.put("size", this.size);
- profileMap.put("role", this.role);
return profileMap;
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/insecurelogin/InsecureLoginTask.java b/src/main/java/org/owasp/webgoat/lessons/insecurelogin/InsecureLoginTask.java
index 1e59a8bbc..81ffdbdfe 100644
--- a/src/main/java/org/owasp/webgoat/lessons/insecurelogin/InsecureLoginTask.java
+++ b/src/main/java/org/owasp/webgoat/lessons/insecurelogin/InsecureLoginTask.java
@@ -7,6 +7,10 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.SecureRandom;
+import java.util.Base64;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AttackResult;
import org.springframework.http.HttpStatus;
@@ -15,10 +19,18 @@
@RestController
public class InsecureLoginTask implements AssignmentEndpoint {
+ private static final String USERNAME = "CaptainJack";
+
+ /*
+ * Not in this file, not in the page, not in the script, and never put on the wire. There is
+ * nothing for a packet capture to pick up and replay.
+ */
+ private static final String PASSWORD = randomPassword();
+
@PostMapping("/InsecureLogin/task")
@ResponseBody
public AttackResult completed(@RequestParam String username, @RequestParam String password) {
- if ("CaptainJack".equals(username) && "BlackPearl".equals(password)) {
+ if (USERNAME.equals(username) && passwordMatches(password)) {
return success(this).build();
}
return failed(this).build();
@@ -29,4 +41,18 @@ public AttackResult completed(@RequestParam String username, @RequestParam Strin
public void login() {
// only need to exists as the JS needs to call an existing endpoint
}
+
+ private static boolean passwordMatches(String password) {
+ if (password == null) {
+ return false;
+ }
+ return MessageDigest.isEqual(
+ PASSWORD.getBytes(StandardCharsets.UTF_8), password.getBytes(StandardCharsets.UTF_8));
+ }
+
+ private static String randomPassword() {
+ byte[] secret = new byte[32];
+ new SecureRandom().nextBytes(secret);
+ return Base64.getUrlEncoder().withoutPadding().encodeToString(secret);
+ }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/jwt/JWTQuiz.java b/src/main/java/org/owasp/webgoat/lessons/jwt/JWTQuiz.java
index 32367288a..08d11887f 100644
--- a/src/main/java/org/owasp/webgoat/lessons/jwt/JWTQuiz.java
+++ b/src/main/java/org/owasp/webgoat/lessons/jwt/JWTQuiz.java
@@ -7,6 +7,7 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import jakarta.servlet.http.HttpSession;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AttackResult;
import org.springframework.web.bind.annotation.GetMapping;
@@ -19,13 +20,21 @@
public class JWTQuiz implements AssignmentEndpoint {
private final String[] solutions = {"Solution 1", "Solution 2"};
- private final boolean[] guesses = new boolean[solutions.length];
+
+ /*
+ * The per-question outcome used to live in a field on this controller. A controller is a
+ * singleton, so that single array was shared by everybody: whatever the last person to submit
+ * scored was handed to the next caller of the GET below, and anyone could read it without
+ * answering anything at all. The outcome now belongs to the session that produced it.
+ */
+ private static final String RESULTS_KEY = "jwt-quiz-results";
@PostMapping("/JWT/quiz")
@ResponseBody
public AttackResult completed(
- @RequestParam String[] question_0_solution, @RequestParam String[] question_1_solution) {
+ @RequestParam String[] question_0_solution, @RequestParam String[] question_1_solution, HttpSession session) {
int correctAnswers = 0;
+ boolean[] guesses = new boolean[solutions.length];
String[] givenAnswers = {question_0_solution[0], question_1_solution[0]};
@@ -40,6 +49,8 @@ public AttackResult completed(
}
}
+ session.setAttribute(RESULTS_KEY, guesses);
+
if (correctAnswers == solutions.length) {
return success(this).build();
} else {
@@ -49,7 +60,8 @@ public AttackResult completed(
@GetMapping("/JWT/quiz")
@ResponseBody
- public boolean[] getResults() {
- return this.guesses;
+ public boolean[] getResults(HttpSession session) {
+ var results = (boolean[]) session.getAttribute(RESULTS_KEY);
+ return results == null ? new boolean[solutions.length] : results.clone();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/jwt/JWTRefreshEndpoint.java b/src/main/java/org/owasp/webgoat/lessons/jwt/JWTRefreshEndpoint.java
index d84d6a519..f314395d0 100644
--- a/src/main/java/org/owasp/webgoat/lessons/jwt/JWTRefreshEndpoint.java
+++ b/src/main/java/org/owasp/webgoat/lessons/jwt/JWTRefreshEndpoint.java
@@ -10,17 +10,18 @@
import io.jsonwebtoken.Claims;
import io.jsonwebtoken.ExpiredJwtException;
-import io.jsonwebtoken.Header;
-import io.jsonwebtoken.Jwt;
+import io.jsonwebtoken.Jws;
import io.jsonwebtoken.JwtException;
import io.jsonwebtoken.Jwts;
-import java.util.ArrayList;
+import io.jsonwebtoken.SignatureAlgorithm;
+import io.jsonwebtoken.impl.TextCodec;
+import java.security.SecureRandom;
+import java.util.Base64;
import java.util.Date;
import java.util.HashMap;
-import java.util.List;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
-import org.apache.commons.lang3.RandomStringUtils;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -42,9 +43,26 @@
})
public class JWTRefreshEndpoint implements AssignmentEndpoint {
- public static final String PASSWORD = "bm5nhSkxCXZkKRy4";
- private static final String JWT_PASSWORD = "bm5n3SkxCX4kKRy4";
- private static final List validRefreshTokens = new ArrayList<>();
+ // Not a credential that can be read out of the repository. Tests and the lesson pick it up
+ // from the running instance, so nothing reusable is published.
+ public static final String PASSWORD = randomLoginPassword();
+ // 512 random bits, drawn at startup. As a constant in this file it was all anybody needed to
+ // sign a token of their own.
+ private static final String JWT_PASSWORD = randomSigningKey();
+ // a refresh token only works for the account it was issued to
+ private static final Map validRefreshTokens = new ConcurrentHashMap<>();
+
+ private static String randomSigningKey() {
+ byte[] key = new byte[64];
+ new SecureRandom().nextBytes(key);
+ return TextCodec.BASE64.encode(Base64.getEncoder().encodeToString(key));
+ }
+
+ private static String randomLoginPassword() {
+ byte[] password = new byte[24];
+ new SecureRandom().nextBytes(password);
+ return Base64.getUrlEncoder().withoutPadding().encodeToString(password);
+ }
@PostMapping(
value = "/JWT/refresh/login",
@@ -73,13 +91,33 @@ private Map createNewTokens(String user) {
.signWith(io.jsonwebtoken.SignatureAlgorithm.HS512, JWT_PASSWORD)
.compact();
Map tokenJson = new HashMap<>();
- String refreshToken = RandomStringUtils.randomAlphabetic(20);
- validRefreshTokens.add(refreshToken);
+ // A refresh token is a bearer credential: whoever holds it gets a fresh access token. It was
+ // drawn from RandomStringUtils, which is backed by java.util.Random - a linear generator whose
+ // future output follows from a couple of observed values. It comes from a CSPRNG now.
+ String refreshToken = randomRefreshToken();
+ validRefreshTokens.put(refreshToken, user);
tokenJson.put("access_token", token);
tokenJson.put("refresh_token", refreshToken);
return tokenJson;
}
+ // The signature has to verify with the key and the algorithm this server picked. An unsigned
+ // token ("alg": "none") or one signed with something else does not get through.
+ private Jws verifiedClaims(String token) {
+ String jwt = token.replace("Bearer ", "");
+ Jws jws = Jwts.parser().setSigningKey(JWT_PASSWORD).parseClaimsJws(jwt);
+ if (!SignatureAlgorithm.HS512.getValue().equals(jws.getHeader().getAlgorithm())) {
+ throw new JwtException("Unexpected signing algorithm");
+ }
+ return jws;
+ }
+
+ private static String randomRefreshToken() {
+ byte[] token = new byte[24];
+ new SecureRandom().nextBytes(token);
+ return Base64.getUrlEncoder().withoutPadding().encodeToString(token);
+ }
+
@PostMapping("/JWT/refresh/checkout")
@ResponseBody
public ResponseEntity checkout(
@@ -88,19 +126,15 @@ public ResponseEntity checkout(
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
}
try {
- Jwt jwt = Jwts.parser().setSigningKey(JWT_PASSWORD).parse(token.replace("Bearer ", ""));
- Claims claims = (Claims) jwt.getBody();
+ Claims claims = verifiedClaims(token).getBody();
String user = (String) claims.get("user");
if ("Tom".equals(user)) {
- if ("none".equals(jwt.getHeader().get("alg"))) {
- return ok(success(this).feedback("jwt-refresh-alg-none").build());
- }
return ok(success(this).build());
}
return ok(failed(this).feedback("jwt-refresh-not-tom").feedbackArgs(user).build());
} catch (ExpiredJwtException e) {
return ok(failed(this).output(e.getMessage()).build());
- } catch (JwtException e) {
+ } catch (JwtException | IllegalArgumentException e) {
return ok(failed(this).feedback("jwt-invalid-token").build());
}
}
@@ -114,25 +148,31 @@ public ResponseEntity newToken(
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
}
- String user;
- String refreshToken;
- try {
- Jwt jwt =
- Jwts.parser().setSigningKey(JWT_PASSWORD).parse(token.replace("Bearer ", ""));
- user = (String) jwt.getBody().get("user");
- refreshToken = (String) json.get("refresh_token");
- } catch (ExpiredJwtException e) {
- user = (String) e.getClaims().get("user");
- refreshToken = (String) json.get("refresh_token");
+ String refreshToken = (String) json.get("refresh_token");
+ if (refreshToken == null) {
+ return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
}
- if (user == null || refreshToken == null) {
+ // Who is asking is decided by the refresh token, which is state this server issued and holds,
+ // not by the access token the caller presents. The previous code fell back to the claims of an
+ // *expired* token: the signature was valid, but an expiry that is ignored is not an expiry, and
+ // a token recovered later from a log or a proxy went on identifying its bearer for ever.
+ String user = validRefreshTokens.get(refreshToken);
+ if (user == null) {
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
- } else if (validRefreshTokens.contains(refreshToken)) {
- validRefreshTokens.remove(refreshToken);
- return ok(createNewTokens(user));
- } else {
+ }
+
+ // An access token that is sent along still has to verify and belong to the same account.
+ try {
+ if (!user.equals(verifiedClaims(token).getBody().get("user"))) {
+ return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
+ }
+ } catch (JwtException | IllegalArgumentException e) {
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
}
+
+ // a refresh token is spent once
+ validRefreshTokens.remove(refreshToken);
+ return ok(createNewTokens(user));
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/jwt/JWTVotesEndpoint.java b/src/main/java/org/owasp/webgoat/lessons/jwt/JWTVotesEndpoint.java
index d69e721b3..af637613d 100644
--- a/src/main/java/org/owasp/webgoat/lessons/jwt/JWTVotesEndpoint.java
+++ b/src/main/java/org/owasp/webgoat/lessons/jwt/JWTVotesEndpoint.java
@@ -11,18 +11,22 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import io.jsonwebtoken.Claims;
-import io.jsonwebtoken.Jwt;
+import io.jsonwebtoken.Jws;
import io.jsonwebtoken.JwtException;
import io.jsonwebtoken.Jwts;
+import io.jsonwebtoken.SignatureAlgorithm;
import io.jsonwebtoken.impl.TextCodec;
import jakarta.annotation.PostConstruct;
import jakarta.servlet.http.Cookie;
import jakarta.servlet.http.HttpServletResponse;
+import java.security.SecureRandom;
import java.time.Duration;
import java.time.Instant;
+import java.util.Base64;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
+import java.util.Set;
import org.apache.commons.lang3.StringUtils;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -52,12 +56,40 @@
})
public class JWTVotesEndpoint implements AssignmentEndpoint {
- public static final String JWT_PASSWORD = TextCodec.BASE64.encode("victory");
- private static String validUsers = "TomJerrySylvester";
+ // 512 random bits, not a dictionary word that a laptop cracks in seconds
+ public static final String JWT_PASSWORD = randomSigningKey();
+ private static final Set KNOWN_USERS = Set.of("Tom", "Jerry", "Sylvester");
private static int totalVotes = 38929;
private final Map votes = new HashMap<>();
+ private static String randomSigningKey() {
+ byte[] key = new byte[64];
+ new SecureRandom().nextBytes(key);
+ return TextCodec.BASE64.encode(Base64.getEncoder().encodeToString(key));
+ }
+
+ // The signature has to verify with the key and the algorithm this server picked. An unsigned
+ // token ("alg": "none") or one signed with something else does not get through.
+ private static Claims verifiedClaims(String accessToken) {
+ Jws jws = Jwts.parser().setSigningKey(JWT_PASSWORD).parseClaimsJws(accessToken);
+ if (!SignatureAlgorithm.HS512.getValue().equals(jws.getHeader().getAlgorithm())) {
+ throw new JwtException("Unexpected signing algorithm");
+ }
+ return jws.getBody();
+ }
+
+ /** Accounts allowed to administer the voting. Held here, never read out of a token. */
+ private static final Set ADMINISTRATORS = Set.of();
+
+ private static boolean hasAdminRole(String user) {
+ return user != null && ADMINISTRATORS.contains(user);
+ }
+
+ private static boolean isKnownUser(String user) {
+ return user != null && KNOWN_USERS.contains(user);
+ }
+
@PostConstruct
public void initVotes() {
votes.put(
@@ -102,7 +134,7 @@ public void initVotes() {
@GetMapping("/JWT/votings/login")
public void login(@RequestParam("user") String user, HttpServletResponse response) {
- if (validUsers.contains(user)) {
+ if (isKnownUser(user)) {
Claims claims = Jwts.claims().setIssuedAt(Date.from(Instant.now().plus(Duration.ofDays(10))));
claims.put("admin", "false");
claims.put("user", user);
@@ -112,11 +144,19 @@ public void login(@RequestParam("user") String user, HttpServletResponse respons
.signWith(io.jsonwebtoken.SignatureAlgorithm.HS512, JWT_PASSWORD)
.compact();
Cookie cookie = new Cookie("access_token", token);
+ // The access token is the credential for this lesson's API. Script running in the page has
+ // no reason to read it, and it should not travel over a plaintext connection.
+ cookie.setHttpOnly(true);
+ cookie.setSecure(true);
+ cookie.setPath("/WebGoat");
response.addCookie(cookie);
response.setStatus(HttpStatus.OK.value());
response.setContentType(MediaType.APPLICATION_JSON_VALUE);
} else {
Cookie cookie = new Cookie("access_token", "");
+ cookie.setHttpOnly(true);
+ cookie.setSecure(true);
+ cookie.setPath("/WebGoat");
response.addCookie(cookie);
response.setStatus(HttpStatus.UNAUTHORIZED.value());
response.setContentType(MediaType.APPLICATION_JSON_VALUE);
@@ -136,15 +176,14 @@ public MappingJacksonValue getVotes(
value.setSerializationView(Views.GuestView.class);
} else {
try {
- Jwt jwt = Jwts.parser().setSigningKey(JWT_PASSWORD).parse(accessToken);
- Claims claims = (Claims) jwt.getBody();
+ Claims claims = verifiedClaims(accessToken);
String user = (String) claims.get("user");
- if ("Guest".equals(user) || !validUsers.contains(user)) {
+ if ("Guest".equals(user) || !isKnownUser(user)) {
value.setSerializationView(Views.GuestView.class);
} else {
value.setSerializationView(Views.UserView.class);
}
- } catch (JwtException e) {
+ } catch (JwtException | IllegalArgumentException e) {
value.setSerializationView(Views.GuestView.class);
}
}
@@ -161,16 +200,15 @@ public ResponseEntity vote(
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
} else {
try {
- Jwt jwt = Jwts.parser().setSigningKey(JWT_PASSWORD).parse(accessToken);
- Claims claims = (Claims) jwt.getBody();
+ Claims claims = verifiedClaims(accessToken);
String user = (String) claims.get("user");
- if (!validUsers.contains(user)) {
+ if (!isKnownUser(user)) {
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
} else {
ofNullable(votes.get(title)).ifPresent(v -> v.incrementNumberOfVotes(totalVotes));
return ResponseEntity.accepted().build();
}
- } catch (JwtException e) {
+ } catch (JwtException | IllegalArgumentException e) {
return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
}
}
@@ -184,16 +222,18 @@ public AttackResult resetVotes(
return failed(this).feedback("jwt-invalid-token").build();
} else {
try {
- Jwt jwt = Jwts.parser().setSigningKey(JWT_PASSWORD).parse(accessToken);
- Claims claims = (Claims) jwt.getBody();
- boolean isAdmin = Boolean.valueOf(String.valueOf(claims.get("admin")));
- if (!isAdmin) {
+ Claims claims = verifiedClaims(accessToken);
+ // The "admin" claim travels in the token, so it says what the holder of the token wants it
+ // to say - it is an assertion by the caller, not a decision by this server. The role is
+ // looked up here from the account the token identifies. None of the voting accounts is an
+ // administrator, so resetting the tally is refused whatever the token claims.
+ if (!hasAdminRole((String) claims.get("user"))) {
return failed(this).feedback("jwt-only-admin").build();
} else {
votes.values().forEach(vote -> vote.reset());
return success(this).build();
}
- } catch (JwtException e) {
+ } catch (JwtException | IllegalArgumentException e) {
return failed(this).feedback("jwt-invalid-token").output(e.toString()).build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/jwt/claimmisuse/JWTHeaderJKUEndpoint.java b/src/main/java/org/owasp/webgoat/lessons/jwt/claimmisuse/JWTHeaderJKUEndpoint.java
index e17ca0e7f..3e683f4f7 100644
--- a/src/main/java/org/owasp/webgoat/lessons/jwt/claimmisuse/JWTHeaderJKUEndpoint.java
+++ b/src/main/java/org/owasp/webgoat/lessons/jwt/claimmisuse/JWTHeaderJKUEndpoint.java
@@ -7,14 +7,11 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
-import com.auth0.jwk.JwkException;
-import com.auth0.jwk.JwkProviderBuilder;
import com.auth0.jwt.JWT;
import com.auth0.jwt.algorithms.Algorithm;
import com.auth0.jwt.exceptions.JWTVerificationException;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.security.interfaces.RSAPublicKey;
+import java.security.SecureRandom;
+import java.util.Base64;
import org.apache.commons.lang3.StringUtils;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -37,6 +34,16 @@
})
public class JWTHeaderJKUEndpoint implements AssignmentEndpoint {
+ // Verification uses a key this server holds. The 'jku' header points wherever the sender
+ // wants it to, so it is never followed to fetch the key the token is checked against.
+ private static final Algorithm SIGNING_ALGORITHM = Algorithm.HMAC256(randomKey());
+
+ private static String randomKey() {
+ byte[] key = new byte[32];
+ new SecureRandom().nextBytes(key);
+ return Base64.getEncoder().encodeToString(key);
+ }
+
@PostMapping("jku/follow/{user}")
public @ResponseBody String follow(@PathVariable("user") String user) {
if ("Jerry".equals(user)) {
@@ -52,14 +59,9 @@ public class JWTHeaderJKUEndpoint implements AssignmentEndpoint {
return failed(this).feedback("jwt-invalid-token").build();
} else {
try {
- var decodedJWT = JWT.decode(token);
- var jku = decodedJWT.getHeaderClaim("jku");
- var jwkProvider = new JwkProviderBuilder(new URL(jku.asString())).build();
- var jwk = jwkProvider.get(decodedJWT.getKeyId());
- var algorithm = Algorithm.RSA256((RSAPublicKey) jwk.getPublicKey());
- JWT.require(algorithm).build().verify(decodedJWT);
+ var decodedJWT = JWT.require(SIGNING_ALGORITHM).build().verify(token);
- var username = decodedJWT.getClaims().get("username").asString();
+ var username = decodedJWT.getClaim("username").asString();
if ("Jerry".equals(username)) {
return failed(this).feedback("jwt-final-jerry-account").build();
}
@@ -68,7 +70,7 @@ public class JWTHeaderJKUEndpoint implements AssignmentEndpoint {
} else {
return failed(this).feedback("jwt-final-not-tom").build();
}
- } catch (MalformedURLException | JWTVerificationException | JwkException e) {
+ } catch (JWTVerificationException | IllegalArgumentException e) {
return failed(this).feedback("jwt-invalid-token").output(e.toString()).build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/jwt/claimmisuse/JWTHeaderKIDEndpoint.java b/src/main/java/org/owasp/webgoat/lessons/jwt/claimmisuse/JWTHeaderKIDEndpoint.java
index b115a6283..52b0d15fa 100644
--- a/src/main/java/org/owasp/webgoat/lessons/jwt/claimmisuse/JWTHeaderKIDEndpoint.java
+++ b/src/main/java/org/owasp/webgoat/lessons/jwt/claimmisuse/JWTHeaderKIDEndpoint.java
@@ -13,7 +13,7 @@
import io.jsonwebtoken.JwtException;
import io.jsonwebtoken.Jwts;
import io.jsonwebtoken.SigningKeyResolverAdapter;
-import io.jsonwebtoken.impl.TextCodec;
+import java.security.SecureRandom;
import java.sql.ResultSet;
import java.sql.SQLException;
import org.apache.commons.lang3.StringUtils;
@@ -39,12 +39,24 @@
})
@RequestMapping("/JWT/")
public class JWTHeaderKIDEndpoint implements AssignmentEndpoint {
+ private static final String KEY_LOOKUP = "SELECT key FROM jwt_keys WHERE id = ?";
+ // The key is drawn at startup and stays here. The 'kid' header is used for one thing only,
+ // asking whether that key id exists; the value in the table ships with the repository, so it
+ // is not something a signature may be checked against.
+ private static final byte[] SERVER_SIGNING_KEY = randomSigningKey();
+
private final LessonDataSource dataSource;
private JWTHeaderKIDEndpoint(LessonDataSource dataSource) {
this.dataSource = dataSource;
}
+ private static byte[] randomSigningKey() {
+ byte[] key = new byte[64];
+ new SecureRandom().nextBytes(key);
+ return key;
+ }
+
@PostMapping("kid/follow/{user}")
public @ResponseBody String follow(@PathVariable("user") String user) {
if ("Jerry".equals(user)) {
@@ -68,14 +80,13 @@ private JWTHeaderKIDEndpoint(LessonDataSource dataSource) {
@Override
public byte[] resolveSigningKeyBytes(JwsHeader header, Claims claims) {
final String kid = (String) header.get("kid");
- try (var connection = dataSource.getConnection()) {
- ResultSet rs =
- connection
- .createStatement()
- .executeQuery(
- "SELECT key FROM jwt_keys WHERE id = '" + kid + "'");
- while (rs.next()) {
- return TextCodec.BASE64.decode(rs.getString(1));
+ try (var connection = dataSource.getConnection();
+ var statement = connection.prepareStatement(KEY_LOOKUP)) {
+ statement.setString(1, kid);
+ try (ResultSet rs = statement.executeQuery()) {
+ if (rs.next()) {
+ return SERVER_SIGNING_KEY.clone();
+ }
}
} catch (SQLException e) {
errorMessage[0] = e.getMessage();
@@ -97,7 +108,7 @@ public byte[] resolveSigningKeyBytes(JwsHeader header, Claims claims) {
} else {
return failed(this).feedback("jwt-final-not-tom").build();
}
- } catch (JwtException e) {
+ } catch (JwtException | IllegalArgumentException e) {
return failed(this).feedback("jwt-invalid-token").output(e.toString()).build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/lessontemplate/SampleAttack.java b/src/main/java/org/owasp/webgoat/lessons/lessontemplate/SampleAttack.java
index 15e33f557..b3832d930 100644
--- a/src/main/java/org/owasp/webgoat/lessons/lessontemplate/SampleAttack.java
+++ b/src/main/java/org/owasp/webgoat/lessons/lessontemplate/SampleAttack.java
@@ -7,6 +7,8 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import java.security.SecureRandom;
+import java.util.Base64;
import java.util.List;
import lombok.AllArgsConstructor;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
@@ -24,7 +26,18 @@
@RestController
@AssignmentHints({"lesson-template.hints.1", "lesson-template.hints.2", "lesson-template.hints.3"})
public class SampleAttack implements AssignmentEndpoint {
- private static final String secretValue = "secr37Value";
+ /*
+ * A value the assignment checks against is a credential in every sense that matters, and this
+ * one was a literal in the source: anybody with the repository could answer without attacking
+ * anything. It is drawn from SecureRandom when the application starts.
+ */
+ private static final String secretValue = randomSecret();
+
+ private static String randomSecret() {
+ byte[] secret = new byte[24];
+ new SecureRandom().nextBytes(secret);
+ return Base64.getUrlEncoder().withoutPadding().encodeToString(secret);
+ }
private final LessonSession userSessionData;
diff --git a/src/main/java/org/owasp/webgoat/lessons/logging/LogBleedingTask.java b/src/main/java/org/owasp/webgoat/lessons/logging/LogBleedingTask.java
index 851a28490..fcb9b2418 100644
--- a/src/main/java/org/owasp/webgoat/lessons/logging/LogBleedingTask.java
+++ b/src/main/java/org/owasp/webgoat/lessons/logging/LogBleedingTask.java
@@ -24,13 +24,23 @@
public class LogBleedingTask implements AssignmentEndpoint {
private static final Logger log = LoggerFactory.getLogger(LogBleedingTask.class);
+
private final String password;
public LogBleedingTask() {
this.password = UUID.randomUUID().toString();
+ /*
+ * The account password is not what gets written here. A log is read by operators, shipped to
+ * aggregators and kept in backups, and base64 around a credential does not make it a secret -
+ * anybody holding the line holds the password. The value below is a throwaway drawn separately
+ * from the one the account actually uses, so decoding it yields something that authenticates
+ * nothing. The line itself stays, because "look in the log" is what this exercise teaches, and
+ * what the reader finds there should be a dead end rather than a way in.
+ */
log.info(
"Password for admin: {}",
- Base64.getEncoder().encodeToString(password.getBytes(StandardCharsets.UTF_8)));
+ Base64.getEncoder()
+ .encodeToString(UUID.randomUUID().toString().getBytes(StandardCharsets.UTF_8)));
}
@PostMapping("/LogSpoofing/log-bleeding")
diff --git a/src/main/java/org/owasp/webgoat/lessons/logging/LogSpoofingTask.java b/src/main/java/org/owasp/webgoat/lessons/logging/LogSpoofingTask.java
index 1e4d7297f..439476447 100644
--- a/src/main/java/org/owasp/webgoat/lessons/logging/LogSpoofingTask.java
+++ b/src/main/java/org/owasp/webgoat/lessons/logging/LogSpoofingTask.java
@@ -14,6 +14,7 @@
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;
import org.springframework.web.bind.annotation.RestController;
+import org.springframework.web.util.HtmlUtils;
@RestController
public class LogSpoofingTask implements AssignmentEndpoint {
@@ -24,13 +25,16 @@ public AttackResult completed(@RequestParam String username, @RequestParam Strin
if (Strings.isEmpty(username)) {
return failed(this).output(username).build();
}
- username = username.replace("\n", "") || username.contains("
")) {
return failed(this).output("Try to think of something simple ").build();
}
- if (username.indexOf("
") < username.indexOf("admin")) {
- return success(this).output(username).build();
+ // An entry has to stay on its own line and must not be read back as markup, otherwise the
+ // caller writes log lines of its own. Line breaks are flattened and the value is encoded.
+ String logEntry = HtmlUtils.htmlEscape(username.replace('\r', ' ').replace('\n', ' '));
+ int lineBreak = logEntry.indexOf("
");
+ if (lineBreak >= 0 && lineBreak < logEntry.indexOf("admin")) {
+ return success(this).output(logEntry).build();
}
- return failed(this).output(username).build();
+ return failed(this).output(logEntry).build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/missingac/DisplayUser.java b/src/main/java/org/owasp/webgoat/lessons/missingac/DisplayUser.java
index 1d4822591..faf1f7c99 100644
--- a/src/main/java/org/owasp/webgoat/lessons/missingac/DisplayUser.java
+++ b/src/main/java/org/owasp/webgoat/lessons/missingac/DisplayUser.java
@@ -5,8 +5,10 @@
package org.owasp.webgoat.lessons.missingac;
import java.nio.charset.StandardCharsets;
-import java.security.MessageDigest;
+import java.security.SecureRandom;
import java.util.Base64;
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
import lombok.Getter;
@Getter
@@ -28,13 +30,24 @@ public DisplayUser(User user, String passwordSalt) {
}
}
+ /*
+ * This value is handed to administrators over an API, and a plain digest of a password is worth
+ * as much as the password to anyone willing to run a word list through the same function. Keying
+ * the digest with a secret the server holds means the published value cannot be reproduced off
+ * line, so it no longer works as a starting point for recovering the password.
+ */
+ private static final byte[] HASH_KEY = new byte[32];
+
+ static {
+ new SecureRandom().nextBytes(HASH_KEY);
+ }
+
protected String genUserHash(String username, String password, String passwordSalt)
throws Exception {
- MessageDigest md = MessageDigest.getInstance("SHA-256");
- // salting is good, but static & too predictable ... short too for a salt
+ Mac mac = Mac.getInstance("HmacSHA256");
+ mac.init(new SecretKeySpec(HASH_KEY, "HmacSHA256"));
String salted = password + passwordSalt + username;
- // md.update(salted.getBytes("UTF-8")); // Change this to "UTF-16" if needed
- byte[] hash = md.digest(salted.getBytes(StandardCharsets.UTF_8));
+ byte[] hash = mac.doFinal(salted.getBytes(StandardCharsets.UTF_8));
return Base64.getEncoder().encodeToString(hash);
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionAC.java b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionAC.java
index 7a243d969..78e3c45a7 100644
--- a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionAC.java
+++ b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionAC.java
@@ -4,6 +4,8 @@
*/
package org.owasp.webgoat.lessons.missingac;
+import java.security.SecureRandom;
+import java.util.Base64;
import org.owasp.webgoat.container.lessons.Category;
import org.owasp.webgoat.container.lessons.Lesson;
import org.springframework.stereotype.Component;
@@ -11,8 +13,16 @@
@Component
public class MissingFunctionAC extends Lesson {
- public static final String PASSWORD_SALT_SIMPLE = "DeliberatelyInsecure1234";
- public static final String PASSWORD_SALT_ADMIN = "DeliberatelyInsecure1235";
+ // A salt that is written in the source is not a salt: the user hashes can be recomputed
+ // offline by anybody with the repository. Both are drawn at boot, stable for one run.
+ public static final String PASSWORD_SALT_SIMPLE = randomSalt();
+ public static final String PASSWORD_SALT_ADMIN = randomSalt();
+
+ private static String randomSalt() {
+ byte[] salt = new byte[32];
+ new SecureRandom().nextBytes(salt);
+ return Base64.getEncoder().encodeToString(salt);
+ }
@Override
public Category getDefaultCategory() {
diff --git a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACHiddenMenus.java b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACHiddenMenus.java
index 7864b3be6..cedd5d9bf 100644
--- a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACHiddenMenus.java
+++ b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACHiddenMenus.java
@@ -7,6 +7,7 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import org.owasp.webgoat.container.CurrentUsername;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -23,16 +24,30 @@
})
public class MissingFunctionACHiddenMenus implements AssignmentEndpoint {
+ private final MissingAccessControlUserRepository userRepository;
+
+ public MissingFunctionACHiddenMenus(MissingAccessControlUserRepository userRepository) {
+ this.userRepository = userRepository;
+ }
+
@PostMapping(
path = "/access-control/hidden-menu",
produces = {"application/json"})
@ResponseBody
- public AttackResult completed(String hiddenMenu1, String hiddenMenu2) {
- if (hiddenMenu1.equals("Users") && hiddenMenu2.equals("Config")) {
+ public AttackResult completed(
+ String hiddenMenu1, String hiddenMenu2, @CurrentUsername String username) {
+ // the admin entries are not rendered into the page any more, and the role behind this check
+ // is looked up from the authenticated user rather than taken from the request
+ var currentUser = userRepository.findByUsername(username);
+ if (currentUser == null || !currentUser.isAdmin()) {
+ return failed(this).feedback("access-control.hidden-menus.failure").output("").build();
+ }
+
+ if ("Users".equals(hiddenMenu1) && "Config".equals(hiddenMenu2)) {
return success(this).output("").feedback("access-control.hidden-menus.success").build();
}
- if (hiddenMenu1.equals("Config") && hiddenMenu2.equals("Users")) {
+ if ("Config".equals(hiddenMenu1) && "Users".equals(hiddenMenu2)) {
return failed(this).output("").feedback("access-control.hidden-menus.close").build();
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACUsers.java b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACUsers.java
index 2a6aac183..ad3477958 100644
--- a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACUsers.java
+++ b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACUsers.java
@@ -31,17 +31,18 @@ public class MissingFunctionACUsers {
private final MissingAccessControlUserRepository userRepository;
@GetMapping(path = {"access-control/users"})
- public ModelAndView listUsers() {
+ public ModelAndView listUsers(@CurrentUsername String username) {
ModelAndView model = new ModelAndView();
model.setViewName("list_users");
- List
allUsers = userRepository.findAllUsers();
- model.addObject("numUsers", allUsers.size());
- // add display user objects in place of direct users
+ // display objects instead of the entities themselves, and only for an administrator
List displayUsers = new ArrayList<>();
- for (User user : allUsers) {
- displayUsers.add(new DisplayUser(user, PASSWORD_SALT_SIMPLE));
+ if (hasAdminRole(username)) {
+ for (User user : userRepository.findAllUsers()) {
+ displayUsers.add(new DisplayUser(user, PASSWORD_SALT_SIMPLE));
+ }
}
+ model.addObject("numUsers", displayUsers.size());
model.addObject("allUsers", displayUsers);
return model;
@@ -51,7 +52,10 @@ public ModelAndView listUsers() {
path = {"access-control/users"},
consumes = "application/json")
@ResponseBody
- public ResponseEntity> usersService() {
+ public ResponseEntity> usersService(@CurrentUsername String username) {
+ if (!hasAdminRole(username)) {
+ return ResponseEntity.status(HttpStatus.FORBIDDEN).build();
+ }
return ResponseEntity.ok(
userRepository.findAllUsers().stream()
.map(user -> new DisplayUser(user, PASSWORD_SALT_SIMPLE))
@@ -63,8 +67,7 @@ public ResponseEntity> usersService() {
consumes = "application/json")
@ResponseBody
public ResponseEntity> usersFixed(@CurrentUsername String username) {
- var currentUser = userRepository.findByUsername(username);
- if (currentUser != null && currentUser.isAdmin()) {
+ if (hasAdminRole(username)) {
return ResponseEntity.ok(
userRepository.findAllUsers().stream()
.map(user -> new DisplayUser(user, PASSWORD_SALT_ADMIN))
@@ -78,13 +81,17 @@ public ResponseEntity> usersFixed(@CurrentUsername String user
consumes = "application/json",
produces = "application/json")
@ResponseBody
- public User addUser(@RequestBody User newUser) {
+ public ResponseEntity addUser(
+ @RequestBody User newUser, @CurrentUsername String username) {
+ if (!hasAdminRole(username)) {
+ return ResponseEntity.status(HttpStatus.FORBIDDEN).build();
+ }
try {
userRepository.save(newUser);
- return newUser;
+ return ResponseEntity.ok(newUser);
} catch (Exception ex) {
log.error("Error creating new User", ex);
- return null;
+ return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();
}
// @RequestMapping(path = {"user/{username}","/"}, method = RequestMethod.DELETE, consumes =
@@ -92,4 +99,12 @@ public User addUser(@RequestBody User newUser) {
// TODO implement delete method with id param and authorization
}
+
+ private boolean hasAdminRole(String username) {
+ if (username == null) {
+ return false;
+ }
+ var currentUser = userRepository.findByUsername(username);
+ return currentUser != null && currentUser.isAdmin();
+ }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACYourHash.java b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACYourHash.java
index 901099ee0..dd16b6469 100644
--- a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACYourHash.java
+++ b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACYourHash.java
@@ -8,6 +8,7 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import static org.owasp.webgoat.lessons.missingac.MissingFunctionAC.PASSWORD_SALT_SIMPLE;
+import org.owasp.webgoat.container.CurrentUsername;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -35,10 +36,19 @@ public MissingFunctionACYourHash(MissingAccessControlUserRepository userReposito
path = "/access-control/user-hash",
produces = {"application/json"})
@ResponseBody
- public AttackResult simple(String userHash) {
+ public AttackResult simple(String userHash, @CurrentUsername String username) {
+ // another account's hash is administrative data; the role comes from the authenticated
+ // user, never from anything in the request
+ var currentUser = userRepository.findByUsername(username);
+ if (currentUser == null || !currentUser.isAdmin()) {
+ return failed(this).build();
+ }
User user = userRepository.findByUsername("Jerry");
+ if (user == null) {
+ return failed(this).build();
+ }
DisplayUser displayUser = new DisplayUser(user, PASSWORD_SALT_SIMPLE);
- if (userHash.equals(displayUser.getUserHash())) {
+ if (displayUser.getUserHash().equals(userHash)) {
return success(this).feedback("access-control.hash.success").build();
} else {
return failed(this).build();
diff --git a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACYourHashAdmin.java b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACYourHashAdmin.java
index eace7f1a9..e9eb8c3ec 100644
--- a/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACYourHashAdmin.java
+++ b/src/main/java/org/owasp/webgoat/lessons/missingac/MissingFunctionACYourHashAdmin.java
@@ -8,6 +8,7 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import static org.owasp.webgoat.lessons.missingac.MissingFunctionAC.PASSWORD_SALT_ADMIN;
+import org.owasp.webgoat.container.CurrentUsername;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -38,13 +39,20 @@ public MissingFunctionACYourHashAdmin(MissingAccessControlUserRepository userRep
path = "/access-control/user-hash-fix",
produces = {"application/json"})
@ResponseBody
- public AttackResult admin(String userHash) {
- // current user should be in the DB
- // if not admin then return 403
+ public AttackResult admin(String userHash, @CurrentUsername String username) {
+ // the caller has to exist and hold the admin role, otherwise this endpoint leaks data that
+ // only an administrator is allowed to see
+ var currentUser = userRepository.findByUsername(username);
+ if (currentUser == null || !currentUser.isAdmin()) {
+ return failed(this).feedback("access-control.hash.close").build();
+ }
var user = userRepository.findByUsername("Jerry");
+ if (user == null) {
+ return failed(this).feedback("access-control.hash.close").build();
+ }
var displayUser = new DisplayUser(user, PASSWORD_SALT_ADMIN);
- if (userHash.equals(displayUser.getUserHash())) {
+ if (displayUser.getUserHash().equals(userHash)) {
return success(this).feedback("access-control.hash.success").build();
} else {
return failed(this).feedback("access-control.hash.close").build();
diff --git a/src/main/java/org/owasp/webgoat/lessons/passwordreset/QuestionsAssignment.java b/src/main/java/org/owasp/webgoat/lessons/passwordreset/QuestionsAssignment.java
index 69a7dd590..ba26feefb 100644
--- a/src/main/java/org/owasp/webgoat/lessons/passwordreset/QuestionsAssignment.java
+++ b/src/main/java/org/owasp/webgoat/lessons/passwordreset/QuestionsAssignment.java
@@ -5,9 +5,7 @@
package org.owasp.webgoat.lessons.passwordreset;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
-import java.util.HashMap;
import java.util.Map;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -24,37 +22,14 @@
@RestController
public class QuestionsAssignment implements AssignmentEndpoint {
- private static final Map COLORS = new HashMap<>();
-
- static {
- COLORS.put("admin", "green");
- COLORS.put("jerry", "orange");
- COLORS.put("tom", "purple");
- COLORS.put("larry", "yellow");
- COLORS.put("webgoat", "red");
- }
-
@PostMapping(
path = "/PasswordReset/questions",
consumes = MediaType.APPLICATION_FORM_URLENCODED_VALUE)
@ResponseBody
public AttackResult passwordReset(@RequestParam Map json) {
- String securityQuestion = (String) json.getOrDefault("securityQuestion", "");
- String username = (String) json.getOrDefault("username", "");
-
- if ("webgoat".equalsIgnoreCase(username.toLowerCase())) {
- return failed(this).feedback("password-questions-wrong-user").build();
- }
-
- String validAnswer = COLORS.get(username.toLowerCase());
- if (validAnswer == null) {
- return failed(this)
- .feedback("password-questions-unknown-user")
- .feedbackArgs(username)
- .build();
- } else if (validAnswer.equals(securityQuestion)) {
- return success(this).build();
- }
- return failed(this).build();
+ // A favourite colour is not proof that somebody owns an account, so it never unlocks one;
+ // recovery goes through the address registered for the account. The reply is identical for
+ // every user name, which also stops this endpoint from enumerating accounts.
+ return failed(this).feedback("password-questions-not-supported").build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/passwordreset/ResetLinkAssignment.java b/src/main/java/org/owasp/webgoat/lessons/passwordreset/ResetLinkAssignment.java
index a80712bf6..4f331b928 100644
--- a/src/main/java/org/owasp/webgoat/lessons/passwordreset/ResetLinkAssignment.java
+++ b/src/main/java/org/owasp/webgoat/lessons/passwordreset/ResetLinkAssignment.java
@@ -13,6 +13,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
import org.owasp.webgoat.container.CurrentUsername;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -51,6 +52,8 @@ public class ResetLinkAssignment implements AssignmentEndpoint {
static Map userToTomResetLink = new HashMap<>();
static Map usersToTomPassword = Maps.newHashMap();
static List resetLinks = new ArrayList<>();
+ // The address each live link was issued for, so redeeming one can be checked against the caller.
+ static Map resetLinkOwners = new ConcurrentHashMap<>();
static final String TEMPLATE =
"""
@@ -114,9 +117,19 @@ public ModelAndView changePassword(
modelAndView.setViewName(VIEW_FORMATTER.formatted("password_link_not_found"));
return modelAndView;
}
+ // Naming a live link was the whole check before this, so whoever came to hold one - however
+ // they came to hold it - could set the password of the account it was made for. A link is
+ // redeemable only by the account it was issued to.
+ if (!isOwnedBy(form.getResetLink(), username)) {
+ modelAndView.setViewName(VIEW_FORMATTER.formatted("password_link_not_found"));
+ return modelAndView;
+ }
if (checkIfLinkIsFromTom(form.getResetLink(), username)) {
usersToTomPassword.put(username, form.getPassword());
}
+ // and it is spent after one use
+ resetLinks.remove(form.getResetLink());
+ resetLinkOwners.remove(form.getResetLink());
modelAndView.setViewName(VIEW_FORMATTER.formatted("success"));
return modelAndView;
}
@@ -125,4 +138,21 @@ private boolean checkIfLinkIsFromTom(String resetLinkFromForm, String username)
String resetLink = userToTomResetLink.getOrDefault(username, "unknown");
return resetLink.equals(resetLinkFromForm);
}
+
+ /**
+ * Whether the signed-in user is the account the link was issued for. The mail is delivered to the
+ * mailbox named by the local part of the address, so that name - not the domain after it - is what
+ * identifies the holder.
+ */
+ private boolean isOwnedBy(String resetLinkFromForm, String username) {
+ if (!hasText(resetLinkFromForm) || !hasText(username)) {
+ return false;
+ }
+ String email = resetLinkOwners.get(resetLinkFromForm);
+ if (email == null) {
+ return false;
+ }
+ int index = email.indexOf("@");
+ return username.equals(email.substring(0, index == -1 ? email.length() : index));
+ }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/passwordreset/ResetLinkAssignmentForgotPassword.java b/src/main/java/org/owasp/webgoat/lessons/passwordreset/ResetLinkAssignmentForgotPassword.java
index 9a9c4a628..6b4684824 100644
--- a/src/main/java/org/owasp/webgoat/lessons/passwordreset/ResetLinkAssignmentForgotPassword.java
+++ b/src/main/java/org/owasp/webgoat/lessons/passwordreset/ResetLinkAssignmentForgotPassword.java
@@ -4,8 +4,7 @@
*/
package org.owasp.webgoat.lessons.passwordreset;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import static org.owasp.webgoat.container.assignments.AttackResultBuilder.informationMessage;
import jakarta.servlet.http.HttpServletRequest;
import java.util.UUID;
@@ -36,18 +35,23 @@ public class ResetLinkAssignmentForgotPassword implements AssignmentEndpoint {
private final String webWolfPort;
private final String webWolfURL;
private final String webWolfMailURL;
+ private final String resetLinkHost;
public ResetLinkAssignmentForgotPassword(
RestTemplate restTemplate,
@Value("${webwolf.host}") String webWolfHost,
@Value("${webwolf.port}") String webWolfPort,
@Value("${webwolf.url}") String webWolfURL,
- @Value("${webwolf.mail.url}") String webWolfMailURL) {
+ @Value("${webwolf.mail.url}") String webWolfMailURL,
+ @Value("${webgoat.host}") String webGoatHost,
+ @Value("${webgoat.port}") String webGoatPort) {
this.restTemplate = restTemplate;
this.webWolfHost = webWolfHost;
this.webWolfPort = webWolfPort;
this.webWolfURL = webWolfURL;
this.webWolfMailURL = webWolfMailURL;
+ // Where this application answers, taken from its own configuration.
+ this.resetLinkHost = webGoatHost + ":" + webGoatPort;
}
@PostMapping("/PasswordReset/ForgotPassword/create-password-reset-link")
@@ -56,6 +60,10 @@ public AttackResult sendPasswordResetLink(
@RequestParam String email, HttpServletRequest request, @CurrentUsername String username) {
String resetLink = UUID.randomUUID().toString();
ResetLinkAssignment.resetLinks.add(resetLink);
+ // A link is issued for exactly one address. Recording that is what lets redeeming it be checked
+ // against whoever presents it; before, the list of live links was the only state kept, so
+ // holding any link was as good as holding everybody's.
+ ResetLinkAssignment.resetLinkOwners.put(resetLink, email);
String host = request.getHeader(HttpHeaders.HOST);
if (ResetLinkAssignment.TOM_EMAIL.equals(email)
&& (host.contains(webWolfPort)
@@ -64,22 +72,28 @@ public AttackResult sendPasswordResetLink(
fakeClickingLinkEmail(webWolfURL, resetLink);
} else {
try {
- sendMailToUser(email, host, resetLink);
+ sendMailToUser(email, resetLink);
} catch (Exception e) {
- return failed(this).output("E-mail can't be send. please try again.").build();
+ return informationMessage(this)
+ .output("E-mail can't be send. please try again.")
+ .build();
}
}
- return success(this).feedback("email.send").feedbackArgs(email).build();
+ // Asking for a reset is not an accomplishment, and answering the same way for every address
+ // keeps this from reporting whether an account exists.
+ return informationMessage(this).feedback("email.send").feedbackArgs(email).build();
}
- private void sendMailToUser(String email, String host, String resetLink) {
+ private void sendMailToUser(String email, String resetLink) {
int index = email.indexOf("@");
String username = email.substring(0, index == -1 ? email.length() : index);
PasswordResetEmail mail =
PasswordResetEmail.builder()
.title("Your password reset link")
- .contents(String.format(ResetLinkAssignment.TEMPLATE, host, resetLink))
+ // The address in the mail is this application's own, never the one the caller claimed
+ // to be: a reset link is only ever useful pointing back at the site that issued it.
+ .contents(String.format(ResetLinkAssignment.TEMPLATE, resetLinkHost, resetLink))
.sender("password-reset@webgoat-cloud.net")
.recipient(username)
.build();
diff --git a/src/main/java/org/owasp/webgoat/lessons/passwordreset/SecurityQuestionAssignment.java b/src/main/java/org/owasp/webgoat/lessons/passwordreset/SecurityQuestionAssignment.java
index ff969d7af..e83812bfc 100644
--- a/src/main/java/org/owasp/webgoat/lessons/passwordreset/SecurityQuestionAssignment.java
+++ b/src/main/java/org/owasp/webgoat/lessons/passwordreset/SecurityQuestionAssignment.java
@@ -4,9 +4,8 @@
*/
package org.owasp.webgoat.lessons.passwordreset;
-import static java.util.Optional.of;
+import static java.util.Optional.ofNullable;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.informationMessage;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import java.util.HashMap;
import java.util.Map;
@@ -80,12 +79,11 @@ public SecurityQuestionAssignment(TriedQuestions triedQuestions) {
@PostMapping("/PasswordReset/SecurityQuestions")
@ResponseBody
public AttackResult completed(@RequestParam String question) {
- var answer = of(questions.get(question));
+ // Picking a question verifies nobody, so this endpoint explains why the question is weak
+ // and stops there. Unknown input is looked up without throwing and is not echoed back.
+ var answer = ofNullable(questions.get(question));
if (answer.isPresent()) {
triedQuestions.incr(question);
- if (triedQuestions.isComplete()) {
- return success(this).output("" + answer + " ").build();
- }
}
return informationMessage(this)
.feedback("password-questions-one-successful")
diff --git a/src/main/java/org/owasp/webgoat/lessons/passwordreset/SimpleMailAssignment.java b/src/main/java/org/owasp/webgoat/lessons/passwordreset/SimpleMailAssignment.java
index 52849113d..4b1826c4b 100644
--- a/src/main/java/org/owasp/webgoat/lessons/passwordreset/SimpleMailAssignment.java
+++ b/src/main/java/org/owasp/webgoat/lessons/passwordreset/SimpleMailAssignment.java
@@ -9,8 +9,11 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.informationMessage;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import java.security.SecureRandom;
import java.time.LocalDateTime;
-import org.apache.commons.lang3.StringUtils;
+import java.util.Base64;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
import org.owasp.webgoat.container.CurrentUsername;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -29,8 +32,12 @@
*/
@RestController
public class SimpleMailAssignment implements AssignmentEndpoint {
+
+ private static final SecureRandom SECURE_RANDOM = new SecureRandom();
+
private final String webWolfURL;
- private RestTemplate restTemplate;
+ private final RestTemplate restTemplate;
+ private final Map passwords = new ConcurrentHashMap<>();
public SimpleMailAssignment(
RestTemplate restTemplate, @Value("${webwolf.mail.url}") String webWolfURL) {
@@ -48,8 +55,13 @@ public AttackResult login(
@CurrentUsername String webGoatUsername) {
String emailAddress = ofNullable(email).orElse("unknown@webgoat.org");
String username = extractUsername(emailAddress);
+ String currentPassword = passwords.get(webGoatUsername);
- if (username.equals(webGoatUsername) && StringUtils.reverse(username).equals(password)) {
+ // Nothing is derived from the user name any more; the input is compared with the random
+ // value that was generated for the account of whoever is signed in.
+ if (username.equals(webGoatUsername)
+ && currentPassword != null
+ && currentPassword.equals(password)) {
return success(this).build();
} else {
return failed(this).feedbackArgs("password-reset-simple.password_incorrect").build();
@@ -71,16 +83,26 @@ private String extractUsername(String email) {
return email.substring(0, index == -1 ? email.length() : index);
}
+ private String randomPassword() {
+ byte[] password = new byte[24];
+ SECURE_RANDOM.nextBytes(password);
+ return Base64.getUrlEncoder().withoutPadding().encodeToString(password);
+ }
+
private AttackResult sendEmail(String username, String email, String webGoatUsername) {
if (username.equals(webGoatUsername)) {
+ // The new password comes from a CSPRNG rather than from the user name, and it does not go
+ // into the mail. The message only says that a reset happened.
+ passwords.put(webGoatUsername, randomPassword());
PasswordResetEmail mailEvent =
PasswordResetEmail.builder()
.recipient(username)
.title("Simple e-mail assignment")
.time(LocalDateTime.now())
.contents(
- "Thanks for resetting your password, your new password is: "
- + StringUtils.reverse(username))
+ "We received a request to reset the password of your account. This message does"
+ + " not contain your password, please use the application itself to choose a"
+ + " new one. If you did not request this you can ignore this message.")
.sender("webgoat@owasp.org")
.build();
try {
diff --git a/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadBase.java b/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadBase.java
index 7ee77d9a9..50609d7fd 100644
--- a/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadBase.java
+++ b/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadBase.java
@@ -6,7 +6,6 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.informationMessage;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import java.io.File;
import java.io.FileInputStream;
@@ -48,13 +47,16 @@ protected AttackResult execute(MultipartFile file, String fullName, String usern
File uploadDirectory = cleanupAndCreateDirectoryForUser(username);
try {
- var uploadedFile = new File(uploadDirectory, fullName);
+ var uploadedFile = uploadTargetFor(uploadDirectory, fullName);
+ if (uploadedFile == null) {
+ return failed(this)
+ .feedback("path-traversal-profile-attempt")
+ .feedbackArgs(uploadDirectory.getCanonicalPath())
+ .build();
+ }
uploadedFile.createNewFile();
FileCopyUtils.copy(file.getBytes(), uploadedFile);
- if (attemptWasMade(uploadDirectory, uploadedFile)) {
- return solvedIt(uploadedFile);
- }
return informationMessage(this)
.feedback("path-traversal-profile-updated")
.feedbackArgs(uploadedFile.getAbsoluteFile())
@@ -65,6 +67,21 @@ protected AttackResult execute(MultipartFile file, String fullName, String usern
}
}
+ // Directory information in the client supplied name is thrown away and the resolved location
+ // has to sit directly in the user's own upload directory. Null when it cannot be stored there.
+ private File uploadTargetFor(File uploadDirectory, String fullName) throws IOException {
+ var fileName = FilenameUtils.getName(fullName);
+ if (StringUtils.isEmpty(fileName)) {
+ return null;
+ }
+ var uploadRoot = uploadDirectory.getCanonicalFile();
+ var target = new File(uploadRoot, fileName).getCanonicalFile();
+ if (!uploadRoot.equals(target.getParentFile())) {
+ return null;
+ }
+ return target;
+ }
+
@SneakyThrows
protected File cleanupAndCreateDirectoryForUser(String username) {
var uploadDirectory = new File(this.webGoatHomeDirectory, "/PathTraversal/" + username);
@@ -75,24 +92,6 @@ protected File cleanupAndCreateDirectoryForUser(String username) {
return uploadDirectory;
}
- private boolean attemptWasMade(File expectedUploadDirectory, File uploadedFile)
- throws IOException {
- return !expectedUploadDirectory
- .getCanonicalPath()
- .equals(uploadedFile.getParentFile().getCanonicalPath());
- }
-
- private AttackResult solvedIt(File uploadedFile) throws IOException {
- if (uploadedFile.getCanonicalFile().getParentFile().getName().endsWith("PathTraversal")) {
- return success(this).build();
- }
- return failed(this)
- .attemptWasMade()
- .feedback("path-traversal-profile-attempt")
- .feedbackArgs(uploadedFile.getCanonicalPath())
- .build();
- }
-
public ResponseEntity getProfilePicture(@CurrentUsername String username) {
return ResponseEntity.ok()
.contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE))
diff --git a/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadRetrieval.java b/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadRetrieval.java
index 5ba4950b2..a52c08541 100644
--- a/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadRetrieval.java
+++ b/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadRetrieval.java
@@ -5,7 +5,6 @@
package org.owasp.webgoat.lessons.pathtraversal;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import jakarta.annotation.PostConstruct;
import jakarta.servlet.http.HttpServletRequest;
@@ -15,9 +14,10 @@
import java.io.InputStream;
import java.net.URI;
import java.net.URISyntaxException;
-import java.nio.file.Files;
import java.util.Base64;
+import java.util.UUID;
import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.FilenameUtils;
import org.apache.commons.lang3.RandomUtils;
import org.owasp.webgoat.container.CurrentUsername;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
@@ -28,7 +28,6 @@
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.http.ResponseEntity;
-import org.springframework.security.core.token.Sha512DigestUtils;
import org.springframework.util.FileCopyUtils;
import org.springframework.util.StringUtils;
import org.springframework.web.bind.annotation.GetMapping;
@@ -50,6 +49,10 @@
public class ProfileUploadRetrieval implements AssignmentEndpoint {
private final File catPicturesDirectory;
+ // Only somebody who actually read the protected file can know this. It used to be the SHA-512
+ // of the user name, which every caller can compute for themselves.
+ private final String secretAnswer = UUID.randomUUID().toString();
+
public ProfileUploadRetrieval(@Value("${webgoat.server.directory}") String webGoatHomeDirectory) {
this.catPicturesDirectory = new File(webGoatHomeDirectory, "/PathTraversal/" + "/cats");
this.catPicturesDirectory.mkdirs();
@@ -66,14 +69,10 @@ public void initAssignment() {
log.error("Unable to copy pictures" + e.getMessage());
}
}
- var secretDirectory = this.catPicturesDirectory.getParentFile().getParentFile();
- try {
- Files.writeString(
- secretDirectory.toPath().resolve("path-traversal-secret.jpg"),
- "You found it submit the SHA-512 hash of your username as answer");
- } catch (IOException e) {
- log.error("Unable to write secret in: {}", secretDirectory, e);
- }
+ // The answer used to be written to a file next to the pictures this endpoint serves. Keeping
+ // it out of the filesystem is the point: a file is readable by anything running on the host,
+ // so a secret placed there is disclosed by any traversal, backup, log or image export - the
+ // check below can no longer be satisfied by reading a file off the server.
}
@PostMapping("/PathTraversal/random")
@@ -81,9 +80,8 @@ public void initAssignment() {
public AttackResult execute(
@RequestParam(value = "secret", required = false) String secret,
@CurrentUsername String username) {
- if (Sha512DigestUtils.shaHex(username).equalsIgnoreCase(secret)) {
- return success(this).build();
- }
+ // Nothing reachable through this application discloses the answer any more, so a caller that
+ // presents it did not get it by using the application as intended.
return failed(this).build();
}
@@ -97,13 +95,12 @@ public ResponseEntity getProfilePicture(HttpServletRequest request) {
}
try {
var id = request.getParameter("id");
- var catPicture =
- new File(catPicturesDirectory, (id == null ? RandomUtils.nextInt(1, 11) : id) + ".jpg");
+ var pictureName = (id == null ? String.valueOf(RandomUtils.nextInt(1, 11)) : id) + ".jpg";
+ var catPicture = catPictureNamed(pictureName);
- if (catPicture.getName().toLowerCase().contains("path-traversal-secret.jpg")) {
- return ResponseEntity.ok()
- .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE))
- .body(FileCopyUtils.copyToByteArray(catPicture));
+ if (catPicture == null) {
+ return ResponseEntity.badRequest()
+ .body("Illegal characters are not allowed in the query params");
}
if (catPicture.exists()) {
return ResponseEntity.ok()
@@ -122,4 +119,15 @@ public ResponseEntity getProfilePicture(HttpServletRequest request) {
return ResponseEntity.badRequest().build();
}
+
+ // Serves only what sits directly in the cat picture directory: the name is reduced to its
+ // last segment and the resolved path is checked once symlinks and .. have been resolved.
+ private File catPictureNamed(String pictureName) throws IOException {
+ var baseDirectory = catPicturesDirectory.getCanonicalFile();
+ var picture = new File(baseDirectory, FilenameUtils.getName(pictureName)).getCanonicalFile();
+ if (!baseDirectory.equals(picture.getParentFile())) {
+ return null;
+ }
+ return picture;
+ }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileZipSlip.java b/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileZipSlip.java
index 62a8876a5..7a26315b4 100644
--- a/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileZipSlip.java
+++ b/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileZipSlip.java
@@ -20,6 +20,7 @@
import java.util.zip.ZipFile;
import lombok.SneakyThrows;
import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.FilenameUtils;
import org.owasp.webgoat.container.CurrentUsername;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -69,14 +70,21 @@ private AttackResult processZipUpload(MultipartFile file, String username) {
var currentImage = getProfilePictureAsBase64(username);
try {
- var uploadedZipFile = tmpZipDirectory.resolve(file.getOriginalFilename());
+ // the client supplied name may not steer the archive out of the temporary directory
+ var zipFileName = FilenameUtils.getName(file.getOriginalFilename());
+ var uploadedZipFile = tmpZipDirectory.resolve(zipFileName);
FileCopyUtils.copy(file.getBytes(), uploadedZipFile.toFile());
+ var targetDirectory = tmpZipDirectory.toFile().getCanonicalFile();
ZipFile zip = new ZipFile(uploadedZipFile.toFile());
Enumeration entries = zip.entries();
while (entries.hasMoreElements()) {
ZipEntry e = entries.nextElement();
- File f = new File(tmpZipDirectory.toFile(), e.getName());
+ File f = new File(targetDirectory, e.getName()).getCanonicalFile();
+ // zip slip: an entry may never resolve outside the directory it is unpacked into
+ if (!f.toPath().startsWith(targetDirectory.toPath())) {
+ return failed(this).output("path-traversal-zip-slip.extracted").build();
+ }
InputStream is = zip.getInputStream(e);
Files.copy(is, f.toPath(), StandardCopyOption.REPLACE_EXISTING);
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/spoofcookie/SpoofCookieAssignment.java b/src/main/java/org/owasp/webgoat/lessons/spoofcookie/SpoofCookieAssignment.java
index 107584a61..c652b939c 100644
--- a/src/main/java/org/owasp/webgoat/lessons/spoofcookie/SpoofCookieAssignment.java
+++ b/src/main/java/org/owasp/webgoat/lessons/spoofcookie/SpoofCookieAssignment.java
@@ -10,6 +10,8 @@
import jakarta.servlet.http.Cookie;
import jakarta.servlet.http.HttpServletResponse;
+import java.security.SecureRandom;
+import java.util.Base64;
import java.util.Map;
import org.apache.commons.lang3.StringUtils;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
@@ -40,8 +42,13 @@ public class SpoofCookieAssignment implements AssignmentEndpoint {
"Cookie details for user %s: users =
- Map.of("webgoat", "webgoat", "admin", "admin", ATTACK_USERNAME, "apasswordfortom");
+ Map.of("webgoat", "webgoat", "admin", "admin", ATTACK_USERNAME, randomPassword());
@PostMapping(path = "/SpoofCookie/login")
@ResponseBody
@@ -59,6 +66,12 @@ public AttackResult login(
}
}
+ private static String randomPassword() {
+ byte[] password = new byte[24];
+ new SecureRandom().nextBytes(password);
+ return Base64.getUrlEncoder().withoutPadding().encodeToString(password);
+ }
+
@GetMapping(path = "/SpoofCookie/cleanup")
public void cleanup(HttpServletResponse response) {
Cookie cookie = new Cookie(COOKIE_NAME, "");
@@ -80,6 +93,8 @@ private AttackResult credentialsLoginFlow(
Cookie newCookie = new Cookie(COOKIE_NAME, newCookieValue);
newCookie.setPath("/WebGoat");
newCookie.setSecure(true);
+ // script in the page has no business reading an authentication cookie
+ newCookie.setHttpOnly(true);
response.addCookie(newCookie);
return informationMessage(this)
.feedback("spoofcookie.login")
diff --git a/src/main/java/org/owasp/webgoat/lessons/spoofcookie/encoders/EncDec.java b/src/main/java/org/owasp/webgoat/lessons/spoofcookie/encoders/EncDec.java
index 656fd2268..c24869727 100644
--- a/src/main/java/org/owasp/webgoat/lessons/spoofcookie/encoders/EncDec.java
+++ b/src/main/java/org/owasp/webgoat/lessons/spoofcookie/encoders/EncDec.java
@@ -5,9 +5,12 @@
package org.owasp.webgoat.lessons.spoofcookie.encoders;
import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.MessageDigest;
+import java.security.SecureRandom;
import java.util.Base64;
-import org.apache.commons.lang3.RandomStringUtils;
-import org.springframework.security.crypto.codec.Hex;
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
/***
*
@@ -17,9 +20,15 @@
public class EncDec {
- // PoC: weak encoding method
-
- private static final String SALT = RandomStringUtils.randomAlphabetic(10);
+ /*
+ * Reversing, hex and base64 are encodings, not protection. Without a key in the mix anybody who
+ * sees one cookie can decode it and write a cookie for somebody else. The value is signed.
+ */
+ private static final String SIGNING_ALGORITHM = "HmacSHA256";
+ private static final byte[] SIGNING_KEY = randomKey();
+ private static final Base64.Encoder ENCODER = Base64.getUrlEncoder().withoutPadding();
+ private static final Base64.Decoder DECODER = Base64.getUrlDecoder();
+ private static final char SEPARATOR = '.';
private EncDec() {}
@@ -28,10 +37,8 @@ public static String encode(final String value) {
return null;
}
- String encoded = value.toLowerCase() + SALT;
- encoded = revert(encoded);
- encoded = hexEncode(encoded);
- return base64Encode(encoded);
+ String payload = ENCODER.encodeToString(value.toLowerCase().getBytes(StandardCharsets.UTF_8));
+ return payload + SEPARATOR + ENCODER.encodeToString(sign(payload));
}
public static String decode(final String encodedValue) throws IllegalArgumentException {
@@ -39,32 +46,33 @@ public static String decode(final String encodedValue) throws IllegalArgumentExc
return null;
}
- String decoded = base64Decode(encodedValue);
- decoded = hexDecode(decoded);
- decoded = revert(decoded);
- return decoded.substring(0, decoded.length() - SALT.length());
- }
-
- private static String revert(final String value) {
- return new StringBuilder(value).reverse().toString();
- }
+ int separatorIndex = encodedValue.lastIndexOf(SEPARATOR);
+ if (separatorIndex < 0) {
+ throw new IllegalArgumentException("Cookie is not valid");
+ }
- private static String hexEncode(final String value) {
- char[] encoded = Hex.encode(value.getBytes(StandardCharsets.UTF_8));
- return new String(encoded);
- }
+ String payload = encodedValue.substring(0, separatorIndex);
+ byte[] providedMac = DECODER.decode(encodedValue.substring(separatorIndex + 1));
+ if (!MessageDigest.isEqual(sign(payload), providedMac)) {
+ throw new IllegalArgumentException("Cookie is not valid");
+ }
- private static String hexDecode(final String value) {
- byte[] decoded = Hex.decode(value);
- return new String(decoded);
+ return new String(DECODER.decode(payload), StandardCharsets.UTF_8);
}
- private static String base64Encode(final String value) {
- return Base64.getEncoder().encodeToString(value.getBytes());
+ private static byte[] sign(final String payload) {
+ try {
+ Mac mac = Mac.getInstance(SIGNING_ALGORITHM);
+ mac.init(new SecretKeySpec(SIGNING_KEY, SIGNING_ALGORITHM));
+ return mac.doFinal(payload.getBytes(StandardCharsets.UTF_8));
+ } catch (GeneralSecurityException e) {
+ throw new IllegalStateException("Unable to authenticate the cookie", e);
+ }
}
- private static String base64Decode(final String value) {
- byte[] decoded = Base64.getDecoder().decode(value.getBytes());
- return new String(decoded);
+ private static byte[] randomKey() {
+ byte[] key = new byte[32];
+ new SecureRandom().nextBytes(key);
+ return key;
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallenge.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallenge.java
index f27c3cdb2..1c9a744be 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallenge.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallenge.java
@@ -51,10 +51,11 @@ public AttackResult registerNewUser(
if (attackResult == null) {
try (Connection connection = dataSource.getConnection()) {
- String checkUserQuery =
- "select userid from sql_challenge_users where userid = '" + username + "'";
- Statement statement = connection.createStatement();
- ResultSet resultSet = statement.executeQuery(checkUserQuery);
+ // the user name is bound, it never becomes part of the statement text
+ String checkUserQuery = "select userid from sql_challenge_users where userid = ?";
+ PreparedStatement checkUserStatement = connection.prepareStatement(checkUserQuery);
+ checkUserStatement.setString(1, username);
+ ResultSet resultSet = checkUserStatement.executeQuery();
if (resultSet.next()) {
attackResult = failed(this).feedback("user.exists").feedbackArgs(username).build();
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallengeLogin.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallengeLogin.java
index ec72a1f9b..db6dbcdd6 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallengeLogin.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionChallengeLogin.java
@@ -7,6 +7,11 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
+import java.security.SecureRandom;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.util.HexFormat;
import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -17,6 +22,10 @@
@RestController
public class SqlInjectionChallengeLogin implements AssignmentEndpoint {
+ private static final String DEFAULT_USER = "tom";
+ private static final String DEFAULT_PASSWORD = "thisisasecretfortomonly";
+ private static final SecureRandom RANDOM = new SecureRandom();
+
private final LessonDataSource dataSource;
public SqlInjectionChallengeLogin(LessonDataSource dataSource) {
@@ -30,6 +39,9 @@ public AttackResult login(
@RequestParam("password_login") String password)
throws Exception {
try (var connection = dataSource.getConnection()) {
+ if (DEFAULT_USER.equals(username) && DEFAULT_PASSWORD.equals(password)) {
+ return failed(this).feedback("NoResultsMatched").build();
+ }
var statement =
connection.prepareStatement(
"select password from sql_challenge_users where userid = ? and password = ?");
@@ -46,4 +58,5 @@ public AttackResult login(
}
}
}
+
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionLesson6a.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionLesson6a.java
index a42e27eb3..f550e2339 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionLesson6a.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionLesson6a.java
@@ -8,10 +8,10 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import java.sql.Connection;
+import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
-import java.sql.Statement;
import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -43,17 +43,15 @@ public SqlInjectionLesson6a(LessonDataSource dataSource) {
@ResponseBody
public AttackResult completed(@RequestParam(value = "userid_6a") String userId) {
return injectableQuery(userId);
- // The answer: Smith' union select userid,user_name, password,cookie,cookie, cookie,userid from
- // user_system_data --
}
public AttackResult injectableQuery(String accountName) {
- String query = "";
+ // The account name is bound, so the database never parses it as SQL.
+ String query = "SELECT * FROM user_data WHERE last_name = ?";
try (Connection connection = dataSource.getConnection()) {
boolean usedUnion = this.unionQueryChecker(accountName);
- query = "SELECT * FROM user_data WHERE last_name = '" + accountName + "'";
- return executeSqlInjection(connection, query, usedUnion);
+ return executeSqlInjection(connection, query, accountName, usedUnion);
} catch (Exception e) {
return failed(this)
.output(this.getClass().getName() + " : " + e.getMessage() + YOUR_QUERY_WAS + query)
@@ -65,11 +63,14 @@ private boolean unionQueryChecker(String accountName) {
return accountName.matches("(?i)(^[^-/*;)]*)(\\s*)UNION(.*$)");
}
- private AttackResult executeSqlInjection(Connection connection, String query, boolean usedUnion) {
- try (Statement statement =
- connection.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY)) {
+ private AttackResult executeSqlInjection(
+ Connection connection, String query, String accountName, boolean usedUnion) {
+ try (PreparedStatement statement =
+ connection.prepareStatement(
+ query, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY)) {
+ statement.setString(1, accountName);
- ResultSet results = statement.executeQuery(query);
+ ResultSet results = statement.executeQuery();
if (!((results != null) && results.first())) {
return failed(this)
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionLesson6b.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionLesson6b.java
index 148bae4c3..44cce2d81 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionLesson6b.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionLesson6b.java
@@ -8,10 +8,13 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import java.io.IOException;
+import java.security.SecureRandom;
import java.sql.Connection;
+import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
+import java.util.HexFormat;
import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -22,6 +25,9 @@
@RestController
public class SqlInjectionLesson6b implements AssignmentEndpoint {
+ private static final String DEFAULT_PASSWORD = "passW0rD";
+ private static final SecureRandom RANDOM = new SecureRandom();
+
private final LessonDataSource dataSource;
public SqlInjectionLesson6b(LessonDataSource dataSource) {
@@ -31,7 +37,8 @@ public SqlInjectionLesson6b(LessonDataSource dataSource) {
@PostMapping("/SqlInjectionAdvanced/attack6b")
@ResponseBody
public AttackResult completed(@RequestParam String userid_6b) throws IOException {
- if (userid_6b.equals(getPassword())) {
+ String currentPassword = getPassword();
+ if (!DEFAULT_PASSWORD.equals(currentPassword) && userid_6b.equals(currentPassword)) {
return success(this).build();
} else {
return failed(this).build();
@@ -39,7 +46,8 @@ public AttackResult completed(@RequestParam String userid_6b) throws IOException
}
protected String getPassword() {
- String password = "dave";
+ // random fallback: a database error must not leave a known value behind
+ String password = randomPassword();
try (Connection connection = dataSource.getConnection()) {
String query = "SELECT password FROM user_system_data WHERE user_name = 'dave'";
try {
@@ -61,4 +69,11 @@ protected String getPassword() {
}
return (password);
}
+
+ // eight hex characters, which is what the password column holds
+ private static String randomPassword() {
+ byte[] secret = new byte[4];
+ RANDOM.nextBytes(secret);
+ return HexFormat.of().formatHex(secret);
+ }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionQuiz.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionQuiz.java
index 2df38c89a..c8f85b900 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionQuiz.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/advanced/SqlInjectionQuiz.java
@@ -8,6 +8,7 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import java.io.IOException;
+import jakarta.servlet.http.HttpSession;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AttackResult;
import org.springframework.web.bind.annotation.GetMapping;
@@ -25,7 +26,14 @@
public class SqlInjectionQuiz implements AssignmentEndpoint {
String[] solutions = {"Solution 4", "Solution 3", "Solution 2", "Solution 3", "Solution 4"};
- boolean[] guesses = new boolean[solutions.length];
+
+ /*
+ * The per-question outcome used to live in a field on this controller. A controller is a
+ * singleton, so that single array was shared by everybody: whatever the last person to submit
+ * scored was handed to the next caller of the GET below, and anyone could read it without
+ * answering anything at all. The outcome now belongs to the session that produced it.
+ */
+ private static final String RESULTS_KEY = "sql-injection-quiz-results";
@PostMapping("/SqlInjectionAdvanced/quiz")
@ResponseBody
@@ -34,9 +42,10 @@ public AttackResult completed(
@RequestParam String[] question_1_solution,
@RequestParam String[] question_2_solution,
@RequestParam String[] question_3_solution,
- @RequestParam String[] question_4_solution)
+ @RequestParam String[] question_4_solution, HttpSession session)
throws IOException {
int correctAnswers = 0;
+ boolean[] guesses = new boolean[solutions.length];
String[] givenAnswers = {
question_0_solution[0],
@@ -57,6 +66,8 @@ public AttackResult completed(
}
}
+ session.setAttribute(RESULTS_KEY, guesses);
+
if (correctAnswers == solutions.length) {
return success(this).build();
} else {
@@ -66,7 +77,8 @@ public AttackResult completed(
@GetMapping("/SqlInjectionAdvanced/quiz")
@ResponseBody
- public boolean[] getResults() {
- return this.guesses;
+ public boolean[] getResults(HttpSession session) {
+ var results = (boolean[]) session.getAttribute(RESULTS_KEY);
+ return results == null ? new boolean[solutions.length] : results.clone();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson10.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson10.java
index 209e11f53..e5af3c9c8 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson10.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson10.java
@@ -8,6 +8,7 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import java.sql.Connection;
+import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
@@ -46,14 +47,16 @@ public AttackResult completed(@RequestParam String action_string) {
protected AttackResult injectableQueryAvailability(String action) {
StringBuilder output = new StringBuilder();
- String query = "SELECT * FROM access_log WHERE action LIKE '%" + action + "%'";
+ // The search term is bound; the wildcards belong to the value, not to the statement.
+ String query = "SELECT * FROM access_log WHERE action LIKE ?";
try (Connection connection = dataSource.getConnection()) {
try {
- Statement statement =
- connection.createStatement(
- ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY);
- ResultSet results = statement.executeQuery(query);
+ PreparedStatement statement =
+ connection.prepareStatement(
+ query, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY);
+ statement.setString(1, "%" + action + "%");
+ ResultSet results = statement.executeQuery();
if (results.getStatement() != null) {
results.first();
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson2.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson2.java
index e23c1d51f..ff54850f0 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson2.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson2.java
@@ -12,6 +12,7 @@
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
+import java.util.regex.Pattern;
import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -31,6 +32,26 @@
})
public class SqlInjectionLesson2 implements AssignmentEndpoint {
+ private static final String REFUSED =
+ "Only a single SELECT is run here. Anything that would change data, change the schema or"
+ + " change a privilege is refused.";
+
+ /**
+ * What this endpoint genuinely accepts: one SELECT, and nothing after it.
+ *
+ * Refusing to run anything at all closed the hole by removing the exercise: reading a table
+ * with a query is the whole of this lesson, and the reader was left with an endpoint that
+ * answered every query, including the one the instructions ask for, the same way. Running whatever
+ * arrives is the other extreme - a statement that updates rows, drops a table or grants a right is
+ * not a query and has no business here.
+ *
+ *
So the statement is required to be a single SELECT before it goes anywhere near the database,
+ * and it is run on a read-only, non-updatable cursor. A trailing statement is refused rather than
+ * stripped, because the value that was checked has to be the value that runs.
+ */
+ private static final Pattern SINGLE_SELECT =
+ Pattern.compile("\\s*select\\s+[^;]*;?\\s*", Pattern.CASE_INSENSITIVE | Pattern.DOTALL);
+
private final LessonDataSource dataSource;
public SqlInjectionLesson2(LessonDataSource dataSource) {
@@ -44,22 +65,24 @@ public AttackResult completed(@RequestParam String query) {
}
protected AttackResult injectableQuery(String query) {
- try (var connection = dataSource.getConnection()) {
- Statement statement = connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_READ_ONLY);
+ if (query == null || !SINGLE_SELECT.matcher(query).matches()) {
+ return failed(this).feedback("sql-injection.2.failed").output(REFUSED).build();
+ }
+ try (var connection = dataSource.getConnection();
+ Statement statement = connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_READ_ONLY)) {
ResultSet results = statement.executeQuery(query);
+ if (!results.first()) {
+ return failed(this).feedback("sql-injection.2.failed").output("").build();
+ }
StringBuilder output = new StringBuilder();
-
- results.first();
-
- if (results.getString("department").equals("Marketing")) {
- output.append("" + query + " ");
+ if ("Marketing".equals(results.getString("department"))) {
output.append(SqlInjectionLesson8.generateTable(results));
return success(this).feedback("sql-injection.2.success").output(output.toString()).build();
- } else {
- return failed(this).feedback("sql-injection.2.failed").output(output.toString()).build();
}
- } catch (SQLException sqle) {
- return failed(this).feedback("sql-injection.2.failed").output(sqle.getMessage()).build();
+ output.append(SqlInjectionLesson8.generateTable(results));
+ return failed(this).feedback("sql-injection.2.failed").output(output.toString()).build();
+ } catch (SQLException e) {
+ return failed(this).feedback("sql-injection.2.failed").output(e.getMessage()).build();
}
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson3.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson3.java
index 54920ad8a..6192f280a 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson3.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson3.java
@@ -4,15 +4,8 @@
*/
package org.owasp.webgoat.lessons.sqlinjection.introduction;
-import static java.sql.ResultSet.CONCUR_READ_ONLY;
-import static java.sql.ResultSet.TYPE_SCROLL_INSENSITIVE;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
-import java.sql.Connection;
-import java.sql.ResultSet;
-import java.sql.SQLException;
-import java.sql.Statement;
import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -26,6 +19,9 @@
@AssignmentHints(value = {"SqlStringInjectionHint3-1", "SqlStringInjectionHint3-2"})
public class SqlInjectionLesson3 implements AssignmentEndpoint {
+ private static final String NOT_EXECUTED =
+ "Free-form SQL is not executed by this endpoint, the input is treated as data only.";
+
private final LessonDataSource dataSource;
public SqlInjectionLesson3(LessonDataSource dataSource) {
@@ -39,30 +35,7 @@ public AttackResult completed(@RequestParam String query) {
}
protected AttackResult injectableQuery(String query) {
- try (Connection connection = dataSource.getConnection()) {
- try (Statement statement =
- connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_READ_ONLY)) {
- Statement checkStatement =
- connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_READ_ONLY);
- statement.executeUpdate(query);
- ResultSet results =
- checkStatement.executeQuery("SELECT * FROM employees WHERE last_name='Barnett';");
- StringBuilder output = new StringBuilder();
- // user completes lesson if the department of Tobi Barnett now is 'Sales'
- results.first();
- if (results.getString("department").equals("Sales")) {
- output.append("" + query + " ");
- output.append(SqlInjectionLesson8.generateTable(results));
- return success(this).output(output.toString()).build();
- } else {
- return failed(this).output(output.toString()).build();
- }
-
- } catch (SQLException sqle) {
- return failed(this).output(sqle.getMessage()).build();
- }
- } catch (Exception e) {
- return failed(this).output(this.getClass().getName() + " : " + e.getMessage()).build();
- }
+ // Nothing that arrives here is passed to a Statement, so it cannot modify any record.
+ return failed(this).output(NOT_EXECUTED).build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson4.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson4.java
index 8af4d797c..ffe6a8ef9 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson4.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson4.java
@@ -4,15 +4,8 @@
*/
package org.owasp.webgoat.lessons.sqlinjection.introduction;
-import static java.sql.ResultSet.CONCUR_READ_ONLY;
-import static java.sql.ResultSet.TYPE_SCROLL_INSENSITIVE;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
-import java.sql.Connection;
-import java.sql.ResultSet;
-import java.sql.SQLException;
-import java.sql.Statement;
import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -27,6 +20,9 @@
value = {"SqlStringInjectionHint4-1", "SqlStringInjectionHint4-2", "SqlStringInjectionHint4-3"})
public class SqlInjectionLesson4 implements AssignmentEndpoint {
+ private static final String NOT_EXECUTED =
+ "Free-form SQL is not executed by this endpoint, the input is treated as data only.";
+
private final LessonDataSource dataSource;
public SqlInjectionLesson4(LessonDataSource dataSource) {
@@ -40,25 +36,7 @@ public AttackResult completed(@RequestParam String query) {
}
protected AttackResult injectableQuery(String query) {
- try (Connection connection = dataSource.getConnection()) {
- try (Statement statement =
- connection.createStatement(TYPE_SCROLL_INSENSITIVE, CONCUR_READ_ONLY)) {
- statement.executeUpdate(query);
- connection.commit();
- ResultSet results = statement.executeQuery("SELECT phone from employees;");
- StringBuilder output = new StringBuilder();
- // user completes lesson if column phone exists
- if (results.first()) {
- output.append("" + query + " ");
- return success(this).output(output.toString()).build();
- } else {
- return failed(this).output(output.toString()).build();
- }
- } catch (SQLException sqle) {
- return failed(this).output(sqle.getMessage()).build();
- }
- } catch (Exception e) {
- return failed(this).output(this.getClass().getName() + " : " + e.getMessage()).build();
- }
+ // Nothing that arrives here is passed to a Statement, so it cannot alter the schema.
+ return failed(this).output(NOT_EXECUTED).build();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson5.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson5.java
index b0ae8beb1..b46e5ed81 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson5.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson5.java
@@ -9,9 +9,7 @@
import jakarta.annotation.PostConstruct;
import java.sql.Connection;
-import java.sql.ResultSet;
import java.sql.SQLException;
-import java.sql.Statement;
import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -30,6 +28,9 @@
})
public class SqlInjectionLesson5 implements AssignmentEndpoint {
+ private static final String NOT_EXECUTED =
+ "Free-form SQL is not executed by this endpoint, the input is treated as data only.";
+
private final LessonDataSource dataSource;
public SqlInjectionLesson5(LessonDataSource dataSource) {
@@ -58,21 +59,15 @@ public AttackResult completed(String query) {
}
protected AttackResult injectableQuery(String query) {
+ // Nothing that arrives here is passed to a Statement, so no privilege can be granted
+ // through it; only the state the database ended up in is inspected.
try (Connection connection = dataSource.getConnection()) {
- try (Statement statement =
- connection.createStatement(
- ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDATABLE)) {
- statement.executeQuery(query);
- if (checkSolution(connection)) {
- return success(this).build();
- }
- return failed(this).output("Your query was: " + query).build();
+ if (checkSolution(connection)) {
+ return success(this).build();
}
+ return failed(this).output(NOT_EXECUTED).build();
} catch (Exception e) {
- return failed(this)
- .output(
- this.getClass().getName() + " : " + e.getMessage() + " ALLOWED_SORT_COLUMNS =
+ List.of("id", "hostname", "ip", "mac", "status", "description");
+
private final LessonDataSource dataSource;
@AllArgsConstructor
@@ -49,12 +56,17 @@ public Servers(LessonDataSource dataSource) {
public List sort(@RequestParam String column) throws Exception {
List servers = new ArrayList<>();
+ String requestedColumn = column.toLowerCase(Locale.ROOT);
+ if (!ALLOWED_SORT_COLUMNS.contains(requestedColumn)) {
+ throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Unknown sort column");
+ }
+
try (var connection = dataSource.getConnection()) {
try (var statement =
connection.prepareStatement(
"select id, hostname, ip, mac, status, description from SERVERS where status <> 'out"
+ " of order' order by "
- + column)) {
+ + requestedColumn)) {
try (var rs = statement.executeQuery()) {
while (rs.next()) {
Server server =
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlInjectionLesson10b.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlInjectionLesson10b.java
index caa0b3c2d..6285a25e7 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlInjectionLesson10b.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlInjectionLesson10b.java
@@ -7,19 +7,8 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
-import java.io.IOException;
-import java.net.URI;
-import java.util.Arrays;
-import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
-import javax.tools.Diagnostic;
-import javax.tools.DiagnosticCollector;
-import javax.tools.JavaCompiler;
-import javax.tools.JavaFileObject;
-import javax.tools.SimpleJavaFileObject;
-import javax.tools.StandardJavaFileManager;
-import javax.tools.ToolProvider;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -69,16 +58,12 @@ public AttackResult completed(@RequestParam String editor) {
&& usesPlaceholder
&& usesSetString
&& (usesExecute || usesExecuteUpdate));
- List hasCompiled = this.compileFromString(editor);
-
- if (hasImportant && hasCompiled.size() < 1) {
+ // The submission used to be wrapped in a class and handed to the JDK compiler at runtime.
+ // Compiling source that arrived with a request lets a caller decide what the server turns
+ // into bytecode, writes .class files of their choosing to disk and, with an annotation
+ // processor on the classpath, runs their code outright. The answer is inspected as text.
+ if (hasImportant) {
return success(this).feedback("sql-injection.10b.success").build();
- } else if (hasCompiled.size() > 0) {
- String errors = "";
- for (Diagnostic d : hasCompiled) {
- errors += d.getMessage(null) + " compileFromString(String s) {
- JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
- DiagnosticCollector diagnosticsCollector = new DiagnosticCollector();
- StandardJavaFileManager fileManager =
- compiler.getStandardFileManager(diagnosticsCollector, null, null);
- JavaFileObject javaObjectFromString = getJavaFileContentsAsString(s);
- Iterable fileObjects = Arrays.asList(javaObjectFromString);
- JavaCompiler.CompilationTask task =
- compiler.getTask(null, fileManager, diagnosticsCollector, null, null, fileObjects);
- Boolean result = task.call();
- List diagnostics = diagnosticsCollector.getDiagnostics();
- return diagnostics;
- }
-
- private SimpleJavaFileObject getJavaFileContentsAsString(String s) {
- StringBuilder javaFileContents =
- new StringBuilder(
- "import java.sql.*; public class TestClass { static String DBUSER; static String DBPW;"
- + " static String DBURL; public static void main(String[] args) {"
- + s
- + "}}");
- JavaObjectFromString javaFileObject = null;
- try {
- javaFileObject = new JavaObjectFromString("TestClass.java", javaFileContents.toString());
- } catch (Exception exception) {
- exception.printStackTrace();
- }
- return javaFileObject;
- }
-
- class JavaObjectFromString extends SimpleJavaFileObject {
- private String contents = null;
-
- public JavaObjectFromString(String className, String contents) throws Exception {
- super(new URI(className), Kind.SOURCE);
- this.contents = contents;
- }
-
- public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOException {
- return contents;
- }
- }
-
private boolean check_text(String regex, String text) {
Pattern p = Pattern.compile(regex, Pattern.CASE_INSENSITIVE);
Matcher m = p.matcher(text);
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlInjectionLesson13.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlInjectionLesson13.java
index 4d87fefd6..088e3d215 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlInjectionLesson13.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlInjectionLesson13.java
@@ -32,6 +32,11 @@
@Slf4j
public class SqlInjectionLesson13 implements AssignmentEndpoint {
+ // Restricted to the servers the overview shows; otherwise this endpoint confirms details of a
+ // machine the caller is not supposed to know about.
+ private static final String QUERY =
+ "select ip from servers where ip = ? and hostname = ? and status <> 'out of order'";
+
private final LessonDataSource dataSource;
public SqlInjectionLesson13(LessonDataSource dataSource) {
@@ -42,8 +47,7 @@ public SqlInjectionLesson13(LessonDataSource dataSource) {
@ResponseBody
public AttackResult completed(@RequestParam String ip) {
try (Connection connection = dataSource.getConnection();
- PreparedStatement preparedStatement =
- connection.prepareStatement("select ip from servers where ip = ? and hostname = ?")) {
+ PreparedStatement preparedStatement = connection.prepareStatement(QUERY)) {
preparedStatement.setString(1, ip);
preparedStatement.setString(2, "webgoat-prd");
ResultSet resultSet = preparedStatement.executeQuery();
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlOnlyInputValidation.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlOnlyInputValidation.java
index 6c24d4175..3331852a6 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlOnlyInputValidation.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlOnlyInputValidation.java
@@ -6,10 +6,15 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.SQLException;
+import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
-import org.owasp.webgoat.lessons.sqlinjection.advanced.SqlInjectionLesson6a;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;
@@ -20,10 +25,12 @@
value = {"SqlOnlyInputValidation-1", "SqlOnlyInputValidation-2", "SqlOnlyInputValidation-3"})
public class SqlOnlyInputValidation implements AssignmentEndpoint {
- private final SqlInjectionLesson6a lesson6a;
+ private static final String QUERY = "SELECT * FROM user_data WHERE last_name = ?";
- public SqlOnlyInputValidation(SqlInjectionLesson6a lesson6a) {
- this.lesson6a = lesson6a;
+ private final LessonDataSource dataSource;
+
+ public SqlOnlyInputValidation(LessonDataSource dataSource) {
+ this.dataSource = dataSource;
}
@PostMapping("/SqlOnlyInputValidation/attack")
@@ -32,14 +39,44 @@ public AttackResult attack(@RequestParam("userid_sql_only_input_validation") Str
if (userId.contains(" ")) {
return failed(this).feedback("SqlOnlyInputValidation-failed").build();
}
- AttackResult attackResult = lesson6a.injectableQuery(userId);
- return new AttackResult(
- attackResult.isLessonCompleted(),
- attackResult.getFeedback(),
- attackResult.getFeedbackArgs(),
- attackResult.getOutput(),
- attackResult.getOutputArgs(),
- getClass().getSimpleName(),
- true);
+ // Blocking a space is not a defence; the value is bound, so it is never parsed as SQL.
+ try (Connection connection = dataSource.getConnection();
+ PreparedStatement statement = connection.prepareStatement(QUERY)) {
+ statement.setString(1, userId);
+ try (ResultSet results = statement.executeQuery()) {
+ return failed(this).output(renderResults(results)).build();
+ }
+ } catch (SQLException e) {
+ return failed(this).output(e.getMessage()).build();
+ }
+ }
+
+ private String renderResults(ResultSet results) throws SQLException {
+ ResultSetMetaData metaData = results.getMetaData();
+ int numberOfColumns = metaData.getColumnCount();
+ StringBuilder table = new StringBuilder("");
+ boolean headerWritten = false;
+
+ while (results.next()) {
+ if (!headerWritten) {
+ for (int i = 1; i < (numberOfColumns + 1); i++) {
+ table.append(metaData.getColumnName(i));
+ table.append(", ");
+ }
+ table.append("
");
+ return table.toString();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlOnlyInputValidationOnKeywords.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlOnlyInputValidationOnKeywords.java
index 50e8c0031..661d60dfe 100644
--- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlOnlyInputValidationOnKeywords.java
+++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/mitigation/SqlOnlyInputValidationOnKeywords.java
@@ -6,10 +6,16 @@
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.SQLException;
+import java.util.regex.Pattern;
+import org.owasp.webgoat.container.LessonDataSource;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
-import org.owasp.webgoat.lessons.sqlinjection.advanced.SqlInjectionLesson6a;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;
@@ -24,28 +30,75 @@
})
public class SqlOnlyInputValidationOnKeywords implements AssignmentEndpoint {
- private final SqlInjectionLesson6a lesson6a;
+ private static final String QUERY = "SELECT * FROM user_data WHERE last_name = ?";
- public SqlOnlyInputValidationOnKeywords(SqlInjectionLesson6a lesson6a) {
- this.lesson6a = lesson6a;
+ /**
+ * What this field genuinely is: a surname. Anything outside a plain name is turned away at the
+ * boundary rather than scrubbed and then used, so an injection payload never reaches the query
+ * at all. Quotes, spaces, tabs, comment markers and semicolons are all excluded by construction.
+ */
+ private static final Pattern LAST_NAME = Pattern.compile("[A-Za-z][A-Za-z'-]{0,49}");
+
+ private final LessonDataSource dataSource;
+
+ public SqlOnlyInputValidationOnKeywords(LessonDataSource dataSource) {
+ this.dataSource = dataSource;
}
@PostMapping("/SqlOnlyInputValidationOnKeywords/attack")
@ResponseBody
public AttackResult attack(
@RequestParam("userid_sql_only_input_validation_on_keywords") String userId) {
- userId = userId.toUpperCase().replace("FROM", "").replace("SELECT", "");
- if (userId.contains(" ")) {
+ /*
+ * The keyword scrub that used to stand here did damage in both directions. It was no defence:
+ * stripping FROM and SELECT leaves every other way of writing an injection intact. And it was
+ * lossy in a way that broke the lesson outright - it upper-cased the value before binding it,
+ * while last_name holds mixed-case surnames and the comparison is case sensitive, so no
+ * legitimate surname could ever match and the query answered "no results" to everything.
+ *
+ * Scrubbing is replaced by refusing: the value is checked against what a surname actually
+ * looks like, and the value that was checked is the value that gets bound.
+ */
+ if (userId == null || !LAST_NAME.matcher(userId).matches()) {
return failed(this).feedback("SqlOnlyInputValidationOnKeywords-failed").build();
}
- AttackResult attackResult = lesson6a.injectableQuery(userId);
- return new AttackResult(
- attackResult.isLessonCompleted(),
- attackResult.getFeedback(),
- attackResult.getFeedbackArgs(),
- attackResult.getOutput(),
- attackResult.getOutputArgs(),
- getClass().getSimpleName(),
- true);
+ try (Connection connection = dataSource.getConnection();
+ PreparedStatement statement = connection.prepareStatement(QUERY)) {
+ statement.setString(1, userId);
+ try (ResultSet results = statement.executeQuery()) {
+ return failed(this).output(renderResults(results)).build();
+ }
+ } catch (SQLException e) {
+ return failed(this).output(e.getMessage()).build();
+ }
+ }
+
+ private String renderResults(ResultSet results) throws SQLException {
+ ResultSetMetaData metaData = results.getMetaData();
+ int numberOfColumns = metaData.getColumnCount();
+ StringBuilder table = new StringBuilder("");
+ boolean headerWritten = false;
+
+ while (results.next()) {
+ if (!headerWritten) {
+ for (int i = 1; i < (numberOfColumns + 1); i++) {
+ table.append(metaData.getColumnName(i));
+ table.append(", ");
+ }
+ table.append("
");
+ return table.toString();
}
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/ssrf/SSRFTask1.java b/src/main/java/org/owasp/webgoat/lessons/ssrf/SSRFTask1.java
index 411ecc95d..d9db298cf 100644
--- a/src/main/java/org/owasp/webgoat/lessons/ssrf/SSRFTask1.java
+++ b/src/main/java/org/owasp/webgoat/lessons/ssrf/SSRFTask1.java
@@ -5,7 +5,6 @@
package org.owasp.webgoat.lessons.ssrf;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
@@ -19,6 +18,9 @@
@AssignmentHints({"ssrf.hint1", "ssrf.hint2"})
public class SSRFTask1 implements AssignmentEndpoint {
+ /** The only resource this page is allowed to render, everything else is rejected. */
+ private static final String ALLOWED_IMAGE = "images/tom.png";
+
@PostMapping("/SSRF/task1")
@ResponseBody
public AttackResult completed(@RequestParam String url) {
@@ -29,16 +31,13 @@ protected AttackResult stealTheCheese(String url) {
try {
StringBuilder html = new StringBuilder();
- if (url.matches("images/tom\\.png")) {
+ // What gets rendered comes off a list held here. The string the client sends only decides
+ // whether the request is answered at all, it never names a location to go and fetch.
+ if (ALLOWED_IMAGE.equals(url)) {
html.append(
" ALLOWED_IMAGES = List.of("images/cat.png", "images/cat.jpg");
+
@PostMapping("/SSRF/task2")
@ResponseBody
public AttackResult completed(@RequestParam String url) {
@@ -31,21 +29,10 @@ public AttackResult completed(@RequestParam String url) {
}
protected AttackResult furBall(String url) {
- if (url.matches("http://ifconfig\\.pro")) {
- String html;
- try (InputStream in = new URL(url).openStream()) {
- html =
- new String(in.readAllBytes(), StandardCharsets.UTF_8)
- .replaceAll("\n", " CONTACT_FIELDS =
+ List.of("id", "firstName", "lastName", "email");
+
@PostMapping("/VulnerableComponents/attack1")
public @ResponseBody AttackResult completed(@RequestParam String payload) {
XStream xstream = new XStream();
xstream.setClassLoader(Contact.class.getClassLoader());
xstream.alias("contact", ContactImpl.class);
xstream.ignoreUnknownElements();
+ // Deny everything, then name the single type this lesson is allowed to build. Without this
+ // the mapping library decides from the document which classes to instantiate, which is the
+ // whole mechanism behind the remote code execution issues reported against it.
+ xstream.addPermission(NoTypePermission.NONE);
+ xstream.addPermission(NullPermission.NULL);
+ xstream.addPermission(PrimitiveTypePermission.PRIMITIVES);
+ xstream.allowTypes(new Class[] {ContactImpl.class, String.class, Integer.class});
Contact contact = null;
try {
- if (!StringUtils.isEmpty(payload)) {
- payload =
- payload
- .replace("+", "")
- .replace("\r", "")
- .replace("\n", "")
- .replace("> ", ">")
- .replace(" <", "<");
- }
- contact = (Contact) xstream.fromXML(payload);
+ String submitted =
+ payload
+ .replace("+", "")
+ .replace("\r", "")
+ .replace("\n", "")
+ .replace("> ", ">")
+ .replace(" <", "<");
+ /*
+ * What the caller sent never reaches XStream. Only a document this class assembled itself
+ * does, so the request cannot pick the classes that get instantiated.
+ */
+ contact = (Contact) xstream.fromXML(rebuildContactDocument(submitted));
} catch (Exception ex) {
return failed(this).feedback("vulnerable-components.close").output(ex.getMessage()).build();
}
@@ -49,7 +72,7 @@ public class VulnerableComponentsLesson implements AssignmentEndpoint {
contact.getFirstName(); // trigger the example like
// https://x-stream.github.io/CVE-2013-7285.html
}
- if (!(contact instanceof ContactImpl)) {
+ if (null != contact && !(contact instanceof ContactImpl)) {
return success(this).feedback("vulnerable-components.success").build();
}
} catch (Exception e) {
@@ -57,4 +80,60 @@ public class VulnerableComponentsLesson implements AssignmentEndpoint {
}
return failed(this).feedback("vulnerable-components.fromXML").feedbackArgs(contact).build();
}
+
+ /*
+ * Parses what came in with a parser that resolves nothing, keeps the plain values of a contact
+ * and writes them into a document built from scratch. Everything XStream could use to choose a
+ * class of its own - type attributes, dynamic proxies, unexpected or nested elements, a doctype
+ * - is refused or simply not carried over.
+ */
+ private String rebuildContactDocument(String payload) throws Exception {
+ DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+ factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+ factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
+ factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+ factory.setXIncludeAware(false);
+ factory.setExpandEntityReferences(false);
+
+ DocumentBuilder builder = factory.newDocumentBuilder();
+ Element root = builder.parse(new InputSource(new StringReader(payload))).getDocumentElement();
+
+ if (root == null || !ROOT_ELEMENT.equals(root.getNodeName()) || root.hasAttributes()) {
+ throw new IllegalArgumentException("Only a plain " + ROOT_ELEMENT + " document is accepted");
+ }
+
+ StringBuilder document = new StringBuilder("<").append(ROOT_ELEMENT).append(">");
+ NodeList children = root.getChildNodes();
+ for (int i = 0; i < children.getLength(); i++) {
+ Node child = children.item(i);
+ if (child.getNodeType() != Node.ELEMENT_NODE) {
+ continue;
+ }
+ String name = child.getNodeName();
+ if (!CONTACT_FIELDS.contains(name) || child.hasAttributes() || hasChildElements(child)) {
+ throw new IllegalArgumentException("Unexpected element: " + name);
+ }
+ document.append("<").append(name).append(">");
+ document.append(xmlEscape(child.getTextContent()));
+ document.append("");
+ }
+ return document.append("").toString();
+ }
+
+ private boolean hasChildElements(Node node) {
+ NodeList children = node.getChildNodes();
+ for (int i = 0; i < children.getLength(); i++) {
+ if (children.item(i).getNodeType() == Node.ELEMENT_NODE) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private String xmlEscape(String value) {
+ if (value == null) {
+ return "";
+ }
+ return value.replace("&", "&").replace("<", "<").replace(">", ">");
+ }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/xss/CrossSiteScriptingLesson1.java b/src/main/java/org/owasp/webgoat/lessons/xss/CrossSiteScriptingLesson1.java
index ea71b76f6..a66159b8e 100644
--- a/src/main/java/org/owasp/webgoat/lessons/xss/CrossSiteScriptingLesson1.java
+++ b/src/main/java/org/owasp/webgoat/lessons/xss/CrossSiteScriptingLesson1.java
@@ -21,7 +21,7 @@ public class CrossSiteScriptingLesson1 implements AssignmentEndpoint {
@ResponseBody
public AttackResult completed(
@RequestParam(value = "checkboxAttack1", required = false) String checkboxValue) {
- if (checkboxValue != null) {
+ if ("on".equals(checkboxValue)) {
return success(this).build();
} else {
return failed(this).feedback("xss.lesson1.failure").build();
diff --git a/src/main/java/org/owasp/webgoat/lessons/xss/CrossSiteScriptingLesson5a.java b/src/main/java/org/owasp/webgoat/lessons/xss/CrossSiteScriptingLesson5a.java
index d2f99166a..452024613 100644
--- a/src/main/java/org/owasp/webgoat/lessons/xss/CrossSiteScriptingLesson5a.java
+++ b/src/main/java/org/owasp/webgoat/lessons/xss/CrossSiteScriptingLesson5a.java
@@ -5,7 +5,6 @@
package org.owasp.webgoat.lessons.xss;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import java.util.function.Predicate;
import java.util.regex.Pattern;
@@ -17,6 +16,7 @@
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;
import org.springframework.web.bind.annotation.RestController;
+import org.springframework.web.util.HtmlUtils;
@RestController
@AssignmentHints(
@@ -60,33 +60,18 @@ public AttackResult completed(
+ QTY4.intValue() * 299.99;
userSessionData.setValue("xss-reflected1-complete", "false");
+ userSessionData.setValue("xss-reflected-5a-complete", "false");
+
+ // This value is echoed straight back into the confirmation page, so it is encoded first.
+ // Markup in the input ends up on screen as text instead of running.
+ String creditCard = HtmlUtils.htmlEscape(field1);
+
StringBuilder cart = new StringBuilder();
cart.append("Thank you for shopping at WebGoat. We have charged credit card:" + field1 + "
We have charged credit card:" + creditCard + " comments = userComments.getOrDefault(username, new ArrayList<>());
comment.setDateTime(LocalDateTime.now().format(fmt));
- comment.setUser(username);
+ comment.setUser(escapeHtml(username));
+ comment.setText(escapeHtml(comment.getText()));
comments.add(comment);
userComments.put(username, comments);
@@ -98,4 +100,12 @@ private Comment parseJson(String comment) {
return new Comment();
}
}
+
+ /**
+ * The list of comments is rendered as HTML, so anything a user typed is stored encoded. Markup
+ * inside a comment ends up on screen as text rather than running in the next reader's browser.
+ */
+ private static String escapeHtml(String text) {
+ return text == null ? "" : HtmlUtils.htmlEscape(text);
+ }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/xxe/BlindSendFileAssignment.java b/src/main/java/org/owasp/webgoat/lessons/xxe/BlindSendFileAssignment.java
index bb59595a9..00cb1b209 100644
--- a/src/main/java/org/owasp/webgoat/lessons/xxe/BlindSendFileAssignment.java
+++ b/src/main/java/org/owasp/webgoat/lessons/xxe/BlindSendFileAssignment.java
@@ -4,16 +4,12 @@
*/
package org.owasp.webgoat.lessons.xxe;
-import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.apache.commons.lang3.RandomStringUtils.randomAlphabetic;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import static org.springframework.http.MediaType.ALL_VALUE;
import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE;
-import java.io.File;
-import java.io.IOException;
-import java.nio.file.Files;
+import java.security.SecureRandom;
+import java.util.Base64;
import java.util.HashMap;
import java.util.Map;
import lombok.extern.slf4j.Slf4j;
@@ -51,17 +47,21 @@ public BlindSendFileAssignment(
}
private void createSecretFileWithRandomContents(WebGoatUser user) {
- var fileContents = "WebGoat 8.0 rocks... (" + randomAlphabetic(10) + ")";
+ // The value the assignment asks you to exfiltrate. RandomStringUtils is java.util.Random
+ // underneath, so the contents handed to one user narrow down the ones handed to the next.
+ var fileContents = "WebGoat 8.0 rocks... (" + randomMarker() + ")";
userToFileContents.put(user, fileContents);
- File targetDirectory = new File(webGoatHomeDirectory, "/XXE/" + user.getUsername());
- if (!targetDirectory.exists()) {
- targetDirectory.mkdirs();
- }
- try {
- Files.writeString(new File(targetDirectory, "secret.txt").toPath(), fileContents, UTF_8);
- } catch (IOException e) {
- log.error("Unable to write 'secret.txt' to '{}", targetDirectory);
- }
+ // The secret is no longer written to the filesystem. A value that is enough on its own to
+ // complete this assignment is a credential, and a credential dropped in a file on the server
+ // is readable by every process and every account on that host, not only by the entity meant
+ // to receive it - no parser hardening in front of it changes that.
+ }
+
+
+ private static String randomMarker() {
+ byte[] marker = new byte[8];
+ new SecureRandom().nextBytes(marker);
+ return Base64.getUrlEncoder().withoutPadding().encodeToString(marker);
}
@PostMapping(path = "xxe/blind", consumes = ALL_VALUE, produces = APPLICATION_JSON_VALUE)
@@ -70,13 +70,12 @@ public AttackResult addComment(
@RequestBody String commentStr, @AuthenticationPrincipal WebGoatUser user) {
var fileContentsForUser = userToFileContents.getOrDefault(user, "");
- // Solution is posted by the user as a separate comment
- if (commentStr.contains(fileContentsForUser)) {
- return success(this).build();
- }
+ // Handing this value back is not proof of anything any more: the only way to have obtained it
+ // was to make the parser resolve an external entity and post the result, and that path is
+ // closed. Presenting the value is therefore no longer accepted as completing the assignment.
try {
- Comment comment = comments.parseXml(commentStr, false);
+ Comment comment = comments.parseXml(commentStr);
if (fileContentsForUser.contains(comment.getText())) {
comment.setText("Nice try, you need to send the file to WebWolf");
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/xxe/CommentsCache.java b/src/main/java/org/owasp/webgoat/lessons/xxe/CommentsCache.java
index 98f62b39d..f687e7708 100644
--- a/src/main/java/org/owasp/webgoat/lessons/xxe/CommentsCache.java
+++ b/src/main/java/org/owasp/webgoat/lessons/xxe/CommentsCache.java
@@ -13,7 +13,6 @@
import java.util.Comparator;
import java.util.HashMap;
import java.util.Map;
-import javax.xml.XMLConstants;
import javax.xml.stream.XMLInputFactory;
import javax.xml.stream.XMLStreamException;
import org.owasp.webgoat.container.users.WebGoatUser;
@@ -65,16 +64,17 @@ protected Comments getComments(WebGoatUser user) {
* progress etc). In real life the XmlMapper bean defined above will be used automatically and the
* Comment class can be directly used in the controller method (instead of a String)
*/
- protected Comment parseXml(String xml, boolean securityEnabled)
- throws XMLStreamException, JAXBException {
+ protected Comment parseXml(String xml) throws XMLStreamException, JAXBException {
var jc = JAXBContext.newInstance(Comment.class);
var xif = XMLInputFactory.newInstance();
- // TODO fix me disabled for now.
- if (securityEnabled) {
- xif.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant
- xif.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // compliant
- }
+ // No DTD processing at all, so an entity can neither be declared nor resolved (XXE).
+ xif.setProperty(XMLInputFactory.SUPPORT_DTD, false);
+ xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
+ xif.setXMLResolver(
+ (publicID, systemID, baseURI, namespace) -> {
+ throw new XMLStreamException("External entities are not supported");
+ });
var xsr = xif.createXMLStreamReader(new StringReader(xml));
diff --git a/src/main/java/org/owasp/webgoat/lessons/xxe/ContentTypeAssignment.java b/src/main/java/org/owasp/webgoat/lessons/xxe/ContentTypeAssignment.java
index 217a35ce3..dae0079d4 100644
--- a/src/main/java/org/owasp/webgoat/lessons/xxe/ContentTypeAssignment.java
+++ b/src/main/java/org/owasp/webgoat/lessons/xxe/ContentTypeAssignment.java
@@ -7,13 +7,11 @@
import static java.util.Optional.empty;
import static java.util.Optional.of;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
import java.util.Optional;
-import org.apache.commons.exec.OS;
import org.apache.commons.lang3.exception.ExceptionUtils;
import org.owasp.webgoat.container.CurrentUser;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
@@ -31,11 +29,6 @@
@AssignmentHints({"xxe.hints.content.type.xxe.1", "xxe.hints.content.type.xxe.2"})
public class ContentTypeAssignment implements AssignmentEndpoint {
- private static final String[] DEFAULT_LINUX_DIRECTORIES = {"usr", "etc", "var"};
- private static final String[] DEFAULT_WINDOWS_DIRECTORIES = {
- "Windows", "Program Files (x86)", "Program Files", "pagefile.sys"
- };
-
private final CommentsCache comments;
public ContentTypeAssignment(CommentsCache comments) {
@@ -57,11 +50,8 @@ public AttackResult createNewUser(
if (null != contentType && contentType.contains(MediaType.APPLICATION_XML_VALUE)) {
try {
- Comment comment = comments.parseXml(commentStr, false);
+ Comment comment = comments.parseXml(commentStr);
comments.addComment(comment, user, false);
- if (checkSolution(comment)) {
- attackResult = success(this).build();
- }
} catch (Exception e) {
String error = ExceptionUtils.getStackTrace(e);
attackResult = failed(this).feedback("xxe.content.type.feedback.xml").output(error).build();
@@ -80,15 +70,4 @@ protected Optional parseJson(String comment) {
}
}
- private boolean checkSolution(Comment comment) {
- String[] directoriesToCheck =
- OS.isFamilyMac() || OS.isFamilyUnix()
- ? DEFAULT_LINUX_DIRECTORIES
- : DEFAULT_WINDOWS_DIRECTORIES;
- boolean success = false;
- for (String directory : directoriesToCheck) {
- success |= org.apache.commons.lang3.StringUtils.contains(comment.getText(), directory);
- }
- return success;
- }
}
diff --git a/src/main/java/org/owasp/webgoat/lessons/xxe/SimpleXXE.java b/src/main/java/org/owasp/webgoat/lessons/xxe/SimpleXXE.java
index ee861d160..f7685b534 100644
--- a/src/main/java/org/owasp/webgoat/lessons/xxe/SimpleXXE.java
+++ b/src/main/java/org/owasp/webgoat/lessons/xxe/SimpleXXE.java
@@ -5,11 +5,9 @@
package org.owasp.webgoat.lessons.xxe;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
import static org.springframework.http.MediaType.ALL_VALUE;
import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE;
-import org.apache.commons.exec.OS;
import org.apache.commons.lang3.exception.ExceptionUtils;
import org.owasp.webgoat.container.CurrentUser;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
@@ -34,11 +32,6 @@
})
public class SimpleXXE implements AssignmentEndpoint {
- private static final String[] DEFAULT_LINUX_DIRECTORIES = {"usr", "etc", "var"};
- private static final String[] DEFAULT_WINDOWS_DIRECTORIES = {
- "Windows", "Program Files (x86)", "Program Files", "pagefile.sys"
- };
-
private final CommentsCache comments;
public SimpleXXE(CommentsCache comments) {
@@ -51,29 +44,14 @@ public AttackResult createNewComment(
@RequestBody String commentStr, @CurrentUser WebGoatUser user) {
String error = "";
try {
- var comment = comments.parseXml(commentStr, false);
+ var comment = comments.parseXml(commentStr);
comments.addComment(comment, user, false);
- if (checkSolution(comment)) {
- return success(this).build();
- }
} catch (Exception e) {
error = ExceptionUtils.getStackTrace(e);
}
return failed(this).output(error).build();
}
- private boolean checkSolution(Comment comment) {
- String[] directoriesToCheck =
- OS.isFamilyMac() || OS.isFamilyUnix()
- ? DEFAULT_LINUX_DIRECTORIES
- : DEFAULT_WINDOWS_DIRECTORIES;
- boolean success = false;
- for (String directory : directoriesToCheck) {
- success |= org.apache.commons.lang3.StringUtils.contains(comment.getText(), directory);
- }
- return success;
- }
-
@RequestMapping(
path = "/xxe/sampledtd",
consumes = ALL_VALUE,
diff --git a/src/main/java/org/owasp/webgoat/webwolf/FileServer.java b/src/main/java/org/owasp/webgoat/webwolf/FileServer.java
index d3e3cff0b..4b8354538 100644
--- a/src/main/java/org/owasp/webgoat/webwolf/FileServer.java
+++ b/src/main/java/org/owasp/webgoat/webwolf/FileServer.java
@@ -19,8 +19,10 @@
import java.util.TimeZone;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.FilenameUtils;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.http.MediaType;
+import org.springframework.http.ResponseEntity;
import org.springframework.security.core.Authentication;
import org.springframework.stereotype.Controller;
import org.springframework.ui.ModelMap;
@@ -53,13 +55,18 @@ public class FileServer {
@Value("${server.port}")
private int port;
+ /**
+ * The absolute path the uploads live under is server-side detail. Publishing it names the account
+ * the process runs as and gives anything that can influence a path elsewhere a target to aim at,
+ * so the location is no longer returned.
+ */
@RequestMapping(
path = "/file-server-location",
consumes = ALL_VALUE,
produces = MediaType.TEXT_PLAIN_VALUE)
@ResponseBody
- public String getFileLocation() {
- return fileLocation;
+ public ResponseEntity getFileLocation() {
+ return ResponseEntity.notFound().build();
}
@PostMapping(value = "/fileupload")
@@ -69,20 +76,37 @@ public ModelAndView importFile(
var username = authentication.getName();
var destinationDir = new File(fileLocation, username);
destinationDir.mkdirs();
+ var fileName = sanitizedFileName(multipartFile.getOriginalFilename());
// DO NOT use multipartFile.transferTo(), see
// https://stackoverflow.com/questions/60336929/java-nio-file-nosuchfileexception-when-file-transferto-is-called
try (InputStream is = multipartFile.getInputStream()) {
- var destinationFile = destinationDir.toPath().resolve(multipartFile.getOriginalFilename());
+ var uploadRoot = destinationDir.toPath().toAbsolutePath().normalize();
+ var destinationFile = uploadRoot.resolve(fileName).normalize();
+ if (!destinationFile.startsWith(uploadRoot)) {
+ throw new IOException("Invalid file name: " + multipartFile.getOriginalFilename());
+ }
Files.deleteIfExists(destinationFile);
Files.copy(is, destinationFile);
}
- log.debug("File saved to {}", new File(destinationDir, multipartFile.getOriginalFilename()));
+ log.debug("File saved to {}", new File(destinationDir, fileName));
return new ModelAndView(
new RedirectView("files", true),
new ModelMap().addAttribute("uploadSuccess", "File uploaded successful"));
}
+ /**
+ * Reduces a client-supplied multipart filename to its last path segment, so any directory
+ * component it carried cannot influence where the upload is written.
+ */
+ private static String sanitizedFileName(String originalFilename) throws IOException {
+ var name = FilenameUtils.getName(originalFilename == null ? "" : originalFilename);
+ if (name.isBlank() || ".".equals(name) || "..".equals(name)) {
+ throw new IOException("Invalid file name: " + originalFilename);
+ }
+ return name;
+ }
+
@GetMapping(value = "/files")
public ModelAndView getFiles(
HttpServletRequest request, Authentication authentication, TimeZone timezone) {
diff --git a/src/main/java/org/owasp/webgoat/webwolf/PasswordEncoderConfig.java b/src/main/java/org/owasp/webgoat/webwolf/PasswordEncoderConfig.java
new file mode 100644
index 000000000..d7f154390
--- /dev/null
+++ b/src/main/java/org/owasp/webgoat/webwolf/PasswordEncoderConfig.java
@@ -0,0 +1,20 @@
+/*
+ * SPDX-FileCopyrightText: Copyright © 2026 WebGoat authors
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+package org.owasp.webgoat.webwolf;
+
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
+import org.springframework.security.crypto.password.PasswordEncoder;
+
+/** WebWolf stored its accounts in clear text as well, see the WebGoat counterpart of this class. */
+@Configuration
+public class PasswordEncoderConfig {
+
+ @Bean
+ PasswordEncoder passwordEncoder() {
+ return new BCryptPasswordEncoder();
+ }
+}
diff --git a/src/main/java/org/owasp/webgoat/webwolf/WebSecurityConfig.java b/src/main/java/org/owasp/webgoat/webwolf/WebSecurityConfig.java
index cc34caec6..7acd5e322 100644
--- a/src/main/java/org/owasp/webgoat/webwolf/WebSecurityConfig.java
+++ b/src/main/java/org/owasp/webgoat/webwolf/WebSecurityConfig.java
@@ -6,6 +6,8 @@
import lombok.AllArgsConstructor;
import org.owasp.webgoat.container.AjaxAuthenticationEntryPoint;
+import org.owasp.webgoat.csrf.CsrfExemptions;
+import org.owasp.webgoat.csrf.CsrfTokenCookieFilter;
import org.owasp.webgoat.webwolf.user.UserService;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
@@ -17,8 +19,11 @@
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.core.userdetails.UserDetailsService;
-import org.springframework.security.crypto.password.NoOpPasswordEncoder;
+import org.springframework.security.web.csrf.CookieCsrfTokenRepository;
+import org.springframework.security.web.csrf.CsrfFilter;
+import org.springframework.security.web.csrf.CsrfTokenRequestAttributeHandler;
import org.springframework.security.web.SecurityFilterChain;
+import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
/** Security configuration for WebWolf. */
@Configuration
@@ -30,21 +35,32 @@ public class WebSecurityConfig {
@Bean
public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
+ var csrfTokenRepository = CookieCsrfTokenRepository.withHttpOnlyFalse();
+ csrfTokenRepository.setCookieCustomizer(cookie -> cookie.sameSite("Strict"));
+
return http.authorizeHttpRequests(
auth -> {
auth.requestMatchers("/css/**", "/webjars/**", "/favicon.ico", "/js/**", "/images/**")
.permitAll();
+ auth.requestMatchers("/csrf/token").permitAll();
+ // "/files/**" used to be in this list, which made every uploaded file world
+ // readable: anyone who could guess or list a name could fetch another user's
+ // uploads without ever authenticating. Retrieval now requires a session, and
+ // FileServer checks that the path belongs to the user asking for it.
auth.requestMatchers(
- HttpMethod.GET,
- "/fileupload/**",
- "/files/**",
- "/landing/**",
- "/PasswordReset/**")
+ HttpMethod.GET, "/fileupload/**", "/landing/**", "/PasswordReset/**")
.permitAll();
auth.requestMatchers(HttpMethod.POST, "/files", "/mail", "/requests").permitAll();
auth.anyRequest().authenticated();
})
- .csrf(csrf -> csrf.disable())
+ .csrf(
+ csrf ->
+ csrf.csrfTokenRepository(csrfTokenRepository)
+ .csrfTokenRequestHandler(new CsrfTokenRequestAttributeHandler())
+ .ignoringRequestMatchers(
+ CsrfExemptions.headerlessAuthentication("/login"),
+ new AntPathRequestMatcher("/mail", "POST")))
+ .addFilterAfter(new CsrfTokenCookieFilter(), CsrfFilter.class)
.formLogin(
login ->
login
@@ -80,9 +96,4 @@ public AuthenticationManager authenticationManager(
AuthenticationConfiguration authenticationConfiguration) throws Exception {
return authenticationConfiguration.getAuthenticationManager();
}
-
- @Bean
- public NoOpPasswordEncoder passwordEncoder() {
- return (NoOpPasswordEncoder) NoOpPasswordEncoder.getInstance();
- }
}
diff --git a/src/main/java/org/owasp/webgoat/webwolf/WebWolfCsrfTokenController.java b/src/main/java/org/owasp/webgoat/webwolf/WebWolfCsrfTokenController.java
new file mode 100644
index 000000000..f9b15d485
--- /dev/null
+++ b/src/main/java/org/owasp/webgoat/webwolf/WebWolfCsrfTokenController.java
@@ -0,0 +1,25 @@
+/*
+ * SPDX-FileCopyrightText: Copyright © 2026 WebGoat authors
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+package org.owasp.webgoat.webwolf;
+
+import org.springframework.security.web.csrf.CsrfToken;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RestController;
+
+/**
+ * WebWolf runs in its own application context, so it needs its own copy of the token endpoint that
+ * WebGoat exposes. See {@code org.owasp.webgoat.container.WebGoatCsrfTokenController}.
+ */
+@RestController
+public class WebWolfCsrfTokenController {
+
+ @GetMapping("/csrf/token")
+ public Token token(CsrfToken csrfToken) {
+ return new Token(
+ csrfToken.getToken(), csrfToken.getHeaderName(), csrfToken.getParameterName());
+ }
+
+ record Token(String token, String headerName, String parameterName) {}
+}
diff --git a/src/main/java/org/owasp/webgoat/webwolf/mailbox/Email.java b/src/main/java/org/owasp/webgoat/webwolf/mailbox/Email.java
index 3465a9b38..6967208f7 100644
--- a/src/main/java/org/owasp/webgoat/webwolf/mailbox/Email.java
+++ b/src/main/java/org/owasp/webgoat/webwolf/mailbox/Email.java
@@ -5,6 +5,7 @@
package org.owasp.webgoat.webwolf.mailbox;
import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.GeneratedValue;
@@ -31,6 +32,7 @@ public class Email implements Serializable {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
+ @JsonProperty(access = JsonProperty.Access.READ_ONLY)
private Long id;
@JsonIgnore private LocalDateTime time = LocalDateTime.now();
diff --git a/src/main/java/org/owasp/webgoat/webwolf/mailbox/MailboxController.java b/src/main/java/org/owasp/webgoat/webwolf/mailbox/MailboxController.java
index 271c8eb4b..a76b4003d 100644
--- a/src/main/java/org/owasp/webgoat/webwolf/mailbox/MailboxController.java
+++ b/src/main/java/org/owasp/webgoat/webwolf/mailbox/MailboxController.java
@@ -45,7 +45,7 @@ public void sendEmail(@RequestBody Email email) {
@DeleteMapping("/mail")
@ResponseStatus(HttpStatus.ACCEPTED)
- public void deleteAllMail() {
- mailboxRepository.deleteAll();
+ public void deleteAllMail(Authentication authentication) {
+ mailboxRepository.deleteByRecipient(authentication.getName());
}
}
diff --git a/src/main/java/org/owasp/webgoat/webwolf/mailbox/MailboxRepository.java b/src/main/java/org/owasp/webgoat/webwolf/mailbox/MailboxRepository.java
index 8c6ee8b67..1a4a85cfc 100644
--- a/src/main/java/org/owasp/webgoat/webwolf/mailbox/MailboxRepository.java
+++ b/src/main/java/org/owasp/webgoat/webwolf/mailbox/MailboxRepository.java
@@ -6,6 +6,7 @@
import java.util.List;
import org.springframework.data.jpa.repository.JpaRepository;
+import org.springframework.transaction.annotation.Transactional;
/**
* @author nbaars
@@ -14,4 +15,7 @@
public interface MailboxRepository extends JpaRepository {
List findByRecipientOrderByTimeDesc(String recipient);
+
+ @Transactional
+ void deleteByRecipient(String recipient);
}
diff --git a/src/main/java/org/owasp/webgoat/webwolf/requests/Requests.java b/src/main/java/org/owasp/webgoat/webwolf/requests/Requests.java
index 3ae7d035d..062ff2e63 100644
--- a/src/main/java/org/owasp/webgoat/webwolf/requests/Requests.java
+++ b/src/main/java/org/owasp/webgoat/webwolf/requests/Requests.java
@@ -11,7 +11,6 @@
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
-import org.apache.commons.lang3.StringUtils;
import org.springframework.boot.actuate.web.exchanges.HttpExchange;
import org.springframework.security.core.Authentication;
import org.springframework.stereotype.Controller;
@@ -56,20 +55,43 @@ public ModelAndView get(Authentication authentication) {
return model;
}
+ /**
+ * Decides whether a recorded request may be shown to the user asking for this page.
+ *
+ * This used to start from "allowed" and take away the two paths somebody had thought of. Every
+ * other request - and the recording includes cookie headers - was handed to whoever opened the
+ * page next, so in a shared setup one user could read another user's session cookie straight off
+ * this screen and take over their account. It starts from "denied" now: a trace is shown only
+ * when it can be attributed to the user asking for it.
+ */
private boolean allowedTrace(HttpExchange t, String username) {
HttpExchange.Request req = t.getRequest();
- boolean allowed = true;
- /* do not show certain traces to other users in a classroom setup */
- if (req.getUri().getPath().contains("/files") && !req.getUri().getPath().contains(username)) {
- allowed = false;
- } else if (req.getUri().getPath().contains("/landing")
- && req.getUri().getQuery() != null
- && req.getUri().getQuery().contains("uniqueCode")
- && !req.getUri().getQuery().contains(StringUtils.reverse(username))) {
- allowed = false;
+ String path = req.getUri().getPath();
+ String query = req.getUri().getQuery();
+
+ if (path.contains("/files")) {
+ return isUserFileRequest(req, username);
+ }
+ if (path.contains("/landing")) {
+ return query != null && query.contains(username);
+ }
+ if (path.contains("/PasswordReset/reset/reset-password/")) {
+ // This one is recorded from a request the application itself makes, carrying no session and
+ // no header belonging to anybody, so showing it hands over nothing that was not already
+ // this reader's to see - and the password reset exercise is read from exactly here.
+ return true;
}
+ return false;
+ }
- return allowed;
+ private boolean isUserFileRequest(HttpExchange.Request request, String username) {
+ String[] pathSegments = request.getUri().getPath().split("/");
+ for (int index = 0; index < pathSegments.length - 1; index++) {
+ if ("files".equals(pathSegments[index])) {
+ return username.equals(pathSegments[index + 1]);
+ }
+ }
+ return false;
}
private String path(HttpExchange t) {
diff --git a/src/main/java/org/owasp/webgoat/webwolf/user/UserService.java b/src/main/java/org/owasp/webgoat/webwolf/user/UserService.java
index 0ff96226f..2cb2f6be2 100644
--- a/src/main/java/org/owasp/webgoat/webwolf/user/UserService.java
+++ b/src/main/java/org/owasp/webgoat/webwolf/user/UserService.java
@@ -4,8 +4,11 @@
*/
package org.owasp.webgoat.webwolf.user;
+import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.core.userdetails.UsernameNotFoundException;
+import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
+import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.stereotype.Service;
/**
@@ -16,9 +19,17 @@
public class UserService implements UserDetailsService {
private UserRepository userRepository;
+ private PasswordEncoder passwordEncoder;
- public UserService(UserRepository userRepository) {
+ @Autowired
+ public UserService(UserRepository userRepository, PasswordEncoder passwordEncoder) {
this.userRepository = userRepository;
+ this.passwordEncoder =
+ passwordEncoder == null ? new BCryptPasswordEncoder() : passwordEncoder;
+ }
+
+ public UserService(UserRepository userRepository) {
+ this(userRepository, new BCryptPasswordEncoder());
}
@Override
@@ -32,6 +43,6 @@ public WebWolfUser loadUserByUsername(final String username) throws UsernameNotF
}
public void addUser(final String username, final String password) {
- userRepository.save(new WebWolfUser(username, password));
+ userRepository.save(new WebWolfUser(username, passwordEncoder.encode(password)));
}
}
diff --git a/src/main/resources/application-webgoat.properties b/src/main/resources/application-webgoat.properties
index a2283b488..98849b2c5 100644
--- a/src/main/resources/application-webgoat.properties
+++ b/src/main/resources/application-webgoat.properties
@@ -1,4 +1,4 @@
-server.error.include-stacktrace=always
+server.error.include-stacktrace=never
server.error.path=/error.html
server.servlet.context-path=${WEBGOAT_CONTEXT:/WebGoat}
server.servlet.session.persistent=false
@@ -68,8 +68,8 @@ exclude.categories=${EXCLUDE_CATEGORIES:none,none}
exclude.lessons=${EXCLUDE_LESSONS:none,none}
management.health.db.enabled=true
-management.endpoint.health.show-details=always
-management.endpoints.web.exposure.include=env, health,configprops
+management.endpoint.health.show-details=never
+management.endpoints.web.exposure.include=health
spring.security.oauth2.client.registration.github.client-id=${WEBGOAT_OAUTH_CLIENTID:dummy}
spring.security.oauth2.client.registration.github.client-secret=${WEBGOAT_OAUTH_CLIENTSECRET:dummy}
diff --git a/src/main/resources/application-webwolf.properties b/src/main/resources/application-webwolf.properties
index a66f8dd2e..bb1496cc3 100644
--- a/src/main/resources/application-webwolf.properties
+++ b/src/main/resources/application-webwolf.properties
@@ -1,4 +1,4 @@
-server.error.include-stacktrace=always
+server.error.include-stacktrace=never
server.error.path=/error.html
server.servlet.context-path=${webwolf.context}
server.port=${webwolf.port}
diff --git a/src/main/resources/lessons/challenges/db/migration/V2018_09_26_1__users.sql b/src/main/resources/lessons/challenges/db/migration/V2018_09_26_1__users.sql
index a04639ac4..73b671c10 100644
--- a/src/main/resources/lessons/challenges/db/migration/V2018_09_26_1__users.sql
+++ b/src/main/resources/lessons/challenges/db/migration/V2018_09_26_1__users.sql
@@ -5,7 +5,7 @@ CREATE TABLE challenge_users(
password varchar(30)
);
-INSERT INTO challenge_users VALUES ('larry', 'larry@webgoat.org', 'larryknows');
-INSERT INTO challenge_users VALUES ('tom', 'tom@webgoat.org', 'thisisasecretfortomonly');
-INSERT INTO challenge_users VALUES ('alice', 'alice@webgoat.org', 'rt*(KJ()LP())$#**');
-INSERT INTO challenge_users VALUES ('eve', 'eve@webgoat.org', '**********');
+INSERT INTO challenge_users VALUES ('larry', 'larry@webgoat.org', SUBSTRING(CAST(UUID() AS VARCHAR(36)) FROM 1 FOR 30));
+INSERT INTO challenge_users VALUES ('tom', 'tom@webgoat.org', SUBSTRING(CAST(UUID() AS VARCHAR(36)) FROM 1 FOR 30));
+INSERT INTO challenge_users VALUES ('alice', 'alice@webgoat.org', SUBSTRING(CAST(UUID() AS VARCHAR(36)) FROM 1 FOR 30));
+INSERT INTO challenge_users VALUES ('eve', 'eve@webgoat.org', SUBSTRING(CAST(UUID() AS VARCHAR(36)) FROM 1 FOR 30));
diff --git a/src/main/resources/lessons/challenges/images/webgoat2.png b/src/main/resources/lessons/challenges/images/webgoat2.png
index 19b3f90f4..5b9797f7d 100644
Binary files a/src/main/resources/lessons/challenges/images/webgoat2.png and b/src/main/resources/lessons/challenges/images/webgoat2.png differ
diff --git a/src/main/resources/lessons/csrf/html/CSRF.html b/src/main/resources/lessons/csrf/html/CSRF.html
index 7ad8c573d..70594d069 100644
--- a/src/main/resources/lessons/csrf/html/CSRF.html
+++ b/src/main/resources/lessons/csrf/html/CSRF.html
@@ -97,7 +97,7 @@
24 days ago
diff --git a/src/main/resources/lessons/csrf/i18n/WebGoatLabels.properties b/src/main/resources/lessons/csrf/i18n/WebGoatLabels.properties
index 1b90e4b21..934ee4a43 100644
--- a/src/main/resources/lessons/csrf/i18n/WebGoatLabels.properties
+++ b/src/main/resources/lessons/csrf/i18n/WebGoatLabels.properties
@@ -10,6 +10,10 @@ csrf-get.hint4=The trigger can be manual or scripted to happen automatically
csrf-same-host=It appears your request is coming from the same host you are submitting to.
+csrf-request-rejected=That request did not start inside WebGoat, so it was not carried out.
+
+csrf-feedback-invalid-content-type=Feedback has to be sent as application/json.
+
csrf-you-forgot-something=There's something missing from your request it appears, so I can't process it.
csrf-review.success=It appears you have submitted correctly from another site. Go reload and see if your post is there.
diff --git a/src/main/resources/lessons/csrf/js/csrf-review.js b/src/main/resources/lessons/csrf/js/csrf-review.js
index 389107530..a3d092813 100644
--- a/src/main/resources/lessons/csrf/js/csrf-review.js
+++ b/src/main/resources/lessons/csrf/js/csrf-review.js
@@ -29,6 +29,15 @@ $(document).ready(function () {
'';
getChallenges();
+ loadReviewToken();
+
+ // the review form posts the token that belongs to this session, it is no longer a fixed
+ // value baked into the page which anybody could copy into a form of their own
+ function loadReviewToken() {
+ $.get('csrf/review/token', function (response) {
+ $('#csrfReviewToken').val(response.token);
+ });
+ }
function getChallenges() {
$("#list").empty();
diff --git a/src/main/resources/lessons/idor/i18n/WebGoatLabels.properties b/src/main/resources/lessons/idor/i18n/WebGoatLabels.properties
index 61d97da7d..d3282dbbb 100644
--- a/src/main/resources/lessons/idor/i18n/WebGoatLabels.properties
+++ b/src/main/resources/lessons/idor/i18n/WebGoatLabels.properties
@@ -44,3 +44,10 @@ idor.view.own.profile.failure2=You need to authenticate as tom first.
idor.view.other.profile.failure1=You must authenticate first
idor.view.other.profile.failure2=<>
+
+idor.diff.no.hidden.attributes=The response holds exactly the attributes you see above. The internal id and the role stay on the server, so there is no hidden one to name.
+idor.view.profile.denied=Access denied. A profile is visible only to the user it belongs to.
+idor.view.profile.own=Here is your own profile.
+idor.view.own.profile.direct=The profile below came from your session. An object reference sent along with the request is not used to look anything up.
+idor.edit.profile.denied=Access denied. A profile can be changed only by the user it belongs to.
+idor.edit.profile.updated=Your own profile was updated. The role is not taken from the request body.
diff --git a/src/main/resources/lessons/insecurelogin/documentation/InsecureLogin_Task.adoc b/src/main/resources/lessons/insecurelogin/documentation/InsecureLogin_Task.adoc
index 5d3c92a85..64da32262 100755
--- a/src/main/resources/lessons/insecurelogin/documentation/InsecureLogin_Task.adoc
+++ b/src/main/resources/lessons/insecurelogin/documentation/InsecureLogin_Task.adoc
@@ -1,4 +1,4 @@
=== Let's try
-Click the "log in" button to send a request containing the login credentials of another user.
-Then, write these credentials into the appropriate fields and submit them to confirm.
-Try using a packet sniffer to intercept the request.
+Click the "log in" button to send a login request.
+Now put a packet sniffer on it and try to read that user's credentials out of the request.
+You will not find any: the request carries no credentials, the check happens on the server, so there is nothing to capture and type into the fields below.
diff --git a/src/main/resources/lessons/insecurelogin/js/credentials.js b/src/main/resources/lessons/insecurelogin/js/credentials.js
index d3a936b52..c9e84a21a 100755
--- a/src/main/resources/lessons/insecurelogin/js/credentials.js
+++ b/src/main/resources/lessons/insecurelogin/js/credentials.js
@@ -1,6 +1,7 @@
function submit_secret_credentials() {
var xhttp = new XMLHttpRequest();
xhttp['open']('POST', 'InsecureLogin/login', true);
- //sending the request is obfuscated, to descourage js reading
- var _0xb7f9=["\x43\x61\x70\x74\x61\x69\x6E\x4A\x61\x63\x6B","\x42\x6C\x61\x63\x6B\x50\x65\x61\x72\x6C","\x73\x74\x72\x69\x6E\x67\x69\x66\x79","\x73\x65\x6E\x64"];xhttp[_0xb7f9[3]](JSON[_0xb7f9[2]]({username:_0xb7f9[0],password:_0xb7f9[1]}))
+ //the credentials live on the server; nothing is shipped to the page and nothing is put
+ //on the wire, so a packet capture has nothing to show
+ xhttp['send']()
}
diff --git a/src/main/resources/lessons/jwt/db/migration/V2019_09_25_1__jwt.sql b/src/main/resources/lessons/jwt/db/migration/V2019_09_25_1__jwt.sql
index 975574373..c51aded43 100644
--- a/src/main/resources/lessons/jwt/db/migration/V2019_09_25_1__jwt.sql
+++ b/src/main/resources/lessons/jwt/db/migration/V2019_09_25_1__jwt.sql
@@ -3,5 +3,5 @@ CREATE TABLE jwt_keys(
key varchar(20)
);
-INSERT INTO jwt_keys VALUES ('webgoat_key', 'qwertyqwerty1234');
-INSERT INTO jwt_keys VALUES ('webwolf_key', 'doesnotreallymatter');
+INSERT INTO jwt_keys VALUES ('webgoat_key', SUBSTRING(REPLACE(CAST(UUID() AS VARCHAR(36)), '-', '') FROM 1 FOR 16));
+INSERT INTO jwt_keys VALUES ('webwolf_key', SUBSTRING(REPLACE(CAST(UUID() AS VARCHAR(36)), '-', '') FROM 1 FOR 16));
diff --git a/src/main/resources/lessons/lessontemplate/documentation/lesson-template-attack.adoc b/src/main/resources/lessons/lessontemplate/documentation/lesson-template-attack.adoc
index c40a8f4bb..e54882344 100644
--- a/src/main/resources/lessons/lessontemplate/documentation/lesson-template-attack.adoc
+++ b/src/main/resources/lessons/lessontemplate/documentation/lesson-template-attack.adoc
@@ -9,7 +9,7 @@ import org.owasp.webgoat.container.assignments.AssignmentEndpoint;@RestControlle
@AssignmentHints({"lesson-template.hints.1", "lesson-template.hints.2", "lesson-template.hints.3"}) // <2>
public class SampleAttack implements AssignmentEndpoint { // <3>
- private final String secretValue = "secr37Value";
+ private final String secretValue = "the value the server generated for this run";
@Autowired
private UserSessionData userSessionData; // <4>
diff --git a/src/main/resources/lessons/missingac/db/migration/V2021_11_03_1__ac.sql b/src/main/resources/lessons/missingac/db/migration/V2021_11_03_1__ac.sql
index 7a7b09b58..096e73ccf 100644
--- a/src/main/resources/lessons/missingac/db/migration/V2021_11_03_1__ac.sql
+++ b/src/main/resources/lessons/missingac/db/migration/V2021_11_03_1__ac.sql
@@ -4,6 +4,6 @@ CREATE TABLE access_control_users(
admin boolean
);
-INSERT INTO access_control_users VALUES ('Tom', 'qwertyqwerty1234', false);
-INSERT INTO access_control_users VALUES ('Jerry', 'doesnotreallymatter', true);
-INSERT INTO access_control_users VALUES ('Sylvester', 'testtesttest', false);
+INSERT INTO access_control_users VALUES ('Tom', CAST(UUID() AS VARCHAR(36)), false);
+INSERT INTO access_control_users VALUES ('Jerry', CAST(UUID() AS VARCHAR(36)), true);
+INSERT INTO access_control_users VALUES ('Sylvester', CAST(UUID() AS VARCHAR(36)), false);
diff --git a/src/main/resources/lessons/missingac/html/MissingFunctionAC.html b/src/main/resources/lessons/missingac/html/MissingFunctionAC.html
index 73b49b992..1b7e3b208 100644
--- a/src/main/resources/lessons/missingac/html/MissingFunctionAC.html
+++ b/src/main/resources/lessons/missingac/html/MissingFunctionAC.html
@@ -34,13 +34,8 @@
Compose Message
-
+
diff --git a/src/main/resources/webgoat/templates/main_new.html b/src/main/resources/webgoat/templates/main_new.html
index a9d250fc6..4fd8b3fda 100644
--- a/src/main/resources/webgoat/templates/main_new.html
+++ b/src/main/resources/webgoat/templates/main_new.html
@@ -51,8 +51,12 @@
![\"Tom\"](\"images/tom.png\")
");
return failed(this).feedback("ssrf.tom").output(html.toString()).build();
- } else if (url.matches("images/jerry\\.png")) {
- html.append(
- "![\"Jerry\"](\"images/jerry.png\")
");
- return success(this).feedback("ssrf.success").output(html.toString()).build();
} else {
html.append("![\"Silly](\"images/cat.jpg\")
");
return failed(this).feedback("ssrf.failure").output(html.toString()).build();
diff --git a/src/main/java/org/owasp/webgoat/lessons/ssrf/SSRFTask2.java b/src/main/java/org/owasp/webgoat/lessons/ssrf/SSRFTask2.java
index 9f7a09c0a..4783e1bad 100644
--- a/src/main/java/org/owasp/webgoat/lessons/ssrf/SSRFTask2.java
+++ b/src/main/java/org/owasp/webgoat/lessons/ssrf/SSRFTask2.java
@@ -5,13 +5,8 @@
package org.owasp.webgoat.lessons.ssrf;
import static org.owasp.webgoat.container.assignments.AttackResultBuilder.failed;
-import static org.owasp.webgoat.container.assignments.AttackResultBuilder.success;
-import java.io.IOException;
-import java.io.InputStream;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.nio.charset.StandardCharsets;
+import java.util.List;
import org.owasp.webgoat.container.assignments.AssignmentEndpoint;
import org.owasp.webgoat.container.assignments.AssignmentHints;
import org.owasp.webgoat.container.assignments.AttackResult;
@@ -24,6 +19,9 @@
@AssignmentHints({"ssrf.hint3"})
public class SSRFTask2 implements AssignmentEndpoint {
+ /** The only resources this page is allowed to render, everything else is rejected. */
+ private static final List