Jenkins allows Deserialization of Untrusted Data via an XML File

@@ -159,6 +159,8 @@ private void init() {
         // but before reflection-based one kicks in.
         registerConverter(new AssociatedConverterImpl(this), -10);
  
+        registerConverter(new BlacklistedTypesConverter(), PRIORITY_VERY_HIGH); // SECURITY-247 defense
+ 
         registerConverter(new DynamicProxyConverter(getMapper()) { // SECURITY-105 defense
             @Override public boolean canConvert(Class type) {
                 return /* this precedes NullConverter */ type != null && super.canConvert(type);
@@ -434,4 +436,20 @@ class PluginClassOwnership implements ClassOwnership {
  
     }
  
+    private static class BlacklistedTypesConverter implements Converter {
+        @Override
+        public void marshal(Object source, HierarchicalStreamWriter writer, MarshallingContext context) {
+            throw new UnsupportedOperationException("Cannot marshal MethodClosure");
+        }
+ 
+        @Override
+        public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext context) {
+            throw new ConversionException("Cannot load MethodClosure for security reasons");
+        }
+ 
+        @Override
+        public boolean canConvert(Class type) {
+            return type != null && "org.codehaus.groovy.runtime.MethodClosure".equals(type.getName());
+        }
+    }
 }

@@ -0,0 +1,43 @@
+package hudson.util;
+ 
+import hudson.Functions;
+import hudson.model.Items;
+import org.apache.commons.io.FileUtils;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+ 
+import java.io.File;
+ 
+import static org.junit.Assert.assertFalse;
+ 
+public class XStream2Security247Test {
+ 
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+ 
+    @Test
+    @Issue("SECURITY-247")
+    public void dontUnmarshalMethodClosure() throws Exception {
+        if (Functions.isWindows())  return;
+        File exploitFile = new File("/tmp/jenkins-security247test");
+        try {
+            // be extra sure there's no file already
+            if (exploitFile.exists() && !exploitFile.delete()) {
+                throw new IllegalStateException("file exists and cannot be deleted");
+            }
+            File tempJobDir = new File(j.jenkins.getRootDir(), "security247");
+            FileUtils.copyInputStreamToFile(XStream2Security247Test.class.getResourceAsStream("/hudson/util/XStream2Security247Test/config.xml"),
+                    new File(tempJobDir, "config.xml"));
+            try {
+                Items.load(j.jenkins, tempJobDir);
+            } catch (Exception e) {
+                // ignore
+            }
+            assertFalse("no file should be created here", exploitFile.exists());
+        } finally {
+            exploitFile.delete();
+        }
+    }
+}

@@ -0,0 +1,27 @@
+<map>
+    <entry>
+        <groovy.util.Expando>
+            <expandoProperties>
+                <entry>
+                    <string>hashCode</string>
+                    <org.codehaus.groovy.runtime.MethodClosure>
+                        <delegate class="groovy.util.Expando" reference="../../../.."/>
+                        <owner class="java.lang.ProcessBuilder">
+                            <command>
+                                <string>touch</string>
+                                <string>/tmp/jenkins-security247test</string>
+                            </command>
+                            <redirectErrorStream>false</redirectErrorStream>
+                        </owner>
+                        <resolveStrategy>0</resolveStrategy>
+                        <directive>0</directive>
+                        <parameterTypes/>
+                        <maximumNumberOfParameters>0</maximumNumberOfParameters>
+                        <method>start</method>
+                    </org.codehaus.groovy.runtime.MethodClosure>
+                </entry>
+            </expandoProperties>
+        </groovy.util.Expando>
+        <int>1</int>
+    </entry>
+</map>
 test
 .../hudson/util/XStream2Security247Test.java  | 91 +++++++++++++++++--
 .../util/XStream2Security247Test/config.xml   |  2 +-
 2 files changed, 85 insertions(+), 8 deletions(-)

@@ -1,35 +1,65 @@
 package hudson.util;
  
-import hudson.Functions;
 import hudson.model.Items;
-import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.*;
+import org.apache.commons.io.IOUtils;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.kohsuke.stapler.StaplerRequest;
+import org.kohsuke.stapler.StaplerResponse;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
  
+import javax.servlet.ServletInputStream;
 import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
  
 import static org.junit.Assert.assertFalse;
+import static org.mockito.Mockito.when;
  
 public class XStream2Security247Test {
  
     @Rule
     public JenkinsRule j = new JenkinsRule();
  
+    @Rule
+    public TemporaryFolder f = new TemporaryFolder();
+ 
+    @Mock
+    private StaplerRequest req;
+ 
+    @Mock
+    private StaplerResponse rsp;
+ 
+    @Before
+    public void setUp() throws Exception {
+        MockitoAnnotations.initMocks(this);
+    }
+ 
     @Test
     @Issue("SECURITY-247")
-    public void dontUnmarshalMethodClosure() throws Exception {
-        if (Functions.isWindows())  return;
-        File exploitFile = new File("/tmp/jenkins-security247test");
+    public void testXmlLoad() throws Exception {
+        File exploitFile = f.newFile();
         try {
             // be extra sure there's no file already
             if (exploitFile.exists() && !exploitFile.delete()) {
                 throw new IllegalStateException("file exists and cannot be deleted");
             }
             File tempJobDir = new File(j.jenkins.getRootDir(), "security247");
-            FileUtils.copyInputStreamToFile(XStream2Security247Test.class.getResourceAsStream("/hudson/util/XStream2Security247Test/config.xml"),
-                    new File(tempJobDir, "config.xml"));
+ 
+            String exploitXml = org.apache.commons.io.IOUtils.toString(
+                    XStream2Security247Test.class.getResourceAsStream(
+                            "/hudson/util/XStream2Security247Test/config.xml"), "UTF-8");
+ 
+            exploitXml = exploitXml.replace("@TOKEN@", exploitFile.getAbsolutePath());
+ 
+            FileUtils.write(new File(tempJobDir, "config.xml"), exploitXml);
+ 
             try {
                 Items.load(j.jenkins, tempJobDir);
             } catch (Exception e) {
@@ -40,4 +70,51 @@ public void dontUnmarshalMethodClosure() throws Exception {
             exploitFile.delete();
         }
     }
+ 
+    @Test
+    @Issue("SECURITY-247")
+    public void testPostJobXml() throws Exception {
+        File exploitFile = f.newFile();
+        try {
+            // be extra sure there's no file already
+            if (exploitFile.exists() && !exploitFile.delete()) {
+                throw new IllegalStateException("file exists and cannot be deleted");
+            }
+            File tempJobDir = new File(j.jenkins.getRootDir(), "security247");
+ 
+            String exploitXml = org.apache.commons.io.IOUtils.toString(
+                    XStream2Security247Test.class.getResourceAsStream(
+                            "/hudson/util/XStream2Security247Test/config.xml"), "UTF-8");
+ 
+            exploitXml = exploitXml.replace("@TOKEN@", exploitFile.getAbsolutePath());
+ 
+            when(req.getMethod()).thenReturn("POST");
+            when(req.getInputStream()).thenReturn(new Stream(IOUtils.toInputStream(exploitXml)));
+            when(req.getContentType()).thenReturn("application/xml");
+            when(req.getParameter("name")).thenReturn("foo");
+ 
+            try {
+                j.jenkins.doCreateItem(req, rsp);
+            } catch (Exception e) {
+                // don't care
+            }
+ 
+            assertFalse("no file should be created here", exploitFile.exists());
+        } finally {
+            exploitFile.delete();
+        }
+    }
+ 
+    private static class Stream extends ServletInputStream {
+        private final InputStream inner;
+ 
+        public Stream(final InputStream inner) {
+            this.inner = inner;
+        }
+ 
+        @Override
+        public int read() throws IOException {
+            return inner.read();
+        }
+    }
 }

@@ -9,7 +9,7 @@
                         <owner class="java.lang.ProcessBuilder">
                             <command>
                                 <string>touch</string>
-                                <string>/tmp/jenkins-security247test</string>
+                                <string>@TOKEN@</string>
                             </command>
                             <redirectErrorStream>false</redirectErrorStream>
                         </owner>
 coverage
 core/src/main/java/hudson/util/XStream2.java | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

@@ -47,6 +47,7 @@
 import hudson.PluginManager;
 import hudson.PluginWrapper;
 import hudson.diagnosis.OldDataMonitor;
+import hudson.remoting.ClassFilter;
 import hudson.util.xstream.ImmutableSetConverter;
 import hudson.util.xstream.ImmutableSortedSetConverter;
 import jenkins.model.Jenkins;
@@ -439,17 +440,26 @@ class PluginClassOwnership implements ClassOwnership {
     private static class BlacklistedTypesConverter implements Converter {
         @Override
         public void marshal(Object source, HierarchicalStreamWriter writer, MarshallingContext context) {
-            throw new UnsupportedOperationException("Cannot marshal MethodClosure");
+            throw new UnsupportedOperationException("Refusing to marshal for security reasons");
         }
  
         @Override
         public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext context) {
-            throw new ConversionException("Cannot load MethodClosure for security reasons");
+            throw new ConversionException("Refusing to unmarshal for security reasons");
         }
  
         @Override
         public boolean canConvert(Class type) {
-            return type != null && "org.codehaus.groovy.runtime.MethodClosure".equals(type.getName());
+            if (type == null) {
+                return false;
+            }
+            try {
+                ClassFilter.DEFAULT.check(type.getName());
+            } catch (SecurityException se) {
+                // claim we can convert all the scary stuff so we can throw exceptions when attempting to do so
+                return true;
+            }
+            return false;
         }
     }
 }
 core/src/main/java/hudson/util/XStream2.java | 1 +
 1 file changed, 1 insertion(+)

@@ -454,6 +454,7 @@ public boolean canConvert(Class type) {
                 return false;
             }
             try {
+                ClassFilter.DEFAULT.check(type);
                 ClassFilter.DEFAULT.check(type.getName());
             } catch (SecurityException se) {
                 // claim we can convert all the scary stuff so we can throw exceptions when attempting to do so