diff --git a/webgoat-integration-tests/src/test/java/org/owasp/webgoat/PathTraversalTest.java b/webgoat-integration-tests/src/test/java/org/owasp/webgoat/PathTraversalTest.java index 22a0606f4..12c536841 100644 --- a/webgoat-integration-tests/src/test/java/org/owasp/webgoat/PathTraversalTest.java +++ b/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")); diff --git a/webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileUploadRetrieval.java b/webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileUploadRetrieval.java index ebdd69fd8..b8e7dc757 100644 --- a/webgoat-lessons/path-traversal/src/main/java/org/owasp/webgoat/path_traversal/ProfileUploadRetrieval.java +++ b/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) { - var catPicture = new File(catPicturesDirectory, (id == null ? RandomUtils.nextInt(1, 11) : id) + ".jpg"); - + 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"); + if (catPicture.getName().toLowerCase().contains("path-traversal-secret.jpg")) { return ResponseEntity.ok() .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE)) .body(FileCopyUtils.copyToByteArray(catPicture)); } - return ResponseEntity.ok() - .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE)) + 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(Base64.getEncoder().encode(FileCopyUtils.copyToByteArray(catPicture))); + .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() - .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE)) - .body(StringUtils.arrayToCommaDelimitedString(catPicture.getParentFile().listFiles()).getBytes()); + return ResponseEntity.badRequest().build(); } } diff --git a/webgoat-lessons/path-traversal/src/main/resources/i18n/WebGoatLabels.properties b/webgoat-lessons/path-traversal/src/main/resources/i18n/WebGoatLabels.properties index 816fa508b..d960c3c30 100644 --- a/webgoat-lessons/path-traversal/src/main/resources/i18n/WebGoatLabels.properties +++ b/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? \ No newline at end of file +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 \ No newline at end of file diff --git a/webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_mitigation.adoc b/webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_mitigation.adoc index 7512c84f5..64c59b579 100644 --- a/webgoat-lessons/path-traversal/src/main/resources/lessonPlans/en/PathTraversal_upload_mitigation.adoc +++ b/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 @@ -39,4 +68,5 @@ By default Spring Boot has protection for usage of for example `../` in a path. if you replace `1.jpg` with `../../secret.txt` it will block the request. With query parameters that protection will not be there. -In the lesson about "File uploads" more examples of vulnerabilities are shown. \ No newline at end of file +In the lesson about "File uploads" more examples of vulnerabilities are shown. + diff --git a/webgoat-lessons/path-traversal/src/test/java/org/owasp/webgoat/path_traversal/ProfileUploadRetrievalTest.java b/webgoat-lessons/path-traversal/src/test/java/org/owasp/webgoat/path_traversal/ProfileUploadRetrievalTest.java index d5598a3ae..1e1f41382 100644 --- a/webgoat-lessons/path-traversal/src/test/java/org/owasp/webgoat/path_traversal/ProfileUploadRetrievalTest.java +++ b/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=../../")) - .andExpect(status().is(200)) - .andExpect(content().string(containsString("/path-traversal-secret.jpg"))) - .andExpect(content().contentTypeCompatibleWith(MediaType.IMAGE_JPEG)); + var uri = new URI("/PathTraversal/random-picture?id=%2E%2E%2F%2E%2E%2F"); + mockMvc.perform(get(uri)) + .andExpect(status().is(404)) + .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));