diff --git a/webgoat-integration-tests/src/test/java/org/owasp/webgoat/XXETest.java b/webgoat-integration-tests/src/test/java/org/owasp/webgoat/XXETest.java index 3b9ec371b..f06481591 100644 --- a/webgoat-integration-tests/src/test/java/org/owasp/webgoat/XXETest.java +++ b/webgoat-integration-tests/src/test/java/org/owasp/webgoat/XXETest.java @@ -31,6 +31,21 @@ public class XXETest extends IntegrationTest { checkResults("xxe/"); } + /* + * This test is to verify that all is secure when XXE security patch is applied. + */ + @Test + public void xxeSecure() throws IOException { + startLesson("XXE"); + webGoatHomeDirectory = getWebGoatServerPath(); + webwolfFileDir = getWebWolfServerPath(); + RestAssured.given().when().relaxedHTTPSValidation() + .cookie("JSESSIONID", getWebGoatCookie()).get(url("/xxe/applysecurity")); + checkAssignment(url("/WebGoat/xxe/simple"), ContentType.XML, xxe3, false); + checkAssignment(url("/WebGoat/xxe/content-type"), ContentType.XML, xxe4, false); + checkAssignment(url("/WebGoat/xxe/blind"), ContentType.XML, "" + getSecret() + "", false); + } + /** * This performs the steps of the exercise before the secret can be committed in the final step. * @@ -68,7 +83,9 @@ public class XXETest extends IntegrationTest { .then() .extract().response().getBody().asString(); result = result.replace("%20", " "); - result = result.substring(result.lastIndexOf("WebGoat 8.0 rocks... ("), result.lastIndexOf("WebGoat 8.0 rocks... (") + 33); + if (-1 != result.lastIndexOf("WebGoat 8.0 rocks... (")) { + result = result.substring(result.lastIndexOf("WebGoat 8.0 rocks... ("), result.lastIndexOf("WebGoat 8.0 rocks... (") + 33); + } return result; } } diff --git a/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/BlindSendFileAssignment.java b/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/BlindSendFileAssignment.java index ee5833961..a5824e65e 100644 --- a/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/BlindSendFileAssignment.java +++ b/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/BlindSendFileAssignment.java @@ -13,6 +13,8 @@ import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.bind.annotation.RestController; import javax.annotation.PostConstruct; +import javax.servlet.http.HttpServletRequest; + import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; @@ -75,14 +77,18 @@ public class BlindSendFileAssignment extends AssignmentEndpoint { @PostMapping(path = "xxe/blind", consumes = MediaType.ALL_VALUE, produces = MediaType.APPLICATION_JSON_VALUE) @ResponseBody - public AttackResult addComment(@RequestBody String commentStr) { + public AttackResult addComment(HttpServletRequest request, @RequestBody String commentStr) { //Solution is posted as a separate comment if (commentStr.contains(CONTENTS)) { return success(this).build(); } try { - Comment comment = comments.parseXml(commentStr); + boolean secure = false; + if (null != request.getSession().getAttribute("applySecurity")) { + secure = true; + } + Comment comment = comments.parseXml(commentStr, secure); if (CONTENTS.contains(comment.getText())) { comment.setText("Nice try, you need to send the file to WebWolf"); } diff --git a/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/Comments.java b/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/Comments.java index 5daea1688..ae5e2315f 100644 --- a/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/Comments.java +++ b/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/Comments.java @@ -32,6 +32,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Component; +import javax.xml.XMLConstants; import javax.xml.bind.JAXBContext; import javax.xml.bind.JAXBException; import javax.xml.bind.Unmarshaller; @@ -84,9 +85,15 @@ public class Comments { * 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) throws JAXBException, XMLStreamException { + protected Comment parseXml(String xml, boolean secure) throws JAXBException, XMLStreamException { var jc = JAXBContext.newInstance(Comment.class); var xif = XMLInputFactory.newInstance(); + + if (secure) { + xif.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); // Compliant + xif.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); // compliant + } + var xsr = xif.createXMLStreamReader(new StringReader(xml)); var unmarshaller = jc.createUnmarshaller(); diff --git a/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/ContentTypeAssignment.java b/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/ContentTypeAssignment.java index 91dc8c5f0..91d63c7a6 100644 --- a/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/ContentTypeAssignment.java +++ b/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/ContentTypeAssignment.java @@ -34,6 +34,8 @@ import org.springframework.web.bind.annotation.*; import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; +import javax.servlet.http.HttpServletRequest; + @RestController @AssignmentHints({"xxe.hints.content.type.xxe.1", "xxe.hints.content.type.xxe.2"}) public class ContentTypeAssignment extends AssignmentEndpoint { @@ -50,7 +52,7 @@ public class ContentTypeAssignment extends AssignmentEndpoint { @PostMapping(path = "xxe/content-type") @ResponseBody - public AttackResult createNewUser(@RequestBody String commentStr, @RequestHeader("Content-Type") String contentType) throws Exception { + public AttackResult createNewUser(HttpServletRequest request, @RequestBody String commentStr, @RequestHeader("Content-Type") String contentType) throws Exception { AttackResult attackResult = failed(this).build(); if (APPLICATION_JSON_VALUE.equals(contentType)) { @@ -61,7 +63,11 @@ public class ContentTypeAssignment extends AssignmentEndpoint { if (null != contentType && contentType.contains(MediaType.APPLICATION_XML_VALUE)) { String error = ""; try { - Comment comment = comments.parseXml(commentStr); + boolean secure = false; + if (null != request.getSession().getAttribute("applySecurity")) { + secure = true; + } + Comment comment = comments.parseXml(commentStr, secure); comments.addComment(comment, false); if (checkSolution(comment)) { attackResult = success(this).build(); diff --git a/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/SimpleXXE.java b/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/SimpleXXE.java index 83ce5a4e4..fe5176351 100644 --- a/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/SimpleXXE.java +++ b/webgoat-lessons/xxe/src/main/java/org/owasp/webgoat/xxe/SimpleXXE.java @@ -30,6 +30,7 @@ import org.owasp.webgoat.assignments.AttackResult; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.http.MediaType; +import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; @@ -39,6 +40,10 @@ import org.springframework.web.bind.annotation.RestController; import static org.springframework.http.MediaType.ALL_VALUE; import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; +import java.util.Random; + +import javax.servlet.http.HttpServletRequest; + /** * @author nbaars @@ -63,10 +68,14 @@ public class SimpleXXE extends AssignmentEndpoint { @PostMapping(path = "xxe/simple", consumes = ALL_VALUE, produces = APPLICATION_JSON_VALUE) @ResponseBody - public AttackResult createNewComment(@RequestBody String commentStr) throws Exception { + public AttackResult createNewComment(HttpServletRequest request, @RequestBody String commentStr) throws Exception { String error = ""; try { - Comment comment = comments.parseXml(commentStr); + boolean secure = false; + if (null != request.getSession().getAttribute("applySecurity")) { + secure = true; + } + Comment comment = comments.parseXml(commentStr, secure); comments.addComment(comment, false); if (checkSolution(comment)) { return success(this).build(); @@ -100,5 +109,16 @@ public class SimpleXXE extends AssignmentEndpoint { + "\">\n" + "%all;"; } + + @GetMapping(path="/xxe/applysecurity",produces=MediaType.TEXT_PLAIN_VALUE) + @ResponseBody + public String setSecurity(HttpServletRequest request) { + + String applySecurity = (String) request.getSession().getAttribute("applySecurity"); + if (applySecurity == null) { + request.getSession().setAttribute("applySecurity", "true"); + } + return "xxe security will be applied"; + } } diff --git a/webgoat-lessons/xxe/src/main/resources/html/XXE.html b/webgoat-lessons/xxe/src/main/resources/html/XXE.html index 5c05d929d..9c5dff2c3 100644 --- a/webgoat-lessons/xxe/src/main/resources/html/XXE.html +++ b/webgoat-lessons/xxe/src/main/resources/html/XXE.html @@ -216,5 +216,10 @@
+
+
+ Apply XXE security patch +
+ \ No newline at end of file diff --git a/webgoat-lessons/xxe/src/main/resources/images/sonar-issue-xxe.png b/webgoat-lessons/xxe/src/main/resources/images/sonar-issue-xxe.png new file mode 100644 index 000000000..1a72e0afd Binary files /dev/null and b/webgoat-lessons/xxe/src/main/resources/images/sonar-issue-xxe.png differ diff --git a/webgoat-lessons/xxe/src/main/resources/images/sonar-issues.png b/webgoat-lessons/xxe/src/main/resources/images/sonar-issues.png new file mode 100644 index 000000000..fc44281eb Binary files /dev/null and b/webgoat-lessons/xxe/src/main/resources/images/sonar-issues.png differ diff --git a/webgoat-lessons/xxe/src/main/resources/images/xxe-suggested-fix.png b/webgoat-lessons/xxe/src/main/resources/images/xxe-suggested-fix.png new file mode 100644 index 000000000..865df6e5f Binary files /dev/null and b/webgoat-lessons/xxe/src/main/resources/images/xxe-suggested-fix.png differ diff --git a/webgoat-lessons/xxe/src/main/resources/lessonPlans/en/XXE_static_code_analysis.adoc b/webgoat-lessons/xxe/src/main/resources/lessonPlans/en/XXE_static_code_analysis.adoc new file mode 100644 index 000000000..f62bd476d --- /dev/null +++ b/webgoat-lessons/xxe/src/main/resources/lessonPlans/en/XXE_static_code_analysis.adoc @@ -0,0 +1,21 @@ +=== Find XXE issues with static code analysis + +Static code analysis can help identify vulnerabilities in code. A well known tool for static code analysis is SonarQube. When you run a code scan on the source code of WebGoat, you will get something like: + +image::images/sonar-issues.png[Sonar OWASP issues,300,200] +If you select the XXE category it will show you the location of the XXE vulnerability. + +image::images/sonar-issue-xxe.png[XXE issue in Comments class] + + +The next step is to identify whether this is a true issue or a false positive. As you already know from the challenge exercise, this is a real issue. In this case it is put in intentionally. + +SonarQube also shows you what you could do to fix this. + +image::images/xxe-suggested-fix.png[XXE suggested fix] + +If you click on the link below, you can try to do the XXE challenges again and you will notice that the vulnerabilities are mitigated. + + + +