From 2f4e04cc74fa402967b79d3fee02331e6359ee6f Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Thu, 12 Sep 2024 14:57:11 -0400 Subject: [PATCH 1/3] [JENKINS-63343] Validate element types in RobustCollectionConverter in some cases --- .../util/RobustCollectionConverter.java | 38 ++++++++++++-- .../util/RobustReflectionConverter.java | 4 ++ .../util/RobustCollectionConverterTest.java | 50 +++++++++++++++++++ 3 files changed, 88 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/hudson/util/RobustCollectionConverter.java b/core/src/main/java/hudson/util/RobustCollectionConverter.java index 64dbbc7d9e9a..5965b468eca9 100644 --- a/core/src/main/java/hudson/util/RobustCollectionConverter.java +++ b/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> + // 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 +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) { diff --git a/core/src/main/java/hudson/util/RobustReflectionConverter.java b/core/src/main/java/hudson/util/RobustReflectionConverter.java index d1bc500003e1..1c0342ab69dd 100644 --- a/core/src/main/java/hudson/util/RobustReflectionConverter.java +++ b/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); } diff --git a/core/src/test/java/hudson/util/RobustCollectionConverterTest.java b/core/src/test/java/hudson/util/RobustCollectionConverterTest.java index 7786fb0833f7..940787339ddc 100644 --- a/core/src/test/java/hudson/util/RobustCollectionConverterTest.java +++ b/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 preparePayload() { } return set; } + + @Test + public void checkElementTypes() { + var expected = new Numbers(1, 2, 3); + var xmlContent = + """ + + + 1 + 2 + oops! + 3 + + + """; + var actual = (Numbers) new XStream2().fromXML(xmlContent); + assertEquals(expected, actual); + } + + public static class Numbers { + private final List 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 + "]"; + } + } } From 966f598d7cec0359aff7694bcd87ff3e10135965 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Wed, 18 Sep 2024 16:39:36 -0400 Subject: [PATCH 2/3] [JENKINS-63343] Also validate keys and values in map entries --- .../util/RobustCollectionConverter.java | 4 +- .../java/hudson/util/RobustMapConverter.java | 48 +++++++++++++++++-- .../util/RobustReflectionConverter.java | 9 ++-- 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/hudson/util/RobustCollectionConverter.java b/core/src/main/java/hudson/util/RobustCollectionConverter.java index 5965b468eca9..572c4570a7fc 100644 --- a/core/src/main/java/hudson/util/RobustCollectionConverter.java +++ b/core/src/main/java/hudson/util/RobustCollectionConverter.java @@ -56,8 +56,8 @@ 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. + * 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 diff --git a/core/src/main/java/hudson/util/RobustMapConverter.java b/core/src/main/java/hudson/util/RobustMapConverter.java index 896138ace0e9..5897645d9902 100644 --- a/core/src/main/java/hudson/util/RobustMapConverter.java +++ b/core/src/main/java/hudson/util/RobustMapConverter.java @@ -30,9 +30,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. @@ -41,13 +44,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(); @@ -63,10 +93,20 @@ 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) { reader.moveDown(); try { - return readBareItem(reader, context, map); + try { + var object = readBareItem(reader, context, map); + if (expectedType != null) { + // When possible, disallow invalid keys and values. + expectedType.cast(object); + } + return object; + } catch (ClassCastException e) { + RobustReflectionConverter.addErrorInContext(context, e); + 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 1c0342ab69dd..26b29b215f6b 100644 --- a/core/src/main/java/hudson/util/RobustReflectionConverter.java +++ b/core/src/main/java/hudson/util/RobustReflectionConverter.java @@ -451,9 +451,12 @@ 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()); + 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); } From 61db49c2e723d223552f799b9206273056a13b3d Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Wed, 18 Sep 2024 17:13:27 -0400 Subject: [PATCH 3/3] [JENKINS-63343] Improve OldDataMonitor error messages and update tests --- .../util/RobustCollectionConverter.java | 5 +- .../java/hudson/util/RobustMapConverter.java | 6 +- .../util/RobustReflectionConverter.java | 2 +- .../util/RobustCollectionConverterTest.java | 53 +++++++-------- .../hudson/util/RobustMapConverterTest.java | 68 ++++++++++++++++++- 5 files changed, 101 insertions(+), 33 deletions(-) diff --git a/core/src/main/java/hudson/util/RobustCollectionConverter.java b/core/src/main/java/hudson/util/RobustCollectionConverter.java index 572c4570a7fc..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; @@ -116,7 +117,9 @@ protected void populateCollection(HierarchicalStreamReader reader, Unmarshalling collection.add(item); XStream2SecurityUtils.checkForCollectionDoSAttack(context, nanoNow); } catch (ClassCastException e) { - RobustReflectionConverter.addErrorInContext(context, e); + var exception = new ConversionException("Invalid type for collection element", e); + reader.appendErrors(exception); + RobustReflectionConverter.addErrorInContext(context, exception); } } catch (CriticalXStreamException e) { throw e; diff --git a/core/src/main/java/hudson/util/RobustMapConverter.java b/core/src/main/java/hudson/util/RobustMapConverter.java index 4677603f8a1e..3349c3c5da50 100644 --- a/core/src/main/java/hudson/util/RobustMapConverter.java +++ b/core/src/main/java/hudson/util/RobustMapConverter.java @@ -103,15 +103,17 @@ private Object read(HierarchicalStreamReader reader, UnmarshallingContext contex } reader.moveDown(); try { + var object = readBareItem(reader, context, map); try { - var object = readBareItem(reader, context, map); if (expectedType != null) { // When possible, disallow invalid keys and values. expectedType.cast(object); } return object; } catch (ClassCastException e) { - RobustReflectionConverter.addErrorInContext(context, 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) { diff --git a/core/src/main/java/hudson/util/RobustReflectionConverter.java b/core/src/main/java/hudson/util/RobustReflectionConverter.java index 26b29b215f6b..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; diff --git a/core/src/test/java/hudson/util/RobustCollectionConverterTest.java b/core/src/test/java/hudson/util/RobustCollectionConverterTest.java index 940787339ddc..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; @@ -40,13 +42,26 @@ 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.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(); @@ -177,50 +192,32 @@ private Set preparePayload() { @Test public void checkElementTypes() { - var expected = new Numbers(1, 2, 3); + var expected = new Data(1, 2, 3); var xmlContent = """ - + 1 2 oops! 3 - + """; - var actual = (Numbers) new XStream2().fromXML(xmlContent); - assertEquals(expected, actual); + var actual = (Data) new XStream2().fromXML(xmlContent); + assertEquals(expected.numbers, actual.numbers); } - public static class Numbers { + public static class Data implements Saveable { private final List numbers; - public Numbers(Integer... numbers) { + public Data(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 + "]"; + 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. + } } }