initial idea for explanation on static code analysis and experience of the fix

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, "<comment><text>" + getSecret() + "</text></comment>", 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;
     }
 }

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");
             }

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();

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();

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();
@@ -101,4 +110,15 @@ public class SimpleXXE extends AssignmentEndpoint {
                 + "%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";
+    }
+
 }

webgoat-lessons/xxe/src/main/resources/html/XXE.html

@@ -216,5 +216,10 @@
     <div class="adoc-content" th:replace="doc:XXE_mitigation.adoc"></div>
 </div>
 
+<div class="lesson-page-wrapper">
+    <div class="adoc-content" th:replace="doc:XXE_static_code_analysis.adoc"></div>
+    <a href="/WebGoat/xxe/applysecurity" onclick="javascript:return false;">Apply XXE security patch</a>
+</div>
+
 
 </html>

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.
+
+
+
+