diff --git a/core/src/main/java/hudson/util/RobustCollectionConverter.java b/core/src/main/java/hudson/util/RobustCollectionConverter.java index 64dbbc7d9e9a..47ca076d18b4 100644 --- a/core/src/main/java/hudson/util/RobustCollectionConverter.java +++ b/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,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 +56,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 deserialized. + * This is used to ensure that deserialized elements have the expected type. + */ + // TODO: We only support one level of parameterization, i.e. the elements of a List> + // will be treated as List, 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 +108,19 @@ 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) { + var exception = new ConversionException("Invalid type for collection element", e); + reader.appendErrors(exception); + RobustReflectionConverter.addErrorInContext(context, exception); + } } catch (CriticalXStreamException e) { throw e; } catch (InputManipulationException e) { diff --git a/core/src/main/java/hudson/util/RobustMapConverter.java b/core/src/main/java/hudson/util/RobustMapConverter.java index f845e38771cc..3349c3c5da50 100644 --- a/core/src/main/java/hudson/util/RobustMapConverter.java +++ b/core/src/main/java/hudson/util/RobustMapConverter.java @@ -31,9 +31,12 @@ 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.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 +45,40 @@ 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. + * This is used to ensure that deserialized keys have the expected type. + */ + // See note above RobustCollectionConverter.elementType for limitations regarding non-concrete parameterized types. + private final @CheckForNull Class keyType; + + /** + * When available, this field holds the declared type of the values of the map being deserialized. + * This is used to ensure that deserialized values have the expected type. + */ + private final @CheckForNull Class valueType; + RobustMapConverter(Mapper mapper) { + this(mapper, null); + } + + 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 +94,7 @@ final class RobustMapConverter extends MapConverter { } } - 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 +103,19 @@ private Object read(HierarchicalStreamReader reader, UnmarshallingContext contex } reader.moveDown(); try { - return readBareItem(reader, context, map); + var object = readBareItem(reader, context, map); + try { + if (expectedType != null) { + // When possible, disallow invalid keys and values. + expectedType.cast(object); + } + return object; + } catch (ClassCastException e) { + var exception = new ConversionException("Invalid type for map entry key/value", e); + reader.appendErrors(exception); + RobustReflectionConverter.addErrorInContext(context, exception); + return ERROR; + } } catch (CriticalXStreamException x) { throw x; } catch (XStreamException | LinkageError x) { diff --git a/core/src/main/java/hudson/util/RobustReflectionConverter.java b/core/src/main/java/hudson/util/RobustReflectionConverter.java index d1bc500003e1..b1b19dcd7fff 100644 --- a/core/src/main/java/hudson/util/RobustReflectionConverter.java +++ b/core/src/main/java/hudson/util/RobustReflectionConverter.java @@ -80,7 +80,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; @@ -451,6 +451,13 @@ 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()); + 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); } diff --git a/core/src/test/java/hudson/util/RobustCollectionConverterTest.java b/core/src/test/java/hudson/util/RobustCollectionConverterTest.java index 7786fb0833f7..510d9a00196c 100644 --- a/core/src/test/java/hudson/util/RobustCollectionConverterTest.java +++ b/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,35 @@ private Set preparePayload() { } return set; } + + @Test + public void checkElementTypes() { + var expected = new Data(1, 2, 3); + var xmlContent = + """ + + + 1 + 2 + oops! + 3 + + + """; + var actual = (Data) new XStream2().fromXML(xmlContent); + assertEquals(expected.numbers, actual.numbers); + } + + public static class Data implements Saveable { + private final List numbers; + + public Data(Integer... numbers) { + this.numbers = new ArrayList(Arrays.asList(numbers)); + } + + @Override + public void save() throws IOException { + // We only implement Saveable so that RobustReflectionConverter logs deserialization problems. + } + } } diff --git a/core/src/test/java/hudson/util/RobustMapConverterTest.java b/core/src/test/java/hudson/util/RobustMapConverterTest.java index b74ac9303f71..611a564381cb 100644 --- a/core/src/test/java/hudson/util/RobustMapConverterTest.java +++ b/core/src/test/java/hudson/util/RobustMapConverterTest.java @@ -32,13 +32,29 @@ import static org.junit.Assert.assertTrue; import com.thoughtworks.xstream.security.InputManipulationException; +import hudson.model.Saveable; +import java.io.IOException; import java.util.HashMap; import java.util.Map; 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 RobustMapConverterTest { + 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; + } + /** * As RobustMapConverter is the replacer of the default MapConverter * We had to patch it in order to not be impacted by CVE-2021-43859 @@ -146,6 +162,7 @@ private Map preparePayload() { @Test public void robustAgainstInvalidEntry() { + RobustReflectionConverter.RECORD_FAILURES_FOR_ALL_AUTHENTICATIONS = true; XStream2 xstream2 = new XStream2(); String xml = """ @@ -184,7 +201,56 @@ public void robustAgainstInvalidEntryWithNoValue() { assertThat(data.map, equalTo(Map.of("key2", "value2"))); } - private static final class Data { + @Test + public void robustAgainstInvalidKeyType() { + XStream2 xstream2 = new XStream2(); + String xml = + """ + + + + 1 + value1 + + + key2 + value2 + + + + """; + Data data = (Data) xstream2.fromXML(xml); + assertThat(data.map, equalTo(Map.of("key2", "value2"))); + } + + @Test + public void robustAgainstInvalidValueType() { + XStream2 xstream2 = new XStream2(); + String xml = + """ + + + + key1 + value1 + + + key2 + 2 + + + + """; + Data data = (Data) xstream2.fromXML(xml); + assertThat(data.map, equalTo(Map.of("key1", "value1"))); + } + + private static class Data implements Saveable { Map map; + + @Override + public void save() throws IOException { + // We only implement Saveable so that RobustReflectionConverter logs deserialization problems. + } } }