Last assignment now filters out .. and / so encoding plays a role now

webgoat-integration-tests/src/test/java/org/owasp/webgoat/PathTraversalTest.java

@@ -76,11 +76,12 @@ public class PathTraversalTest extends IntegrationTest {
     public void assignment4() throws IOException {
         startLesson("PathTraversal");
 
-        RestAssured.given()
+        var uri = "/WebGoat/PathTraversal/random-picture?id=%2E%2E%2F%2E%2E%2Fpath-traversal-secret";
+        RestAssured.given().urlEncodingEnabled(false)
                 .when()
                 .relaxedHTTPSValidation()
                 .cookie("JSESSIONID", getWebGoatCookie())
-                .get("/WebGoat/PathTraversal/random-picture?id=../../path-traversal-secret")
+                .get(uri)
                 .then()
                 .statusCode(200)
                 .content(CoreMatchers.is("You found it submit the SHA-512 hash of your username as answer"));

webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileUploadRetrieval.java

@@ -6,15 +6,16 @@ import org.owasp.webgoat.assignments.AssignmentEndpoint;
 import org.owasp.webgoat.assignments.AssignmentHints;
 import org.owasp.webgoat.assignments.AttackResult;
 import org.springframework.beans.factory.annotation.Value;
+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.security.crypto.codec.Hex;
 import org.springframework.util.FileCopyUtils;
 import org.springframework.util.StringUtils;
 import org.springframework.web.bind.annotation.*;
 
 import javax.annotation.PostConstruct;
+import javax.servlet.http.HttpServletRequest;
 import java.io.File;
 import java.io.IOException;
 import java.net.URI;
@@ -26,7 +27,13 @@ import static org.springframework.util.FileCopyUtils.copy;
 import static org.springframework.util.ResourceUtils.getFile;
 
 @RestController
-@AssignmentHints({"path-traversal-profile-retrieve.hint1", "path-traversal-profile-retrieve.hint2", "path-traversal-profile-retrieve.hint3, path-traversal-profile-retrieve.hint4", "path-traversal-profile-retrieve.hint5"})
+@AssignmentHints({
+        "path-traversal-profile-retrieve.hint1",
+        "path-traversal-profile-retrieve.hint2",
+        "path-traversal-profile-retrieve.hint3",
+        "path-traversal-profile-retrieve.hint4",
+        "path-traversal-profile-retrieve.hint5",
+        "path-traversal-profile-retrieve.hint6"})
 @Slf4j
 public class ProfileUploadRetrieval extends AssignmentEndpoint {
 
@@ -65,25 +72,33 @@ public class ProfileUploadRetrieval extends AssignmentEndpoint {
 
     @GetMapping("/PathTraversal/random-picture")
     @ResponseBody
-    public ResponseEntity<?> getProfilePicture(@RequestParam(value = "id", required = false) String id) {
+    public ResponseEntity<?> getProfilePicture(HttpServletRequest request) {
+        var queryParams = request.getQueryString();
+        if (queryParams != null && (queryParams.contains("..") || queryParams.contains("/"))) {
+            return ResponseEntity.badRequest().body("Illegal characters are not allowed in the query params");
+        }
+        try {
+            var id = request.getParameter("id");
             var catPicture = new File(catPicturesDirectory, (id == null ? RandomUtils.nextInt(1, 11) : id) + ".jpg");
 
-        try {
             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.exists()) {
                 return ResponseEntity.ok()
                         .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE))
                         .location(new URI("/PathTraversal/random-picture?id=" + catPicture.getName()))
                         .body(Base64.getEncoder().encode(FileCopyUtils.copyToByteArray(catPicture)));
+            }
+            return ResponseEntity.status(HttpStatus.NOT_FOUND)
+                    .location(new URI("/PathTraversal/random-picture?id=" + catPicture.getName()))
+                    .body(StringUtils.arrayToCommaDelimitedString(catPicture.getParentFile().listFiles()).getBytes());
         } catch (IOException | URISyntaxException e) {
-            log.error("Unable to download picture", e);
+            log.error("Image not found", e);
         }
 
-        return ResponseEntity.ok()
+        return ResponseEntity.badRequest().build();
-                .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE))
-                .body(StringUtils.arrayToCommaDelimitedString(catPicture.getParentFile().listFiles()).getBytes());
     }
 }

webgoat-lessons/path-traversal/src/main/resources/i18n/WebGoatLabels.properties

@@ -44,4 +44,5 @@ path-traversal-profile-retrieve.hint1=Can you specify the image to be fetched?
 path-traversal-profile-retrieve.hint2=Look at the location header...
 path-traversal-profile-retrieve.hint3=Use /random?id=1 for example to fetch a specific image
 path-traversal-profile-retrieve.hint4=Use /random/?id=../../1.jpg to navigate to a different directory
-path-traversal-profile-retrieve.hint5=Do you see a difference when retrieving a file or a directory?
+path-traversal-profile-retrieve.hint5='..' and '/' are no longer allowed, can you bypass this restriction
+path-traversal-profile-retrieve.hint6=Use url encoding for ../ to bypass the restriction

webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_mitigation.adoc

@@ -32,6 +32,35 @@ in any other directory.
 Make sure that in any case you build detection for catching these cases but be careful with returning explicit information
 to the user. Every small detail might give the attacker knowledge about your system.
 
