Merge branch 'master' into scalable-icon-support-2-manage-jenkins

.github/workflows/changelog.yml

@@ -17,7 +17,7 @@ jobs:
       # Drafts your next Release notes as Pull Requests are merged into "master"
       - name: Generate GitHub Release Draft
         id: release-drafter
-        uses: release-drafter/[email protected].0
+        uses: release-drafter/[email protected].1
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
       # Generates a YAML changelog file using https://github.com/jenkinsci/jenkins-core-changelog-generator

bom/pom.xml

@@ -39,7 +39,7 @@ THE SOFTWARE.
 
   <properties>
     <asm.version>9.2</asm.version>
-    <slf4jVersion>1.7.35</slf4jVersion>
+    <slf4jVersion>1.7.36</slf4jVersion>
     <stapler.version>1642.ve454c4518974</stapler.version>
     <groovy.version>2.4.21</groovy.version>
   </properties>

core/src/main/java/hudson/slaves/SlaveComputer.java

@@ -230,7 +230,7 @@ public TaskListener getListener() {
     public String getIconClassName() {
         Future<?> l = lastConnectActivity;
         if (l != null && !l.isDone())
-            return "icon-computer-flash";
+            return "icon-computer";
         return super.getIconClassName();
     }
 

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

@@ -33,9 +33,11 @@
 import com.thoughtworks.xstream.core.ClassLoaderReference;
 import com.thoughtworks.xstream.io.HierarchicalStreamReader;
 import com.thoughtworks.xstream.mapper.Mapper;
+import com.thoughtworks.xstream.security.InputManipulationException;
 import java.util.Collection;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.logging.Logger;
 import jenkins.util.xstream.CriticalXStreamException;
 
 /**
@@ -83,9 +85,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);
             } catch (CriticalXStreamException e) {
                 throw e;
+            } catch (InputManipulationException e) {
+                Logger.getLogger(RobustCollectionConverter.class.getName()).warning(
+                        "DoS detected and prevented. If the heuristic was too aggressive, " +
+                                "you can customize the behavior by setting the hudson.util.XStream2.collectionUpdateLimit system property. " +
+                                "See https://www.jenkins.io/redirect/xstream-dos-prevention for more information.");
+                throw new CriticalXStreamException(e);
             } catch (XStreamException | LinkageError e) {
                 RobustReflectionConverter.addErrorInContext(context, e);
             }

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

@@ -29,15 +29,16 @@
 import com.thoughtworks.xstream.converters.collections.MapConverter;
 import com.thoughtworks.xstream.io.HierarchicalStreamReader;
 import com.thoughtworks.xstream.mapper.Mapper;
+import com.thoughtworks.xstream.security.InputManipulationException;
 import java.util.Map;
+import java.util.logging.Logger;
 import jenkins.util.xstream.CriticalXStreamException;
 
 /**
  * Loads a {@link Map} while tolerating read errors on its keys and values.
  */
 @SuppressWarnings({"rawtypes", "unchecked"})
 final class RobustMapConverter extends MapConverter {
-
     private static final Object ERROR = new Object();
 
     RobustMapConverter(Mapper mapper) {
@@ -48,7 +49,17 @@ final class RobustMapConverter extends MapConverter {
         Object key = read(reader, context, map);
         Object value = read(reader, context, map);
         if (key != ERROR && value != ERROR) {
-            target.put(key, value);
+            try {
+                long nanoNow = System.nanoTime();
+                target.put(key, value);
+                XStream2SecurityUtils.checkForCollectionDoSAttack(context, nanoNow);
+            } catch (InputManipulationException e) {
+                Logger.getLogger(RobustMapConverter.class.getName()).warning(
+                        "DoS detected and prevented. If the heuristic was too aggressive, " +
+                                "you can customize the behavior by setting the hudson.util.XStream2.collectionUpdateLimit system property. " +
+                                "See https://www.jenkins.io/redirect/xstream-dos-prevention for more information.");
+                throw new CriticalXStreamException(e);
+            }
         }
     }
 

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

@@ -42,6 +42,7 @@
 import com.thoughtworks.xstream.io.HierarchicalStreamReader;
 import com.thoughtworks.xstream.io.HierarchicalStreamWriter;
 import com.thoughtworks.xstream.mapper.Mapper;
+import com.thoughtworks.xstream.security.InputManipulationException;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.diagnosis.OldDataMonitor;
 import hudson.model.Saveable;
@@ -369,6 +370,12 @@ public Object doUnmarshal(final Object result, final HierarchicalStreamReader re
                 }
             } catch (CriticalXStreamException e) {
                 throw e;
+            } catch (InputManipulationException e) {
+                LOGGER.warning(
+                        "DoS detected and prevented. If the heuristic was too aggressive, " +
+                                "you can customize the behavior by setting the hudson.util.XStream2.collectionUpdateLimit system property. " +
+                                "See https://www.jenkins.io/redirect/xstream-dos-prevention for more information.");
+                throw new CriticalXStreamException(e);
             } catch (XStreamException e) {
                 if (critical) {
                     throw new CriticalXStreamException(e);

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

@@ -82,7 +82,10 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.model.Jenkins;
+import jenkins.util.SystemProperties;
 import jenkins.util.xstream.SafeURLConverter;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 /**
  * {@link XStream} customized in various ways for Jenkins’ needs.
@@ -91,6 +94,17 @@
 public class XStream2 extends XStream {
 
     private static final Logger LOGGER = Logger.getLogger(XStream2.class.getName());
+    /**
+     * Determine what is the value (in seconds) of the "collectionUpdateLimit" added by XStream
+     * to protect against http://x-stream.github.io/CVE-2021-43859.html.
+     * It corresponds to the accumulated timeout when adding an item to a collection.
+     *
+     * Default: 5 seconds (in contrary to XStream default to 20 which is a bit too tolerant)
+     * If negative: disable the DoS protection
+     */
+    @Restricted(NoExternalUse.class)
+    public static final String COLLECTION_UPDATE_LIMIT_PROPERTY_NAME = XStream2.class.getName() + ".collectionUpdateLimit";
+    private static final int COLLECTION_UPDATE_LIMIT_DEFAULT_VALUE = 5;
 
     private RobustReflectionConverter reflectionConverter;
     private final ThreadLocal<Boolean> oldData = new ThreadLocal<>();
@@ -250,6 +264,9 @@ static String trimVersion(String version) {
     }
 
     private void init() {
+        int updateLimit = SystemProperties.getInteger(COLLECTION_UPDATE_LIMIT_PROPERTY_NAME, COLLECTION_UPDATE_LIMIT_DEFAULT_VALUE);
+        this.setCollectionUpdateLimit(updateLimit);
+
         // list up types that should be marshalled out like a value, without referential integrity tracking.
         addImmutableType(Result.class, false);
 

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

@@ -0,0 +1,61 @@
+/*
+ * Copyright (C) 2021, 2022 XStream Committers.
+ * All rights reserved.
+ *
+ * The software in this package is published under the terms of the BSD
+ * style license a copy of which has been included with this distribution in
+ * the LICENSE.txt file.
+ *
+ * Created on 21. September 2021 by Joerg Schaible
+ */
+// Updated when included in Jenkins code by changing currentTimeMillis to nanoTime + comments
+
+package hudson.util;
+
+import com.thoughtworks.xstream.XStream;
+import com.thoughtworks.xstream.converters.ConversionException;
+import com.thoughtworks.xstream.converters.UnmarshallingContext;
+import com.thoughtworks.xstream.security.InputManipulationException;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+
+/**
+ * Strongly inspired by https://github.com/x-stream/xstream/blob/61a00fa225dc99488013869b57b772af8e2fea03/xstream/src/java/com/thoughtworks/xstream/core/SecurityUtils.java#L25
+ * and taking into account https://github.com/x-stream/xstream/issues/282
+ *
+ * Once the related issue is fixed, we will be able to use the regular method from XStream.
+ *
+ * @see com.thoughtworks.xstream.core.SecurityUtils
+ */
+@Restricted(NoExternalUse.class)
+public class XStream2SecurityUtils {
+    /**
+     * Check the consumed time adding elements to collections or maps.
+     *
+     * Every custom converter should call this method after an unmarshalled element has been added to a collection or
+     * map. In case of an attack the operation will take too long, because the calculation of the hash code or the
+     * comparison of the elements in the collection operate on recursive structures.
+     *
+     * @param context the unmarshalling context
+     * @param startNano the nanoTime just before the element was added to the collection or map
+     * @since 1.4.19
+     */
+    public static void checkForCollectionDoSAttack(final UnmarshallingContext context, final long startNano) {
+        final int diff = (int) ((System.nanoTime() - startNano) / 1000_000_000);
+        if (diff > 0) {
+            final Integer secondsUsed = (Integer) context.get(XStream.COLLECTION_UPDATE_SECONDS);
+            if (secondsUsed != null) {
+                final Integer limit = (Integer) context.get(XStream.COLLECTION_UPDATE_LIMIT);
+                if (limit == null) {
+                    throw new ConversionException("Missing limit for updating collections.");
+                }
+                final int seconds = secondsUsed + diff;
+                if (seconds > limit) {
+                    throw new InputManipulationException(
+                            "Denial of Service attack assumed. Adding elements to collections or maps exceeds " + limit + " seconds.");
+                }
+                context.put(XStream.COLLECTION_UPDATE_SECONDS, seconds);
+            }
+        }
+    }
+}

core/src/main/java/jenkins/security/s2m/CallableDirectionChecker.java

@@ -29,8 +29,6 @@ public class CallableDirectionChecker extends RoleChecker {
 
     private static final String BYPASS_PROP = CallableDirectionChecker.class.getName() + ".allow";
 
-    private static final String ALLOW_ANY_ROLE_PROP = CallableDirectionChecker.class.getName() + ".allowAnyRole";
-
     /**
      * Switch to disable all the defense mechanism completely.
      *
@@ -49,12 +47,6 @@ public void check(RoleSensitive subject, @NonNull Collection<Role> expected) thr
             return;    // known to be safe
         }
 
-        if (expected.isEmpty() && SystemProperties.getBoolean(ALLOW_ANY_ROLE_PROP, true)) {
-            // TODO Is this even something we want to support, or should all infrastructure callables be exempted from the required role check?
-            LOGGER.log(Level.FINE, "Executing {0} is allowed since it is targeted for any role", name);
-            return;
-        }
-
         if (BYPASS) {
             LOGGER.log(Level.FINE, "Allowing {0} to be sent from agent to controller because bypass is set", name);
             return;