From fcaa2d8589e3085a354c09f0161f0b240302cd65 Mon Sep 17 00:00:00 2001 From: Nanne Baars Date: Thu, 5 Jan 2023 07:47:38 +0100 Subject: [PATCH] Fix zip slip lesson. The lesson did not work properly as the directory is reused across several path traversal lessons. First thing before uploading the zip file we now clean the directory. The html had a reference to a location of the profile picture, this was part of a hint but this only causes confusion as this is not indicating to where you need to upload the picture with the Zip Slip vulnerability. The assignment now contains a direct hint as where the image needs to be saved. The assignment is about creating a vulnerable zip file and NOT about guessing where the image should be saved inside WebGoat. --- .../pathtraversal/ProfileUploadBase.java | 41 +++++++++++++------ .../lessons/pathtraversal/ProfileZipSlip.java | 14 ++++--- .../PathTraversal_zip_slip_assignment.adoc | 4 +- .../pathtraversal/html/PathTraversal.html | 2 - 4 files changed, 41 insertions(+), 20 deletions(-) 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 a1ca0967e..131f1674a 100644 --- a/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadBase.java +++ b/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileUploadBase.java @@ -3,10 +3,14 @@ package org.owasp.webgoat.lessons.pathtraversal; import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.nio.file.Files; +import java.util.Arrays; import java.util.Base64; +import java.util.List; import lombok.AllArgsConstructor; import lombok.Getter; import lombok.SneakyThrows; +import org.apache.commons.io.FilenameUtils; import org.owasp.webgoat.container.assignments.AssignmentEndpoint; import org.owasp.webgoat.container.assignments.AttackResult; import org.owasp.webgoat.container.session.WebSession; @@ -32,14 +36,9 @@ public class ProfileUploadBase extends AssignmentEndpoint { return failed(this).feedback("path-traversal-profile-empty-name").build(); } - var uploadDirectory = - new File(this.webGoatHomeDirectory, "/PathTraversal/" + webSession.getUserName()); - if (uploadDirectory.exists()) { - FileSystemUtils.deleteRecursively(uploadDirectory); - } + File uploadDirectory = cleanupAndCreateDirectoryForUser(); try { - uploadDirectory.mkdirs(); var uploadedFile = new File(uploadDirectory, fullName); uploadedFile.createNewFile(); FileCopyUtils.copy(file.getBytes(), uploadedFile); @@ -57,6 +56,17 @@ public class ProfileUploadBase extends AssignmentEndpoint { } } + @SneakyThrows + protected File cleanupAndCreateDirectoryForUser() { + var uploadDirectory = + new File(this.webGoatHomeDirectory, "/PathTraversal/" + webSession.getUserName()); + if (uploadDirectory.exists()) { + FileSystemUtils.deleteRecursively(uploadDirectory); + } + Files.createDirectories(uploadDirectory.toPath()); + return uploadDirectory; + } + private boolean attemptWasMade(File expectedUploadDirectory, File uploadedFile) throws IOException { return !expectedUploadDirectory @@ -87,18 +97,25 @@ public class ProfileUploadBase extends AssignmentEndpoint { var profileDirectoryFiles = profilePictureDirectory.listFiles(); if (profileDirectoryFiles != null && profileDirectoryFiles.length > 0) { - try (var inputStream = new FileInputStream(profileDirectoryFiles[0])) { - return Base64.getEncoder().encode(FileCopyUtils.copyToByteArray(inputStream)); - } catch (IOException e) { - return defaultImage(); - } + return Arrays.stream(profileDirectoryFiles) + .filter(file -> FilenameUtils.isExtension(file.getName(), List.of("jpg", "png"))) + .findFirst() + .map( + file -> { + try (var inputStream = new FileInputStream(profileDirectoryFiles[0])) { + return Base64.getEncoder().encode(FileCopyUtils.copyToByteArray(inputStream)); + } catch (IOException e) { + return defaultImage(); + } + }) + .orElse(defaultImage()); } else { return defaultImage(); } } @SneakyThrows - private byte[] defaultImage() { + protected byte[] defaultImage() { var inputStream = getClass().getResourceAsStream("/images/account.png"); return Base64.getEncoder().encode(FileCopyUtils.copyToByteArray(inputStream)); } 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 c14223560..49c7b15c3 100644 --- a/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileZipSlip.java +++ b/src/main/java/org/owasp/webgoat/lessons/pathtraversal/ProfileZipSlip.java @@ -13,13 +13,19 @@ import java.util.Enumeration; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; import lombok.SneakyThrows; +import lombok.extern.slf4j.Slf4j; import org.owasp.webgoat.container.assignments.AssignmentHints; import org.owasp.webgoat.container.assignments.AttackResult; import org.owasp.webgoat.container.session.WebSession; import org.springframework.beans.factory.annotation.Value; import org.springframework.http.ResponseEntity; import org.springframework.util.FileCopyUtils; -import org.springframework.web.bind.annotation.*; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.PostMapping; +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.multipart.MultipartFile; @RestController @@ -29,6 +35,7 @@ import org.springframework.web.multipart.MultipartFile; "path-traversal-zip-slip.hint3", "path-traversal-zip-slip.hint4" }) +@Slf4j public class ProfileZipSlip extends ProfileUploadBase { public ProfileZipSlip( @@ -52,12 +59,9 @@ public class ProfileZipSlip extends ProfileUploadBase { @SneakyThrows private AttackResult processZipUpload(MultipartFile file) { var tmpZipDirectory = Files.createTempDirectory(getWebSession().getUserName()); - var uploadDirectory = - new File(getWebGoatHomeDirectory(), "/PathTraversal/" + getWebSession().getUserName()); + cleanupAndCreateDirectoryForUser(); var currentImage = getProfilePictureAsBase64(); - Files.createDirectories(uploadDirectory.toPath()); - try { var uploadedZipFile = tmpZipDirectory.resolve(file.getOriginalFilename()); FileCopyUtils.copy(file.getBytes(), uploadedZipFile.toFile()); diff --git a/src/main/resources/lessons/pathtraversal/documentation/PathTraversal_zip_slip_assignment.adoc b/src/main/resources/lessons/pathtraversal/documentation/PathTraversal_zip_slip_assignment.adoc index b96dc4d9e..2e3b70053 100644 --- a/src/main/resources/lessons/pathtraversal/documentation/PathTraversal_zip_slip_assignment.adoc +++ b/src/main/resources/lessons/pathtraversal/documentation/PathTraversal_zip_slip_assignment.adoc @@ -2,11 +2,13 @@ This time the developers only allow you to upload zip files. However, they made a programming mistake in uploading the zip file will extract it, but it will not replace your image. Can you find a way to overwrite your current image bypassing the programming mistake? +To make the assignment a bit easier below you will find the location of the profile image you need to replace: + |=== |OS |Location |`operatingSystem:os[]` -|`webGoatTempDir:temppath[]PathTraversal` +|`webGoatTempDir:temppath[]PathTraversal/username:user[]/username:user[].jpg` |=== diff --git a/src/main/resources/lessons/pathtraversal/html/PathTraversal.html b/src/main/resources/lessons/pathtraversal/html/PathTraversal.html index fd19a7495..5f716f71a 100644 --- a/src/main/resources/lessons/pathtraversal/html/PathTraversal.html +++ b/src/main/resources/lessons/pathtraversal/html/PathTraversal.html @@ -229,8 +229,6 @@ enctype="multipart/form-data" action="/WebGoat/PathTraversal/zip-slip">
- Preview Image