[JENKINS-63343] Validate element types in RobustCollectionConverter in some cases

core/src/main/java/hudson/util/RobustCollectionConverter.java

@@ -34,11 +34,14 @@
 import com.thoughtworks.xstream.io.HierarchicalStreamReader;
 import com.thoughtworks.xstream.mapper.Mapper;
 import com.thoughtworks.xstream.security.InputManipulationException;
+import edu.umd.cs.findbugs.annotations.CheckForNull;
+import java.lang.reflect.Type;
 import java.util.Collection;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.CopyOnWriteArraySet;
 import java.util.logging.Logger;
 import jenkins.util.xstream.CriticalXStreamException;
+import org.jvnet.tiger_types.Types;
 
 /**
  * {@link CollectionConverter} that ignores {@link XStreamException}.
@@ -52,14 +55,33 @@
 @SuppressWarnings({"rawtypes", "unchecked"})
 public class RobustCollectionConverter extends CollectionConverter {
     private final SerializableConverter sc;
+    /**
+     * When available, this field holds the declared type of the collection being (de)serialized.
+     * This is used during deserialization to ensure that elements have the expected type.
+     */
+    // TODO: We only support one level of parameterization, i.e. the elements of a List<List<Integer>>
+    // will be treated as List<Object>, and so the inner list could have invalid non-Integer elements. It is unclear if
+    // we can do better without significant changes.
+    private final @CheckForNull Class<?> elementType;
 
     public RobustCollectionConverter(XStream xs) {
-        this(xs.getMapper(), xs.getReflectionProvider());
+        this(xs.getMapper(), xs.getReflectionProvider(), null);
     }
 
     public RobustCollectionConverter(Mapper mapper, ReflectionProvider reflectionProvider) {
+        this(mapper, reflectionProvider, null);
+    }
+
+    public RobustCollectionConverter(Mapper mapper, ReflectionProvider reflectionProvider, Type collectionType) {
         super(mapper);
         sc = new SerializableConverter(mapper, reflectionProvider, new ClassLoaderReference(null));
+        if (collectionType != null && Collection.class.isAssignableFrom(Types.erasure(collectionType))) {
+            var baseType = Types.getBaseClass(collectionType, Collection.class);
+            var typeArg = Types.getTypeArgument(baseType, 0, Object.class);
+            this.elementType = Types.erasure(typeArg);
+        } else {
+            this.elementType = null;
+        }
     }
 
     @Override
@@ -85,9 +107,17 @@ protected void populateCollection(HierarchicalStreamReader reader, Unmarshalling
             reader.moveDown();
             try {
                 Object item = readBareItem(reader, context, collection);
-                long nanoNow = System.nanoTime();
-                collection.add(item);
-                XStream2SecurityUtils.checkForCollectionDoSAttack(context, nanoNow);
+                try {
+                    if (elementType != null) {
+                        // When possible, disallow invalid elements.
+                        elementType.cast(item);
+                    }
+                    long nanoNow = System.nanoTime();
+                    collection.add(item);
+                    XStream2SecurityUtils.checkForCollectionDoSAttack(context, nanoNow);
+                } catch (ClassCastException e) {
+                    RobustReflectionConverter.addErrorInContext(context, e);
+                }
             } catch (CriticalXStreamException e) {
                 throw e;
             } catch (InputManipulationException e) {

core/src/main/java/hudson/util/RobustReflectionConverter.java

@@ -451,6 +451,10 @@ private boolean fieldDefinedInClass(Object result, String attrName) {
 
     protected Object unmarshalField(final UnmarshallingContext context, final Object result, Class type, Field field) {
         Converter converter = mapper.getLocalConverter(field.getDeclaringClass(), field.getName());
+        // TODO: Is there a better way to make something like this work? Does it break any custom converters?
+        if (converter == null && new RobustCollectionConverter(mapper, reflectionProvider).canConvert(type)) {
+            converter = new RobustCollectionConverter(mapper, reflectionProvider, field.getGenericType());
+        }
         return context.convertAnother(result, type, converter);
     }
 

core/src/test/java/hudson/util/RobustCollectionConverterTest.java

@@ -40,6 +40,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import jenkins.util.xstream.CriticalXStreamException;
 import org.junit.Test;
@@ -173,4 +174,53 @@ private Set<Object> preparePayload() {
         }
         return set;
     }
+
+    @Test
+    public void checkElementTypes() {
+        var expected = new Numbers(1, 2, 3);
+        var xmlContent =
+                """
+                <hudson.util.RobustCollectionConverterTest_-Numbers>
+                  <numbers>
+                    <int>1</int>
+                    <int>2</int>
+                    <string>oops!</string>
+                    <int>3</int>
+                  </numbers>
+                </hudson.util.RobustCollectionConverterTest_-Numbers>
+                """;
+        var actual = (Numbers) new XStream2().fromXML(xmlContent);
+        assertEquals(expected, actual);
+    }
+
+    public static class Numbers {
+        private final List<Integer> numbers;
+
+        public Numbers(Integer... numbers) {
+            this.numbers = new ArrayList(Arrays.asList(numbers));
+        }
+
+        @Override
+        public int hashCode() {
+            int hash = 7;
+            hash = 37 * hash + Objects.hashCode(this.numbers);
+            return hash;
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj) {
+                return true;
+            } else if (obj == null || getClass() != obj.getClass()) {
+                return false;
+            }
+            final Numbers other = (Numbers) obj;
+            return Objects.equals(this.numbers, other.numbers);
+        }
+
+        @Override
+        public String toString() {
+            return "Numbers[" + numbers + "]";
+        }
+    }
 }