[JENKINS-63343] Validate element types for collections and maps when deserializing XML files

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

@@ -26,6 +26,7 @@
 
 import com.thoughtworks.xstream.XStream;
 import com.thoughtworks.xstream.XStreamException;
+import com.thoughtworks.xstream.converters.ConversionException;
 import com.thoughtworks.xstream.converters.UnmarshallingContext;
 import com.thoughtworks.xstream.converters.collections.CollectionConverter;
 import com.thoughtworks.xstream.converters.reflection.ReflectionProvider;
@@ -34,11 +35,15 @@
 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 hudson.diagnosis.OldDataMonitor;
+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 +57,39 @@
 @SuppressWarnings({"rawtypes", "unchecked"})
 public class RobustCollectionConverter extends CollectionConverter {
     private final SerializableConverter sc;
+    /**
+     * When available, this field holds the declared type of the collection being deserialized.
+     */
+    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);
+    }
+
+    /**
+     * Creates a converter that will validate the types of collection elements during deserialization.
+     * <p>Elements with invalid types will be omitted from deserialized collections and may result in an
+     * {@link OldDataMonitor} warning.
+     * <p>This type checking currently uses the erasure of the type argument, so for example, the element type for a
+     * {@code List<Optional<Integer>>} is just a raw {@code Optional}, so non-integer values inside of the optional
+     * would still deserialize successfully and the resulting optional would be included in the list.
+     *
+     * @see RobustReflectionConverter#unmarshalField
+     */
+    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 +115,19 @@
             reader.moveDown();
             try {
                 Object item = readBareItem(reader, context, collection);
-                long nanoNow = System.nanoTime();
-                collection.add(item);
-                XStream2SecurityUtils.checkForCollectionDoSAttack(context, nanoNow);
+                if (elementType != null && item != null && !elementType.isInstance(item)) {
+                    var exception = new ConversionException("Invalid type for collection element");
+                    // c.f. TreeUnmarshaller.addInformationTo
+                    exception.add("required-type", elementType.getName());
+                    exception.add("class", item.getClass().getName());
+                    exception.add("converter-type", getClass().getName());
+                    reader.appendErrors(exception);
+                    RobustReflectionConverter.addErrorInContext(context, exception);
+                } else {
+                    long nanoNow = System.nanoTime();
+                    collection.add(item);
+                    XStream2SecurityUtils.checkForCollectionDoSAttack(context, nanoNow);
+                }
             } catch (CriticalXStreamException e) {
                 throw e;
             } catch (InputManipulationException e) {

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

@@ -31,9 +31,13 @@
 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 hudson.diagnosis.OldDataMonitor;
+import java.lang.reflect.Type;
 import java.util.Map;
 import java.util.logging.Logger;
 import jenkins.util.xstream.CriticalXStreamException;
+import org.jvnet.tiger_types.Types;
 
 /**
  * Loads a {@link Map} while tolerating read errors on its keys and values.
@@ -42,13 +46,47 @@
 final class RobustMapConverter extends MapConverter {
     private static final Object ERROR = new Object();
 
+    /**
+     * When available, this field holds the declared type of the keys of the map being deserialized.
+     */
+    private final @CheckForNull Class<?> keyType;
+
+    /**
+     * When available, this field holds the declared type of the values of the map being deserialized.
+     */
+    private final @CheckForNull Class<?> valueType;
+
     RobustMapConverter(Mapper mapper) {
+        this(mapper, null);
+    }
+
+    /**
+     * Creates a converter that will validate the types of map entry keys and values during deserialization.
+     * <p>Map entries whose key or value has an invalid type will be omitted from deserialized maps and may result in
+     * an {@link OldDataMonitor} warning.
+     * <p>This type checking currently uses the erasure of the type argument, so for example, the value type for a
+     * {@code Map<String, Optional<Integer>>} is just a raw {@code Optional}, so non-integer values inside of the
+     * optional would still deserialize successfully and the resulting map entry would be included in the map.
+     *
+     * @see RobustReflectionConverter#unmarshalField
+     */
+    RobustMapConverter(Mapper mapper, Type mapType) {
         super(mapper);
+        if (mapType != null && Map.class.isAssignableFrom(Types.erasure(mapType))) {
+            var baseType = Types.getBaseClass(mapType, Map.class);
+            var keyTypeArg = Types.getTypeArgument(baseType, 0, Object.class);
+            this.keyType = Types.erasure(keyTypeArg);
+            var valueTypeArg = Types.getTypeArgument(baseType, 1, Object.class);
+            this.valueType = Types.erasure(valueTypeArg);
+        } else {
+            this.keyType = null;
+            this.valueType = null;
+        }
     }
 
     @Override protected void putCurrentEntryIntoMap(HierarchicalStreamReader reader, UnmarshallingContext context, Map map, Map target) {
-        Object key = read(reader, context, map);
-        Object value = read(reader, context, map);
+        Object key = read(reader, context, map, keyType);
+        Object value = read(reader, context, map, valueType);
         if (key != ERROR && value != ERROR) {
             try {
                 long nanoNow = System.nanoTime();
@@ -64,7 +102,7 @@
         }
     }
 
-    private Object read(HierarchicalStreamReader reader, UnmarshallingContext context, Map map) {
+    private Object read(HierarchicalStreamReader reader, UnmarshallingContext context, Map map, @CheckForNull Class<?> expectedType) {
         if (!reader.hasMoreChildren()) {
             var exception = new ConversionException("Invalid map entry");
             reader.appendErrors(exception);
@@ -73,7 +111,18 @@
         }
         reader.moveDown();
         try {
-            return readBareItem(reader, context, map);
+            var object = readBareItem(reader, context, map);
+            if (expectedType != null && object != null && !expectedType.isInstance(object)) {
+                var exception = new ConversionException("Invalid type for map entry key/value");
+                // c.f. TreeUnmarshaller.addInformationTo
+                exception.add("required-type", expectedType.getName());
+                exception.add("class", object.getClass().getName());
+                exception.add("converter-type", getClass().getName());
+                reader.appendErrors(exception);
+                RobustReflectionConverter.addErrorInContext(context, exception);
+                return ERROR;
+            }
+            return object;
         } catch (CriticalXStreamException x) {
             throw x;
         } catch (XStreamException | LinkageError x) {

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

@@ -48,6 +48,7 @@
 import hudson.model.Saveable;
 import hudson.security.ACL;
 import java.lang.reflect.Field;
+import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -65,6 +66,7 @@
 import jenkins.util.xstream.CriticalXStreamException;
 import net.jcip.annotations.GuardedBy;
 import org.acegisecurity.Authentication;
+import org.jvnet.tiger_types.Types;
 
 /**
  * Custom {@link ReflectionConverter} that handle errors more gracefully.
@@ -80,7 +82,7 @@
 @SuppressWarnings({"rawtypes", "unchecked"})
 public class RobustReflectionConverter implements Converter {
 
-    private static /* non-final for Groovy */ boolean RECORD_FAILURES_FOR_ALL_AUTHENTICATIONS = SystemProperties.getBoolean(RobustReflectionConverter.class.getName() + ".recordFailuresForAllAuthentications", false);
+    static /* non-final for Groovy */ boolean RECORD_FAILURES_FOR_ALL_AUTHENTICATIONS = SystemProperties.getBoolean(RobustReflectionConverter.class.getName() + ".recordFailuresForAllAuthentications", false);
     private static /* non-final for Groovy */ boolean RECORD_FAILURES_FOR_ADMINS = SystemProperties.getBoolean(RobustReflectionConverter.class.getName() + ".recordFailuresForAdmins", false);
 
     protected final ReflectionProvider reflectionProvider;
@@ -324,7 +326,8 @@
             }
         }
 
-        Map implicitCollectionsForCurrentObject = null;
+        Map<String, Collection<Object>> implicitCollectionsForCurrentObject = new HashMap<>();
+        Map<String, Class<?>> implicitCollectionElementTypesForCurrentObject = new HashMap<>();
         while (reader.hasMoreChildren()) {
             reader.moveDown();
 
@@ -365,7 +368,7 @@
                         reflectionProvider.writeField(result, fieldName, value, classDefiningField);
                         seenFields.add(classDefiningField, fieldName);
                     } else {
-                        implicitCollectionsForCurrentObject = writeValueToImplicitCollection(context, value, implicitCollectionsForCurrentObject, result, fieldName);
+                        writeValueToImplicitCollection(reader, context, value, implicitCollectionsForCurrentObject, implicitCollectionElementTypesForCurrentObject, result, fieldName);
                     }
                 }
             } catch (CriticalXStreamException e) {
@@ -451,18 +454,23 @@
 
     protected Object unmarshalField(final UnmarshallingContext context, final Object result, Class type, Field field) {
         Converter converter = mapper.getLocalConverter(field.getDeclaringClass(), field.getName());
+        if (converter == null) {
+            if (new RobustCollectionConverter(mapper, reflectionProvider).canConvert(type)) {
+                converter = new RobustCollectionConverter(mapper, reflectionProvider, field.getGenericType());
+            } else if (new RobustMapConverter(mapper).canConvert(type)) {
+                converter = new RobustMapConverter(mapper, field.getGenericType());
+            }
+        }
         return context.convertAnother(result, type, converter);
     }
 
-    private Map writeValueToImplicitCollection(UnmarshallingContext context, Object value, Map implicitCollections, Object result, String itemFieldName) {
+    private void writeValueToImplicitCollection(HierarchicalStreamReader reader, UnmarshallingContext context, Object value, Map<String, Collection<Object>> implicitCollections, Map<String, Class<?>> implicitCollectionElementTypes, Object result, String itemFieldName) {
         String fieldName = mapper.getFieldNameForItemTypeAndName(context.getRequiredType(), value.getClass(), itemFieldName);
         if (fieldName != null) {
-            if (implicitCollections == null) {
-                implicitCollections = new HashMap(); // lazy instantiation
-            }
-            Collection collection = (Collection) implicitCollections.get(fieldName);
+            Collection collection = implicitCollections.get(fieldName);
             if (collection == null) {
-                Class fieldType = mapper.defaultImplementationOf(reflectionProvider.getFieldType(result, fieldName, null));
+                Field field = reflectionProvider.getField(result.getClass(), fieldName);
+                Class<?> fieldType = mapper.defaultImplementationOf(field.getType());
                 if (!Collection.class.isAssignableFrom(fieldType)) {
                     throw new ObjectAccessException("Field " + fieldName + " of " + result.getClass().getName() +
                             " is configured for an implicit Collection, but field is of type " + fieldType.getName());
@@ -473,10 +481,25 @@
                 collection = (Collection) pureJavaReflectionProvider.newInstance(fieldType);
                 reflectionProvider.writeField(result, fieldName, collection, null);
                 implicitCollections.put(fieldName, collection);
+                Type fieldGenericType = field.getGenericType();
+                Type elementGenericType = Types.getTypeArgument(Types.getBaseClass(fieldGenericType, Collection.class), 0, Object.class);
+                Class<?> elementType = Types.erasure(elementGenericType);
+                implicitCollectionElementTypes.put(fieldName, elementType);
+            }
+            Class<?> elementType = implicitCollectionElementTypes.getOrDefault(fieldName, Object.class);
+            if (!elementType.isInstance(value)) {
+                var exception = new ConversionException("Invalid element type for implicit collection for field: " + fieldName);
+                // c.f. TreeUnmarshaller.addInformationTo
+                exception.add("required-type", elementType.getName());
+                exception.add("class", value.getClass().getName());
+                exception.add("converter-type", getClass().getName());
+                reader.appendErrors(exception);
+                throw exception;
             }
             collection.add(value);
+        } else {
+            // TODO: Should we warn in this case? The value will be ignored.
         }
-        return implicitCollections;
     }
 
     private Class determineWhichClassDefinesField(HierarchicalStreamReader reader) {

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

@@ -32,6 +32,8 @@
 import static org.junit.Assert.assertTrue;
 
 import com.thoughtworks.xstream.security.InputManipulationException;
+import hudson.model.Saveable;
+import java.io.IOException;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -42,10 +44,24 @@
 import java.util.Map;
 import java.util.Set;
 import jenkins.util.xstream.CriticalXStreamException;
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
 
 public class RobustCollectionConverterTest {
+    private final boolean originalRecordFailures = RobustReflectionConverter.RECORD_FAILURES_FOR_ALL_AUTHENTICATIONS;
+
+    @Before
+    public void before() {
+        RobustReflectionConverter.RECORD_FAILURES_FOR_ALL_AUTHENTICATIONS = true;
+    }
+
+    @After
+    public void after() {
+        RobustReflectionConverter.RECORD_FAILURES_FOR_ALL_AUTHENTICATIONS = originalRecordFailures;
+    }
+
     @Test
     public void workingByDefaultWithSimplePayload() {
         XStream2 xstream2 = new XStream2();
@@ -173,4 +189,58 @@ private Set<Object> preparePayload() {
         }
         return set;
     }
+
+    @Issue("JENKINS-63343")
+    @Test
+    public void checkElementTypes() {
+        var xmlContent =
+                """
+                <hudson.util.RobustCollectionConverterTest_-Data>
+                  <numbers>
+                    <int>1</int>
+                    <int>2</int>
+                    <string>oops!</string>
+                    <null/>
+                    <int>3</int>
+                  </numbers>
+                </hudson.util.RobustCollectionConverterTest_-Data>
+                """;
+        var actual = (Data) new XStream2().fromXML(xmlContent);
+        assertEquals(Arrays.asList(1, 2, null, 3), actual.numbers);
+    }
+
+    @Test
+    public void rawtypes() {
+        var xmlContent =
+                """
+                <hudson.util.RobustCollectionConverterTest_-DataRaw>
+                  <values>
+                    <int>1</int>
+                    <int>2</int>
+                    <string>oops!</string>
+                    <int>3</int>
+                  </values>
+                </hudson.util.RobustCollectionConverterTest_-DataRaw>
+                """;
+        var actual = (DataRaw) new XStream2().fromXML(xmlContent);
+        assertEquals(List.of(1, 2, "oops!", 3), actual.values);
+    }
+
+    public static class Data implements Saveable {
+        private List<Integer> numbers;
+
+        @Override
+        public void save() throws IOException {
+            // We only implement Saveable so that RobustReflectionConverter logs deserialization problems.
+        }
+    }
+
+    public static class DataRaw implements Saveable {
+        private List values;
+
+        @Override
+        public void save() throws IOException {
+            // We only implement Saveable so that RobustReflectionConverter logs deserialization problems.
+        }
+    }
 }