deserialization made solvable again (#673)

* first objects and unit tests for making a fix for the lesson

* example added

* unit test for windows and linux

* added unit tests hints and feedbacks and updated lesson pages

* small typo correction

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

@@ -0,0 +1,34 @@
+package org.owasp.webgoat;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.dummy.insecure.framework.VulnerableTaskHolder;
+import org.junit.Test;
+import org.owasp.webgoat.deserialization.SerializationHelper;
+
+public class DeserializationTest extends IntegrationTest {
+
+	private static String OS = System.getProperty("os.name").toLowerCase();
+    
+    @Test
+    public void runTests() throws IOException {
+        startLesson("InsecureDeserialization");
+        
+        Map<String, Object> params = new HashMap<>();
+        params.clear();
+                
+        if (OS.indexOf("win")>-1) {
+        	params.put("token", SerializationHelper.toString(new VulnerableTaskHolder("wait", "ping localhost -n 5")));
+        } else {
+            params.put("token", SerializationHelper.toString(new VulnerableTaskHolder("wait", "sleep 5")));
+        }
+        checkAssignment(url("/WebGoat/InsecureDeserialization/task"),params,true);
+        
+        checkResults("/InsecureDeserialization/");
+    
+    }
+    
+    
+}

webgoat-lessons/insecure-deserialization/src/main/java/org/dummy/insecure/framework/VulnerableTaskHolder.java

@@ -0,0 +1,70 @@
+package org.dummy.insecure.framework;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.io.ObjectInputStream;
+import java.io.Serializable;
+import java.time.LocalDateTime;
+
+public class VulnerableTaskHolder implements Serializable {
+
+	private static final long serialVersionUID = 2;
+
+	private String taskName;
+	private String taskAction;
+	private LocalDateTime requestedExecutionTime;
+	
+	public VulnerableTaskHolder(String taskName, String taskAction) {
+		super();
+		this.taskName = taskName;
+		this.taskAction = taskAction;
+		this.requestedExecutionTime = LocalDateTime.now();
+	}
+	
+	@Override
+	public String toString() {
+		return "VulnerableTaskHolder [taskName=" + taskName + ", taskAction=" + taskAction + ", requestedExecutionTime="
+				+ requestedExecutionTime + "]";
+	}
+
+	/**
+	 * Execute a task when de-serializing a saved or received object.
+	 * @author stupid develop
+	 */
+	private void readObject( ObjectInputStream stream ) throws Exception {
+        //unserialize data so taskName and taskAction are available
+		stream.defaultReadObject();
+		
+		//do something with the data
+		System.out.println("restoring task: "+taskName);
+		System.out.println("restoring time: "+requestedExecutionTime);
+		
+		if (requestedExecutionTime!=null && 
+				(requestedExecutionTime.isBefore(LocalDateTime.now().minusMinutes(10))
+				|| requestedExecutionTime.isAfter(LocalDateTime.now()))) {
+			//do nothing is the time is not within 10 minutes after the object has been created
+			System.out.println(this.toString());
+			throw new IllegalArgumentException("outdated");
+		}
+		
+		//condition is here to prevent you from destroying the goat altogether
+		if ((taskAction.startsWith("sleep")||taskAction.startsWith("ping"))
+				&& taskAction.length() < 22) {
+		System.out.println("about to execute: "+taskAction);
+		try {
+            Process p = Runtime.getRuntime().exec(taskAction);
+            BufferedReader in = new BufferedReader(
+                                new InputStreamReader(p.getInputStream()));
+            String line = null;
+            while ((line = in.readLine()) != null) {
+                System.out.println(line);
+            }
+        } catch (IOException e) {
+            e.printStackTrace();
+        }
+		}
+       
+    }
+	
+}

webgoat-lessons/insecure-deserialization/src/main/java/org/owasp/webgoat/deserialization/InsecureDeserializationTask.java

@@ -22,7 +22,9 @@
 
 package org.owasp.webgoat.deserialization;
 
+import org.dummy.insecure.framework.VulnerableTaskHolder;
 import org.owasp.webgoat.assignments.AssignmentEndpoint;
+import org.owasp.webgoat.assignments.AssignmentHints;
 import org.owasp.webgoat.assignments.AttackResult;
 import org.springframework.web.bind.annotation.PostMapping;
 import org.springframework.web.bind.annotation.RequestParam;
@@ -31,39 +33,41 @@ import org.springframework.web.bind.annotation.RestController;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.io.InvalidClassException;
 import java.io.ObjectInputStream;
 import java.util.Base64;
 
 @RestController
+@AssignmentHints({"insecure-deserialization.hints.1","insecure-deserialization.hints.2","insecure-deserialization.hints.3"})
 public class InsecureDeserializationTask extends AssignmentEndpoint {
 
     @PostMapping("/InsecureDeserialization/task")
     @ResponseBody
     public AttackResult completed(@RequestParam String token) throws IOException {
         String b64token;
-        byte[] data;
-        ObjectInputStream ois;
-        Object o;
         long before, after;
         int delay;
 
         b64token = token.replace('-', '+').replace('_', '/');
-        try {
+        
-            data = Base64.getDecoder().decode(b64token);
+        try (ObjectInputStream ois = new ObjectInputStream(new ByteArrayInputStream(Base64.getDecoder().decode(b64token)))) {
-            ois = new ObjectInputStream(new ByteArrayInputStream(data));
+        	before = System.currentTimeMillis();
+        	Object o = ois.readObject();
+        	if (!(o instanceof VulnerableTaskHolder)) {
+        		if (o instanceof String) {
+        			return trackProgress(failed().feedback("insecure-deserialization.stringobject").build());
+        		}
+        		return trackProgress(failed().feedback("insecure-deserialization.wrongobject").build());
+        	}
+            after = System.currentTimeMillis();
+        } catch (InvalidClassException e) {
+            return trackProgress(failed().feedback("insecure-deserialization.invalidversion").build());
+        } catch (IllegalArgumentException e) {
+            return trackProgress(failed().feedback("insecure-deserialization.expired").build());
         } catch (Exception e) {
-            return trackProgress(failed().build());
+            return trackProgress(failed().feedback("insecure-deserialization.invalidversion").build());
         }
 
-        before = System.currentTimeMillis();
-        try {
-            o = ois.readObject();
-        } catch (Exception e) {
-            o = null;
-        }
-        after = System.currentTimeMillis();
-        ois.close();
-
         delay = (int) (after - before);
         if (delay > 7000) {
             return trackProgress(failed().build());

webgoat-lessons/insecure-deserialization/src/main/java/org/owasp/webgoat/deserialization/SerializationHelper.java

@@ -0,0 +1,54 @@
+package org.owasp.webgoat.deserialization;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.DataOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.Base64;
+
+public class SerializationHelper {
+	
+	private final static char[] hexArray = "0123456789ABCDEF".toCharArray();
+	
+    public static Object fromString( String s ) throws IOException ,
+            ClassNotFoundException {
+        byte [] data = Base64.getDecoder().decode( s );
+        ObjectInputStream ois = new ObjectInputStream(
+                new ByteArrayInputStream(  data ) );
+        Object o  = ois.readObject();
+        ois.close();
+        return o;
+    }
+
+    public static String toString( Serializable o ) throws IOException {
+
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        ObjectOutputStream oos = new ObjectOutputStream( baos );
+        oos.writeObject( o );
+        oos.close();
+        return Base64.getEncoder().encodeToString(baos.toByteArray());
+    }
+
+    public static String show() throws IOException {
+    	ByteArrayOutputStream baos = new ByteArrayOutputStream();
+    	DataOutputStream dos = new DataOutputStream(baos);
+    	dos.writeLong(-8699352886133051976L);
+    	dos.close();
+    	byte[] longBytes = baos.toByteArray();
+    	return bytesToHex(longBytes);
+    }
+    
+    public static String bytesToHex(byte[] bytes) {
+        char[] hexChars = new char[bytes.length * 2];
+        for ( int j = 0; j < bytes.length; j++ ) {
+            int v = bytes[j] & 0xFF;
+            hexChars[j * 2] = hexArray[v >>> 4];
+            hexChars[j * 2 + 1] = hexArray[v & 0x0F];
+        }
+        return new String(hexChars);
+    }
+
+}

webgoat-lessons/insecure-deserialization/src/main/resources/i18n/WebGoatLabels.properties

@@ -1,4 +1,14 @@
 insecure-deserialization.title=Insecure Deserialization
 
 insecure-deserialization.intercept.success=Dangerous object received!
-insecure-deserialization.intercept.failure=Try again
+insecure-deserialization.intercept.failure=Try again
+
+insecure-deserialization.invalidversion=The serialization id does not match. Probably the version has been updated. Let's try again.
+insecure-deserialization.expired=The task is not executable between now and the next ten minutes, so the action will be ignored. Maybe you copied an old solution? Let's try again.
+insecure-deserialization.wrongobject=That is not the VulnerableTaskHolder object. Good try! because the code is not checking this after running the readObject(). Let's try again with the right object.
+insecure-deserialization.stringobject=That is not the VulnerableTaskHolder object. However a plain String is harmless. Let's try again with the right object.
+
+
+insecure-deserialization.hints.1=WebGoat probably contains the org.dummy.insecure.framework.VulnerableTaskHolder class as shown on the lesson pages. Use this to construct and serialize your attack.
+insecure-deserialization.hints.2=The VulnerableTaskHolder might have been updated on the server with a next version number.
+insecure-deserialization.hints.3=Not all actions are allowed anymore. The readObject has been changed. For serializing it does not effect the data. Follow the additional hints from the feedback on your attempts.

webgoat-lessons/insecure-deserialization/src/main/resources/lessonPlans/en/InsecureDeserialization_SimpleExploit.adoc

@@ -20,12 +20,37 @@ Attackers need to find a class in the classpath that supports serialization and
 
 [source,java]
 ----
-public class GadgetObject implements Serializable {
+package org.dummy.insecure.framework;
-    String cmd;
 
-    private void readObject( ObjectInputStream stream ) throws Exception {
+import java.io.BufferedReader;
-        Runtime.getRuntime().exec(cmd);
+import java.io.IOException;
-    }
+import java.io.InputStreamReader;
+import java.io.ObjectInputStream;
+import java.io.Serializable;
+import java.time.LocalDateTime;
+
+public class VulnerableTaskHolder implements Serializable {
+
+	private static final long serialVersionUID = 1;
+
+	private String taskName;
+	private String taskAction;
+	private LocalDateTime requestedExecutionTime;
+	
+	public VulnerableTaskHolder(String taskName, String taskAction) {
+		super();
+		this.taskName = taskName;
+		this.taskAction = taskAction;
+		this.requestedExecutionTime = LocalDateTime.now();
+	}
+
+	private void readObject( ObjectInputStream stream ) throws Exception {
+        //deserialize data so taskName and taskAction are available
+		stream.defaultReadObject();
+		
+		//blindly run some code. #code injection
+		Runtime.getRuntime().exec(taskAction);
+     }
 }
 ----
 
@@ -35,8 +60,7 @@ If the java class shown above exists, attackers can serialize that object and ob
 
 [source,java]
 ----
-GadgetObject go = new GadgetObject();
+VulnerableTaskHolder go = new VulnerableTaskHolder("delete all", "rm -rf somefile");
-go.cmd = "touch /tmp/pwned.txt";
 
 ByteArrayOutputStream bos = new ByteArrayOutputStream();
 ObjectOutputStream oos = new ObjectOutputStream(bos);

webgoat-lessons/insecure-deserialization/src/test/java/org/owasp/webgoat/deserialization/DeserializeTest.java

@@ -0,0 +1,94 @@
+package org.owasp.webgoat.deserialization;
+
+import static org.hamcrest.Matchers.is;
+import static org.mockito.Mockito.when;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
+import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup;
+
+import org.dummy.insecure.framework.VulnerableTaskHolder;
+import org.hamcrest.CoreMatchers;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.junit.MockitoJUnitRunner;
+import org.owasp.webgoat.assignments.AssignmentEndpointTest;
+import org.springframework.test.web.servlet.MockMvc;
+import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;
+
+@RunWith(MockitoJUnitRunner.class)
+public class DeserializeTest extends AssignmentEndpointTest {
+
+	private MockMvc mockMvc;
+	
+	private static String OS = System.getProperty("os.name").toLowerCase();
+	
+	@Before
+    public void setup() {
+        InsecureDeserializationTask insecureTask = new InsecureDeserializationTask();
+        init(insecureTask);
+        this.mockMvc = standaloneSetup(insecureTask).build();
+        when(webSession.getCurrentLesson()).thenReturn(new InsecureDeserialization());
+    }
+
+    @Test
+    public void success() throws Exception {
+    	if (OS.indexOf("win")>-1) {
+    		mockMvc.perform(MockMvcRequestBuilders.post("/InsecureDeserialization/task")
+                    .header("x-request-intercepted", "true")
+                    .param("token", SerializationHelper.toString(new VulnerableTaskHolder("wait", "ping localhost -n 5"))))
+            		.andExpect(status().isOk()).andExpect(jsonPath("$.lessonCompleted", is(true)));
+    	} else {
+    		mockMvc.perform(MockMvcRequestBuilders.post("/InsecureDeserialization/task")
+                .header("x-request-intercepted", "true")
+                .param("token", SerializationHelper.toString(new VulnerableTaskHolder("wait", "sleep 5"))))
+        		.andExpect(status().isOk()).andExpect(jsonPath("$.lessonCompleted", is(true)));
+    	}
+    }
+    
+    @Test
+    public void fail() throws Exception {
+        mockMvc.perform(MockMvcRequestBuilders.post("/InsecureDeserialization/task")
+                .header("x-request-intercepted", "true")
+                .param("token", SerializationHelper.toString(new VulnerableTaskHolder("delete", "rm *"))))
+        		.andExpect(status().isOk()).andExpect(jsonPath("$.lessonCompleted", is(false)));
+    }
+    
+    @Test
+    public void wrongVersion() throws Exception {
+    	String token = "rO0ABXNyADFvcmcuZHVtbXkuaW5zZWN1cmUuZnJhbWV3b3JrLlZ1bG5lcmFibGVUYXNrSG9sZGVyAAAAAAAAAAECAANMABZyZXF1ZXN0ZWRFeGVjdXRpb25UaW1ldAAZTGphdmEvdGltZS9Mb2NhbERhdGVUaW1lO0wACnRhc2tBY3Rpb250ABJMamF2YS9sYW5nL1N0cmluZztMAAh0YXNrTmFtZXEAfgACeHBzcgANamF2YS50aW1lLlNlcpVdhLobIkiyDAAAeHB3DgUAAAfjCR4GIQgMLRSoeHQACmVjaG8gaGVsbG90AAhzYXlIZWxsbw";
+        mockMvc.perform(MockMvcRequestBuilders.post("/InsecureDeserialization/task")
+                .header("x-request-intercepted", "true")
+                .param("token", token))
+        		.andExpect(status().isOk())
+                .andExpect(jsonPath("$.feedback", CoreMatchers.is(messages.getMessage("insecure-deserialization.invalidversion"))))
+        		.andExpect(jsonPath("$.lessonCompleted", is(false)));
+    }
+    
+    @Test
+    public void expiredTask() throws Exception {
+    	String token = "rO0ABXNyADFvcmcuZHVtbXkuaW5zZWN1cmUuZnJhbWV3b3JrLlZ1bG5lcmFibGVUYXNrSG9sZGVyAAAAAAAAAAICAANMABZyZXF1ZXN0ZWRFeGVjdXRpb25UaW1ldAAZTGphdmEvdGltZS9Mb2NhbERhdGVUaW1lO0wACnRhc2tBY3Rpb250ABJMamF2YS9sYW5nL1N0cmluZztMAAh0YXNrTmFtZXEAfgACeHBzcgANamF2YS50aW1lLlNlcpVdhLobIkiyDAAAeHB3DgUAAAfjCR4IDC0YfvNIeHQACmVjaG8gaGVsbG90AAhzYXlIZWxsbw";
+        mockMvc.perform(MockMvcRequestBuilders.post("/InsecureDeserialization/task")
+                .header("x-request-intercepted", "true")
+                .param("token", token))
+        		.andExpect(status().isOk())
+                .andExpect(jsonPath("$.feedback", CoreMatchers.is(messages.getMessage("insecure-deserialization.expired"))))
+        		.andExpect(jsonPath("$.lessonCompleted", is(false)));
+    }
+    
+
+    
+    @Test
+    public void checkOtherObject() throws Exception {
+    	String token = "rO0ABXQAVklmIHlvdSBkZXNlcmlhbGl6ZSBtZSBkb3duLCBJIHNoYWxsIGJlY29tZSBtb3JlIHBvd2VyZnVsIHRoYW4geW91IGNhbiBwb3NzaWJseSBpbWFnaW5l";
+    	mockMvc.perform(MockMvcRequestBuilders.post("/InsecureDeserialization/task")
+                .header("x-request-intercepted", "true")
+                .param("token", token))
+        		.andExpect(status().isOk())
+                .andExpect(jsonPath("$.feedback", CoreMatchers.is(messages.getMessage("insecure-deserialization.stringobject"))))
+        		.andExpect(jsonPath("$.lessonCompleted", is(false)));
+    }
+    
+    
+
+}

webgoat-lessons/insecure-deserialization/src/test/resources/logback-test.xml

@@ -0,0 +1,16 @@
+<configuration>
+
+  <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
+    <!-- encoders are assigned the type
+         ch.qos.logback.classic.encoder.PatternLayoutEncoder by default -->
+    <encoder>
+      <pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</pattern>
+    </encoder>
+  </appender>
+  
+  <logger name="org.owasp.webgoat.plugin" level="INFO"/>
+
+  <root level="ERROR">
+    <appender-ref ref="STDOUT" />
+  </root>
+</configuration>