+==== Be aware...
+
+As shown in the previous examples be careful which method you use for retrieving parameters especially query parameters.
+Spring Boot does a decent job denying invalid path variables. To recap:
+
+[source]
+----
+@Getter("/f")
+public void f(@RequestParam("name") String name) {
+  //name is automatically decoded so %2E%2E%2F%2E%2E%2Ftest will become ../../test
+}
+
+@Getter("/g")
+public void g(HttpServletRequest request) {
+  var queryString = request.getQueryString(); // will return
+}
+
+@Getter("/h")
+public void h(HttpServletRequest request) {
+  var name = request.getParam("name"); //will return ../../test
+----
+
+If you invoke `/f` with `/f?name=%2E%2E%2F%2E%2E%2Ftest` it will become `../../test`. If you invoke `g` with
+`/g?name=%2E%2E%2F%2E%2E%2Ftest` it will return `%2E%2E%2F%2E%2E%2Ftest` *NO* decoding will be applied.
+The behaviour of `/h` with the same parameter will be the same as `/f`
+
+As you can see be careful and familiarize yourself with the correct methods to call. In every case write a
+unit test in such cases which covers encoded characters.
+
 ==== Spring Boot protection
 
 By default Spring Boot has protection for usage of for example `../` in a path. The implementation can be found in
@@ -40,3 +69,4 @@ if you replace `1.jpg` with `../../secret.txt` it will block the request. With q
 will not be there.
 
 In the lesson about "File uploads" more examples of vulnerabilities are shown.
+

webgoat-lessons/path-traversal/src/test/java/org/owasp/webgoat/path_traversal/ProfileUploadRetrievalTest.java

@@ -10,11 +10,15 @@ import org.springframework.http.MediaType;
 import org.springframework.security.core.token.Sha512DigestUtils;
 import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
 import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;
+import org.springframework.test.web.servlet.result.MockMvcResultHandlers;
 import org.springframework.test.web.servlet.setup.MockMvcBuilders;
 
+import java.net.URI;
+
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.Matchers.containsString;
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
 
 @RunWith(SpringJUnit4ClassRunner.class)
@@ -33,26 +37,28 @@ public class ProfileUploadRetrievalTest extends LessonTest {
     @Test
     public void solve() throws Exception {
         //Look at the response
-        mockMvc.perform(MockMvcRequestBuilders.get("/PathTraversal/random-picture"))
+        mockMvc.perform(get("/PathTraversal/random-picture"))
                 .andExpect(status().is(200))
                 .andExpect(header().exists("Location"))
                 .andExpect(header().string("Location", containsString("?id=")))
                 .andExpect(content().contentTypeCompatibleWith(MediaType.IMAGE_JPEG));
 
         //Browse the directories
-        mockMvc.perform(MockMvcRequestBuilders.get("/PathTraversal/random-picture?id=../../"))
+        var uri = new URI("/PathTraversal/random-picture?id=%2E%2E%2F%2E%2E%2F");
-                .andExpect(status().is(200))
+        mockMvc.perform(get(uri))
-                .andExpect(content().string(containsString("/path-traversal-secret.jpg")))
+                .andExpect(status().is(404))
-                .andExpect(content().contentTypeCompatibleWith(MediaType.IMAGE_JPEG));
+                .andDo(MockMvcResultHandlers.print())
+                .andExpect(content().string(containsString("/path-traversal-secret.jpg")));
 
         //Retrieve the secret file (note: .jpg is added by the server)
-        mockMvc.perform(MockMvcRequestBuilders.get("/PathTraversal/random-picture?id=../../path-traversal-secret"))
+        uri = new URI("/PathTraversal/random-picture?id=%2E%2E%2F%2E%2E%2Fpath-traversal-secret");
+        mockMvc.perform(get(uri))
                 .andExpect(status().is(200))
                 .andExpect(content().string("You found it submit the SHA-512 hash of your username as answer"))
                 .andExpect(content().contentTypeCompatibleWith(MediaType.IMAGE_JPEG));
 
         //Post flag
-        mockMvc.perform(MockMvcRequestBuilders.post("/PathTraversal/random").param("secret", Sha512DigestUtils.shaHex("unit-test")))
+        mockMvc.perform(post("/PathTraversal/random").param("secret", Sha512DigestUtils.shaHex("unit-test")))
                 .andExpect(status().is(200))
                 .andExpect(jsonPath("$.assignment", equalTo("ProfileUploadRetrieval")))
                 .andExpect(jsonPath("$.lessonCompleted", is(true)));
@@ -60,7 +66,7 @@ public class ProfileUploadRetrievalTest extends LessonTest {
 
     @Test
     public void shouldReceiveRandomPicture() throws Exception {
-        mockMvc.perform(MockMvcRequestBuilders.get("/PathTraversal/random-picture"))
+        mockMvc.perform(get("/PathTraversal/random-picture"))
                 .andExpect(status().is(200))
                 .andExpect(header().exists("Location"))
                 .andExpect(content().contentTypeCompatibleWith(MediaType.IMAGE_JPEG));
@@ -68,7 +74,7 @@ public class ProfileUploadRetrievalTest extends LessonTest {
 
     @Test
     public void unknownFileShouldGiveDirectoryContents() throws Exception {
-        mockMvc.perform(MockMvcRequestBuilders.get("/PathTraversal/random-picture?id=test"))
+        mockMvc.perform(get("/PathTraversal/random-picture?id=test"))
                 .andExpect(status().is(200))
                 .andExpect(content().string(containsString("cats/8.jpg")))
                 .andExpect(content().contentTypeCompatibleWith(MediaType.IMAGE_JPEG));