Fix lessons

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

@@ -2,10 +2,7 @@ package org.owasp.webgoat;
 
 import io.restassured.RestAssured;
 import org.hamcrest.CoreMatchers;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
+import org.junit.*;
 import org.junit.rules.TemporaryFolder;
 import org.springframework.security.core.token.Sha512DigestUtils;
 
@@ -16,15 +13,8 @@ import java.util.Map;
 
 public class PathTraversalTest extends IntegrationTest {
 
-    private static String OS = System.getProperty("os.name").toLowerCase();
     @Rule
     public TemporaryFolder temporaryFolder = new TemporaryFolder();
-    private File folder;
-
-    @Before
-    public void setup() throws IOException {
-        this.folder = temporaryFolder.newFolder();
-    }
 
     @Test
     public void assignment1() throws IOException {
@@ -75,7 +65,7 @@ public class PathTraversalTest extends IntegrationTest {
                         .when()
                         .relaxedHTTPSValidation()
                         .cookie("JSESSIONID", getWebGoatCookie())
-                        .multiPart("uploadedFileRetrieval", "../test.jpg", Files.readAllBytes(fileToUpload.toPath()))
+                        .multiPart("uploadedFileRemoveUserInput", "../test.jpg", Files.readAllBytes(fileToUpload.toPath()))
                         .post("/WebGoat/PathTraversal/profile-upload-remove-user-input")
                         .then()
                         .statusCode(200)
@@ -90,7 +80,7 @@ public class PathTraversalTest extends IntegrationTest {
                 .when()
                 .relaxedHTTPSValidation()
                 .cookie("JSESSIONID", getWebGoatCookie())
-                .get("/WebGoat/PathTraversal/random?id=../../path-traversal-secret")
+                .get("/WebGoat/PathTraversal/random-picture?id=../../path-traversal-secret")
                 .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/ProfileUploadBase.java

@@ -5,15 +5,11 @@ import lombok.SneakyThrows;
 import org.owasp.webgoat.assignments.AssignmentEndpoint;
 import org.owasp.webgoat.assignments.AttackResult;
 import org.owasp.webgoat.session.WebSession;
-import org.springframework.beans.factory.annotation.Value;
 import org.springframework.http.MediaType;
 import org.springframework.http.ResponseEntity;
 import org.springframework.util.FileCopyUtils;
 import org.springframework.util.FileSystemUtils;
 import org.springframework.util.StringUtils;
-import org.springframework.web.bind.annotation.GetMapping;
-import org.springframework.web.bind.annotation.RequestParam;
-import org.springframework.web.bind.annotation.ResponseBody;
 import org.springframework.web.multipart.MultipartFile;
 
 import java.io.File;
@@ -45,7 +41,8 @@ public class ProfileUploadBase extends AssignmentEndpoint {
             var uploadedFile = new File(uploadDirectory, fullName);
             uploadedFile.createNewFile();
             FileCopyUtils.copy(file.getBytes(), uploadedFile);
-            if (userAttemptedToSolveLesson(uploadDirectory, uploadedFile)) {
+
+            if (attemptWasMade(uploadDirectory, uploadedFile)) {
                 return solvedIt(uploadedFile);
             }
             return informationMessage(this).feedback("path-traversal-profile-updated").feedbackArgs(uploadedFile.getAbsoluteFile()).build();
@@ -55,7 +52,7 @@ public class ProfileUploadBase extends AssignmentEndpoint {
         }
     }
 
-    private boolean userAttemptedToSolveLesson(File expectedUploadDirectory, File uploadedFile) throws IOException {
+    private boolean attemptWasMade(File expectedUploadDirectory, File uploadedFile) throws IOException {
         return !expectedUploadDirectory.getCanonicalPath().equals(uploadedFile.getParentFile().getCanonicalPath());
     }
 
@@ -63,7 +60,7 @@ public class ProfileUploadBase extends AssignmentEndpoint {
         if (uploadedFile.getCanonicalFile().getParentFile().getName().endsWith("PathTraversal")) {
             return success(this).build();
         }
-        return failed(this).build();
+        return failed(this).attemptWasMade().feedback("path-traversal-profile-attempt").feedbackArgs(uploadedFile.getCanonicalPath()).build();
     }
 
     public ResponseEntity<?> getProfilePicture() {

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

@@ -24,7 +24,7 @@ public class ProfileUploadFix extends ProfileUploadBase {
     public AttackResult uploadFileHandler(
             @RequestParam("uploadedFileFix") MultipartFile file,
             @RequestParam(value = "fullNameFix", required = false) String fullName) {
-        return super.execute(file, fullName != null ? fullName.replaceAll("../", "") : "");
+        return super.execute(file, fullName != null ? fullName.replace("../", "") : "");
     }
 
     @GetMapping("/PathTraversal/profile-picture-fix")

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

@@ -23,7 +23,7 @@ public class ProfileUploadRemoveUserInput extends ProfileUploadBase {
 
     @PostMapping(value = "/PathTraversal/profile-upload-remove-user-input", consumes = ALL_VALUE, produces = APPLICATION_JSON_VALUE)
     @ResponseBody
-    public AttackResult uploadFileHandler(@RequestParam("uploadedFileRetrieval") MultipartFile file) {
+    public AttackResult uploadFileHandler(@RequestParam("uploadedFileRemoveUserInput") MultipartFile file) {
         return super.execute(file, file.getOriginalFilename());
     }
 }

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

@@ -55,14 +55,15 @@ public class ProfileUploadRetrieval extends AssignmentEndpoint {
     }
 
     @PostMapping("/PathTraversal/random")
-    protected AttackResult execute(@RequestParam(value = "secret", required = false) String secret) {
+    @ResponseBody
+    public AttackResult execute(@RequestParam(value = "secret", required = false) String secret) {
         if (Sha512DigestUtils.shaHex(getWebSession().getUserName()).equalsIgnoreCase(secret)) {
             return success(this).build();
         }
         return failed(this).build();
     }
 
-    @GetMapping("/PathTraversal/random")
+    @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");
@@ -75,7 +76,7 @@ public class ProfileUploadRetrieval extends AssignmentEndpoint {
             }
             return ResponseEntity.ok()
                     .contentType(MediaType.parseMediaType(MediaType.IMAGE_JPEG_VALUE))
-                    .location(new URI("/PathTraversal/random?id=" + catPicture.getName()))
+                    .location(new URI("/PathTraversal/random-picture?id=" + catPicture.getName()))
                     .body(Base64.getEncoder().encode(FileCopyUtils.copyToByteArray(catPicture)));
         } catch (IOException | URISyntaxException e) {
             log.error("Unable to download picture", e);

webgoat-lessons/path-traversal/src/main/resources/html/PathTraversal.html

@@ -129,7 +129,7 @@
                   successCallback="profileUploadCallbackRemoveUserInput"
                   failureCallback="profileUploadCallbackRemoveUserInput"
                   informationalCallback="profileUploadCallbackRemoveUserInput"
-                  prepareData="profileUploadFix"
+                  prepareData="profileUploadRemoveUserInput"
                   enctype="multipart/form-data"
                   action="/WebGoat/PathTraversal/profile-upload-remove-user-input">
                 <div class="preview text-center">
@@ -176,19 +176,21 @@
 <div class="lesson-page-wrapper">
     <div class="adoc-content" th:replace="doc:PathTraversal_retrieval.adoc"></div>
     <div class="attack-container">
-        <div class="assignment-success"><i class="fa fa-2 fa-check hidden" aria-hidden="true"></i></div>
-        <div class="upload-container">
-            <div class="form-group">
-                <button class="btn btn-primary btn-block" onclick="newRandomPicture()">Show random cat picture</button>
+
+        <div class="container-fluid">
+            <div class="input-group" style="margin-top: 10px">
+                <button class="btn btn-primary" onclick="newRandomPicture()">Show random cat picture
+                </button>
             </div>
+            <br/>
             <div>
-                <img id="randomCatPicture" th:src="@{/images/cats/1.jpg}"/>
+                <img id="randomCatPicture" th:src="@{/images/cats/1.jpg}" width="50%" height="50%"/>
             </div>
 
 
             <br/>
-
             <form class="attack-form" method="POST" name="form" action="/WebGoat/PathTraversal/random">
+                <div class="assignment-success"><i class="fa fa-2 fa-check hidden" aria-hidden="true"></i></div>
                 <div class="form-group">
                     <div class="input-group">
                         <div class="input-group-addon"><i class="fa fa-flag-checkered" aria-hidden="true"
@@ -199,8 +201,8 @@
                         <button type="submit" class="btn btn-primary">Submit secret</button>
                     </div>
                 </div>
-            </form>
 
+            </form>
 
             <div class="attack-feedback"></div>
             <div class="attack-output"></div>

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

@@ -25,6 +25,7 @@
 path-traversal-title=Path traversal
 path-traversal-profile-updated=Profile has been updated, your image is available at: {0}"
 path-traversal-profile-empty-file=File appears to be empty please upload a non empty file
+path-traversal-profile-attempt=Nice try, but the directory({0}) is incorrect, please write the file to the correct directory
 path-traversal-profile-empty-name=Name is empty
 path-traversal-profile.hint1=Try updating the profile WebGoat will display the location
 path-traversal-profile.hint2=Look at the displayed location how is the file name on the server constructed?

webgoat-lessons/path-traversal/src/main/resources/js/path_traversal.js

@@ -35,7 +35,7 @@ webgoat.customjs.profileUploadCallbackFix = function () {
 webgoat.customjs.profileUploadRemoveUserInput = function () {
     var picture = document.getElementById("uploadedFileRemoveUserInput").files[0];
     var formData = new FormData();
-    formData.append("uploadedFile", picture);
+    formData.append("uploadedFileRemoveUserInput", picture);
     formData.append("fullName", $("#fullNameRemoveUserInput").val());
     formData.append("email", $("#emailRemoveUserInput").val());
     formData.append("password", $("#passwordRemoveUserInput").val());
@@ -56,7 +56,7 @@ webgoat.customjs.profileUploadCallbackRetrieval = function () {
 }
 
 function newRandomPicture() {
-    $.get("PathTraversal/random", function (result, status) {
+    $.get("PathTraversal/random-picture", function (result, status) {
         document.getElementById("randomCatPicture").src = "data:image/png;base64," + result;
     });
 }

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

@@ -8,7 +8,7 @@ upload overwriting critical system files.
 
 For example let's assume we have an application which hosts some files and they can be requested in the following
 format: `http://example.com/file=report.pdf` now as an attacker you are interested in other files of course so
-you try `http://example.com/../../../../../etc/passwd`. In this case you try walk up to the root of the filesystem
+you try `http://example.com/file=../../../../../etc/passwd`. In this case you try walk up to the root of the filesystem
 and then go into `/etc/passwd` to gain access to this file. The `../` is called dot-dot-slash which is another name
 for this attack.
 
@@ -21,17 +21,4 @@ Also note that avoiding applications filtering those encodings double encoding m
 might be necessary in the case where you have a system A which calls system B. System A will only decode once and
 will call B with the still encoded URL.
 
-Now let's try some examples
-
-1. Example with upload where you have to overwrite one of the files
-1a Example upload where developer implemented a fix by removing ../ from the path
-1b No longer using name of user to store file so you need proxy to intercept
-2. Example with download for example the idea is to download a pdf but there is also a txt file
-3. Same as 2 with blacklisting implemented (see example from Workpress plugin)
-4. Zip path traversal where you have to create a zip file which overwrites a file in another directory
-
-
-- Run app with least priviledge
-- Before storing the file calculate the location
-- Same for reading is it actually reading from the directory you allow
 

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

@@ -1,6 +1,6 @@
 === Retrieving other files with a path traversal
 
 Path traversals are not limited to file uploads also when retrieving files it can be the case that a path traversal
-is possible to retrieve other files from the system. In this assignment try to find a file called `secret.txt`
+is possible to retrieve other files from the system. In this assignment try to find a file called `path-traversal-secret.jpg`
 
 

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

@@ -1,5 +1,6 @@
 === Path traversal while uploading files
 
+The developer became aware of the vulnerability and implemented a fix which removed the `../` from the input.
 Again the same assignment but can you bypass the implemented fix?
 
 |===
@@ -7,5 +8,4 @@ Again the same assignment but can you bypass the implemented fix?
 
 |`operatingSystem:os[]`
 |`webGoatTempDir:temppath[]PathTraversal/`
-
 |===

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

@@ -1,6 +1,6 @@
 === Path traversal while uploading files
 
-The developer became aware of the vulnerability by not validating the input of the `full name` input field.
+The developer again became aware of the vulnerability by not validating the input of the `full name` input field.
 A fix was made in an attempt to solve this vulnerability.
 
 Again the same assignment but can you bypass the implemented fix?

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

@@ -0,0 +1,42 @@
+package org.owasp.webgoat.path_traversal;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mockito.Mockito;
+import org.owasp.webgoat.plugins.LessonTest;
+import org.owasp.webgoat.session.WebSession;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.mock.web.MockMultipartFile;
+import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+
+import java.io.File;
+import java.io.IOException;
+
+import static org.junit.Assert.*;
+
+@RunWith(SpringJUnit4ClassRunner.class)
+public class ProfileUploadBaseTest extends LessonTest {
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+    private File folder;
+    @Autowired
+    private PathTraversal pathTraversal;
+
+
+    @Before
+    public void setup() throws IOException {
+        this.folder = temporaryFolder.newFolder();
+    }
+
+    @Test
+    public void shouldNotOverwriteExistingFile() throws IOException {
+        var existingFile = new File(folder, "test.jpg").createNewFile();
+        var profilePicture = new MockMultipartFile("uploadedFileFix", "../picture.jpg", "text/plain", "an image".getBytes());
+        new ProfileUploadBase(this.folder.getPath(), this.webSession).execute(profilePicture, "test.jpg");
+    }
+
+}

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

@@ -1,6 +1,7 @@
 package org.owasp.webgoat.path_traversal;
 
 import org.hamcrest.CoreMatchers;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -9,11 +10,15 @@ import org.junit.runner.RunWith;
 import org.mockito.Mockito;
 import org.owasp.webgoat.plugins.LessonTest;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Value;
 import org.springframework.mock.web.MockMultipartFile;
 import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
 import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;
 import org.springframework.test.web.servlet.setup.MockMvcBuilders;
 
+import java.io.File;
+
+import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
 

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

@@ -13,6 +13,7 @@ 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 static org.springframework.test.web.servlet.result.MockMvcResultHandlers.print;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
 
@@ -41,6 +42,29 @@ public class ProfileUploadTest extends LessonTest {
                 .andExpect(jsonPath("$.lessonCompleted", CoreMatchers.is(true)));
     }
 
+    @Test
+    public void attemptWithWrongDirectory() throws Exception {
+        var profilePicture = new MockMultipartFile("uploadedFile", "../picture.jpg", "text/plain", "an image".getBytes());
+
+        mockMvc.perform(MockMvcRequestBuilders.multipart("/PathTraversal/profile-upload")
+                .file(profilePicture)
+                .param("fullName", "../../" + webSession.getUserName()))
+                .andExpect(status().is(200))
+                .andExpect(jsonPath("$.assignment", CoreMatchers.equalTo("ProfileUpload")))
+                .andExpect(jsonPath("$.feedback", CoreMatchers.containsString("Nice try")))
+                .andExpect(jsonPath("$.lessonCompleted", CoreMatchers.is(false)));
+    }
+
+    @Test
+    public void shouldNotOverrideExistingFile() throws Exception {
+        var profilePicture = new MockMultipartFile("uploadedFile", "picture.jpg", "text/plain", "an image".getBytes());
+        mockMvc.perform(MockMvcRequestBuilders.multipart("/PathTraversal/profile-upload")
+                .file(profilePicture)
+                .param("fullName", "../" + webSession.getUserName()))
+                .andExpect(jsonPath("$.output", CoreMatchers.containsString("Is a directory")))
+                .andExpect(status().is(200));
+    }
+
     @Test
     public void normalUpdate() throws Exception {
         var profilePicture = new MockMultipartFile("uploadedFile", "picture.jpg", "text/plain", "an image".getBytes());