High· 8.0GHSA-qv6f-rcv6-6q3x CVE-2021-21604CWE-502Published May 24, 2022

Improper handling of REST API XML deserialization errors in Jenkins

Research is free — Hunters explains how the bug works, the root-cause code pattern, how the fix addresses it, and how to test whether a target is affected, in chat. Investigate & write exploit is a paid run — the engine reads the advisory and fix commits, then builds and validates a working proof-of-concept exploit with reproduction steps.

Affected versions

0 → fixed in 2.263.22.264 → fixed in 2.275

Details

Jenkins provides XML REST APIs to configure views, jobs, and other items. When deserialization fails because of invalid data, Jenkins 2.274 and earlier, LTS 2.263.1 and earlier stores invalid object references created through these endpoints in the Old Data Monitor. If an administrator discards the old data, some erroneous data submitted to these endpoints may be persisted. This allows attackers with View/Create, Job/Create, Agent/Create, or their respective */Configure permissions to inject crafted content into Old Data Monitor that results in the instantiation of potentially unsafe objects when discarded by an administrator.\n\nJenkins 2.275, LTS 2.263.2 does not record submissions from users in Old Data Monitor anymore. In case of problems, the [Java system properties](https://www.jenkins.io/doc/book/managing/system-properties/) `hudson.util.RobustReflectionConverter.recordFailuresForAdmins` and `hudson.util.RobustReflectionConverter.recordFailuresForAllAuthentications` can be set to true to record configuration data submissions from administrators or all users, partially or completely disabling this fix.

The fix

[SECURITY-1923]

Jeff Thompson· Jan 11, 2021, 04:51 PM+459−2f1056bd814

core/src/main/java/hudson/util/RobustReflectionConverter.java+30 −2

@@ -57,7 +57,12 @@
 import java.util.logging.Logger;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import net.jcip.annotations.GuardedBy;
+ 
+import hudson.security.ACL;
+import jenkins.model.Jenkins;
+import jenkins.util.SystemProperties;
 import jenkins.util.xstream.CriticalXStreamException;
+import org.acegisecurity.Authentication;
  
 /**
  * Custom {@link ReflectionConverter} that handle errors more gracefully.
@@ -73,6 +78,9 @@
 @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);
+    private static /* non-final for Groovy */ boolean RECORD_FAILURES_FOR_ADMINS = SystemProperties.getBoolean(RobustReflectionConverter.class.getName() + ".recordFailuresForAdmins", false);
+ 
     protected final ReflectionProvider reflectionProvider;
     protected final Mapper mapper;
     protected transient SerializationMembers serializationMethodInvoker;
@@ -368,8 +376,8 @@ public Object doUnmarshal(final Object result, final HierarchicalStreamReader re
             reader.moveUp();
         }
  
-        // Report any class/field errors in Saveable objects
-        if (context.get("ReadError") != null && context.get("Saveable") == result) {
+        // Report any class/field errors in Saveable objects if it happens during loading of existing data from disk
+        if (shouldReportUnloadableDataForCurrentUser() && context.get("ReadError") != null && context.get("Saveable") == result) {
             // Avoid any error in OldDataMonitor to be catastrophic. See JENKINS-62231 and JENKINS-59582
             // The root cause is the OldDataMonitor extension is not ready before a plugin triggers an error, for 
             // example when trying to load a field that was created by a new version and you downgrade to the previous
@@ -392,6 +400,26 @@ public Object doUnmarshal(final Object result, final HierarchicalStreamReader re
         return result;
     }
  
+    /**
+     * Returns whether the current user authentication is allowed to have errors loading data reported.
+     *
+     * <p>{@link ACL#SYSTEM} always has errors reported.
+     * If {@link #RECORD_FAILURES_FOR_ALL_AUTHENTICATIONS} is {@code true}, errors are reported for all authentications.
+     * Otherwise errors are reported for users with {@link Jenkins#ADMINISTER} permission if {@link #RECORD_FAILURES_FOR_ADMINS} is {@code true}.</p>
+     *
+     * @return whether the current user authentication is allowed to have errors loading data reported.
+     */
+    private static boolean shouldReportUnloadableDataForCurrentUser() {
+        if (RECORD_FAILURES_FOR_ALL_AUTHENTICATIONS) {
+            return true;
+        }
+        final Authentication authentication = Jenkins.getAuthentication();
+        if (authentication.equals(ACL.SYSTEM)) {
+            return true;
+        }
+        return RECORD_FAILURES_FOR_ADMINS && Jenkins.get().hasPermission(Jenkins.ADMINISTER);
+    }
+ 
     public static void addErrorInContext(UnmarshallingContext context, Throwable e) {
         LOGGER.log(FINE, "Failed to load", e);
         ArrayList<Throwable> list = (ArrayList<Throwable>)context.get("ReadError");

test/src/test/java/hudson/model/ComputerSEC1923Test.java+81 −0

@@ -0,0 +1,81 @@
+package hudson.model;
+ 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import hudson.ExtensionList;
+import hudson.diagnosis.OldDataMonitor;
+import jenkins.model.Jenkins;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
+ 
+import java.util.Map;
+ 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+ 
+public class ComputerSEC1923Test {
+ 
+    private static final String CONFIGURATOR = "configure_user";
+ 
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+ 
+    @Before
+    public void setupSecurity() {
+        j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+        MockAuthorizationStrategy mas = new MockAuthorizationStrategy();
+        mas.grant(Computer.CONFIGURE, Computer.EXTENDED_READ, Jenkins.READ)
+                .everywhere()
+                .to(CONFIGURATOR);
+        j.jenkins.setAuthorizationStrategy(mas);
+    }
+ 
+    @Issue("SECURITY-1923")
+    @Test
+    public void configDotXmlWithValidXmlAndBadField() throws Exception {
+        Computer computer = j.createSlave().toComputer();
+ 
+        JenkinsRule.WebClient wc = j.createWebClient();
+        wc.login(CONFIGURATOR);
+        WebRequest req = new WebRequest(wc.createCrumbedUrl(String.format("%s/config.xml", computer.getUrl())), HttpMethod.POST);
+        req.setAdditionalHeader("Content-Type", "application/xml");
+        req.setRequestBody(VALID_XML_BAD_FIELD_USER_XML);
+ 
+        try {
+            wc.getPage(req);
+            fail("Should have returned failure.");
+        } catch (FailingHttpStatusCodeException e) {
+            // This really shouldn't return 500, but that's what it does now.
+            assertThat(e.getStatusCode(), equalTo(500));
+        }
+ 
+        OldDataMonitor odm = ExtensionList.lookupSingleton(OldDataMonitor.class);
+        Map<Saveable, OldDataMonitor.VersionRange> data = odm.getData();
+ 
+        assertThat(data.size(), equalTo(0));
+ 
+        odm.doDiscard(null, null);
+ 
+        User.AllUsers.scanAll();
+        boolean createUser = false;
+        User badUser = User.getById("foo", createUser);
+ 
+        assertNull("Should not have created user.", badUser);
+    }
+ 
+    private static final String VALID_XML_BAD_FIELD_USER_XML =
+            "<hudson.model.User>\n" +
+                    "  <id>foo</id>\n" +
+                    "  <fullName>Foo User</fullName>\n" +
+                    "  <badField/>\n" +
+                    "</hudson.model.User>\n";
+ 
+ 
+}

test/src/test/java/hudson/model/FreeStyleProjectSEC1923Test.java+80 −0

@@ -0,0 +1,80 @@
+package hudson.model;
+ 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import hudson.ExtensionList;
+import hudson.diagnosis.OldDataMonitor;
+import jenkins.model.Jenkins;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
+ 
+import java.util.Map;
+ 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+ 
+public class FreeStyleProjectSEC1923Test {
+ 
+    private static final String CONFIGURATOR = "configure_user";
+ 
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+ 
+    @Before
+    public void setupSecurity() {
+        j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+        MockAuthorizationStrategy mas = new MockAuthorizationStrategy();
+        mas.grant(Item.CONFIGURE, Item.READ, Jenkins.READ)
+                .everywhere()
+                .to(CONFIGURATOR);
+        j.jenkins.setAuthorizationStrategy(mas);
+    }
+ 
+    @Issue("SECURITY-1923")
+    @Test
+    public void configDotXmlWithValidXmlAndBadField() throws Exception {
+        FreeStyleProject project = j.createFreeStyleProject();
+ 
+        JenkinsRule.WebClient wc = j.createWebClient();
+        wc.login(CONFIGURATOR);
+        WebRequest req = new WebRequest(wc.createCrumbedUrl(String.format("%s/config.xml", project.getUrl())), HttpMethod.POST);
+        req.setAdditionalHeader("Content-Type", "application/xml");
+        req.setRequestBody(VALID_XML_BAD_FIELD_USER_XML);
+ 
+        try {
+            wc.getPage(req);
+            fail("Should have returned failure.");
+        } catch (FailingHttpStatusCodeException e) {
+            // This really shouldn't return 500, but that's what it does now.
+            assertThat(e.getStatusCode(), equalTo(500));
+        }
+ 
+        OldDataMonitor odm = ExtensionList.lookupSingleton(OldDataMonitor.class);
+        Map<Saveable, OldDataMonitor.VersionRange> data = odm.getData();
+ 
+        assertThat(data.size(), equalTo(0));
+ 
+        odm.doDiscard(null, null);
+ 
+        User.AllUsers.scanAll();
+        boolean createUser = false;
+        User badUser = User.getById("foo", createUser);
+ 
+        assertNull("Should not have created user.", badUser);
+    }
+ 
+    private static final String VALID_XML_BAD_FIELD_USER_XML =
+            "<hudson.model.User>\n" +
+                    "  <id>foo</id>\n" +
+                    "  <fullName>Foo User</fullName>\n" +
+                    "  <badField/>\n" +
+                    "</hudson.model.User>\n";
+ 
+}

test/src/test/java/hudson/model/ItemGroupMixInSEC1923Test.java+81 −0

@@ -0,0 +1,81 @@
+package hudson.model;
+ 
+ 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import hudson.ExtensionList;
+import hudson.diagnosis.OldDataMonitor;
+import jenkins.model.Jenkins;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
+ 
+import java.net.URL;
+import java.util.Map;
+ 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+ 
+public class ItemGroupMixInSEC1923Test {
+ 
+  private static final String CREATOR = "create_user";
+ 
+  @Rule
+  public JenkinsRule j = new JenkinsRule();
+ 
+  @Before
+  public void setupSecurity() {
+    j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+    MockAuthorizationStrategy mas = new MockAuthorizationStrategy();
+    mas.grant(Item.CREATE, Item.CONFIGURE, Item.READ, Jenkins.READ)
+            .everywhere()
+            .to(CREATOR);
+    j.jenkins.setAuthorizationStrategy(mas);
+  }
+ 
+  @Issue("SECURITY-1923")
+  @Test
+  public void doCreateItemWithValidXmlAndBadField() throws Exception {
+    JenkinsRule.WebClient wc = j.createWebClient();
+    wc.login(CREATOR);
+    WebRequest req = new WebRequest(wc.createCrumbedUrl("createItem?name=testProject"), HttpMethod.POST);
+    req.setAdditionalHeader("Content-Type", "application/xml");
+    req.setRequestBody(VALID_XML_BAD_FIELD_USER_XML);
+ 
+    try {
+      wc.getPage(req);
+      fail("Should have returned failure.");
+    } catch (FailingHttpStatusCodeException e) {
+      // This really shouldn't return 500, but that's what it does now.
+      assertThat(e.getStatusCode(), equalTo(500));
+    }
+ 
+    OldDataMonitor odm = ExtensionList.lookupSingleton(OldDataMonitor.class);
+    Map<Saveable, OldDataMonitor.VersionRange> data = odm.getData();
+ 
+    assertThat(data.size(), equalTo(0));
+ 
+    odm.doDiscard(null, null);
+ 
+    User.AllUsers.scanAll();
+    boolean createUser = false;
+    User badUser = User.getById("foo", createUser);
+ 
+    assertNull("Should not have created user.", badUser);
+  }
+ 
+  private static final String VALID_XML_BAD_FIELD_USER_XML =
+          "<hudson.model.User>\n" +
+                  "  <id>foo</id>\n" +
+                  "  <fullName>Foo User</fullName>\n" +
+                  "  <badField/>\n" +
+                  "</hudson.model.User>\n";
+}

test/src/test/java/hudson/model/ViewSEC1923Test.java+187 −0

@@ -0,0 +1,187 @@
+package hudson.model;
+ 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import hudson.ExtensionList;
+import hudson.diagnosis.OldDataMonitor;
+import jenkins.model.Jenkins;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
+ 
+import java.util.Map;
+ 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
+ 
+public class ViewSEC1923Test {
+ 
+    private static final String CREATE_VIEW = "create_view";
+    private static final String CONFIGURATOR = "configure_user";
+ 
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+ 
+    @Before
+    public void setupSecurity() {
+        j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+        MockAuthorizationStrategy mas = new MockAuthorizationStrategy();
+        mas.grant(View.CREATE, View.READ, Jenkins.READ)
+                .everywhere()
+                .to(CREATE_VIEW);
+        mas.grant(View.CONFIGURE, View.READ, Jenkins.READ)
+                .everywhere()
+                .to(CONFIGURATOR);
+        j.jenkins.setAuthorizationStrategy(mas);
+    }
+ 
+    @Test
+    @Issue("SECURITY-1923")
+    public void simplifiedOriginalDescription() throws Exception {
+        /*  This is a simplified version of the original report in SECURITY-1923.
+            The XML is broken, because the root element doesn't have a matching end.
+            The last line is almost a matching end, but it lacks the slash character.
+            Instead that line gets interpreted as another contained element, one that
+            doesn't actually exist on the class. This causes it to get logged by the
+            old data monitor. */
+ 
+        JenkinsRule.WebClient wc = j.createWebClient();
+        wc.login(CREATE_VIEW);
+ 
+        /*  The view to create has to be nonexistent, otherwise a different code path is followed
+            and the vulnerability doesn't manifest. */
+        WebRequest req = new WebRequest(wc.createCrumbedUrl("createView?name=nonexistent"), HttpMethod.POST);
+        req.setAdditionalHeader("Content-Type", "application/xml");
+        req.setRequestBody(ORIGINAL_BAD_USER_XML);
+ 
+        try {
+            wc.getPage(req);
+            fail("Should have returned failure.");
+        } catch (FailingHttpStatusCodeException e) {
+            // This really shouldn't return 500, but that's what it does now.
+            assertThat(e.getStatusCode(), equalTo(500));
+ 
+            // This should have a different message, but this is the current behavior demonstrating the problem.
+            assertThat(e.getResponse().getContentAsString(), containsString("A problem occurred while processing the request."));
+        }
+ 
+        OldDataMonitor odm = ExtensionList.lookupSingleton(OldDataMonitor.class);
+        Map<Saveable, OldDataMonitor.VersionRange> data = odm.getData();
+ 
+        assertThat(data.size(), equalTo(0));
+ 
+        odm.doDiscard(null, null);
+ 
+        View view = j.getInstance().getView("nonexistent");
+ 
+        // The view should still be nonexistent, as we gave it a user and not a view.
+        assertNull("Should not have created view.", view);
+ 
+        User.AllUsers.scanAll();
+        boolean createUser = false;
+        User badUser = User.getById("foo", createUser);
+ 
+        assertNull("Should not have created user.", badUser);
+    }
+ 
+    @Test
+    @Issue("SECURITY-1923")
+    public void simplifiedWithValidXmlAndBadField() throws Exception {
+        /*  This is the same thing as the original report, except it uses valid XML.
+            It just adds in additional invalid field, which gets picked up by the old data monitor.
+            Way too much duplicated code here, but this is just for demonstration. */
+ 
+        JenkinsRule.WebClient wc = j.createWebClient();
+        wc.login(CREATE_VIEW);
+ 
+        /*  The view to create has to be nonexistent, otherwise a different code path is followed
+            and the vulnerability doesn't manifest. */
+        WebRequest req = new WebRequest(wc.createCrumbedUrl("createView?name=nonexistent"), HttpMethod.POST);
+        req.setAdditionalHeader("Content-Type", "application/xml");
+        req.setRequestBody(VALID_XML_BAD_FIELD_USER_XML);
+ 
+        try {
+            wc.getPage(req);
+            fail("Should have returned failure.");
+        } catch (FailingHttpStatusCodeException e) {
+            // This really shouldn't return 500, but that's what it does now.
+            assertThat(e.getStatusCode(), equalTo(500));
+ 
+            // This should have a different message, but this is the current behavior demonstrating the problem.
+            assertThat(e.getResponse().getContentAsString(), containsString("A problem occurred while processing the request."));
+        }
+ 
+        OldDataMonitor odm = ExtensionList.lookupSingleton(OldDataMonitor.class);
+        Map<Saveable, OldDataMonitor.VersionRange> data = odm.getData();
+ 
+        assertThat(data.size(), equalTo(0));
+ 
+        odm.doDiscard(null, null);
+ 
+        View view = j.getInstance().getView("nonexistent");
+ 
+        // The view should still be nonexistent, as we gave it a user and not a view.
+        assertNull("Should not have created view.", view);
+ 
+        User.AllUsers.scanAll();
+        boolean createUser = false;
+        User badUser = User.getById("foo", createUser);
+ 
+        assertNull("Should not have created user.", badUser);
+    }
+ 
+    @Test
+    @Issue("SECURITY-1923")
+    public void configDotXmlWithValidXmlAndBadField() throws Exception {
+        ListView view = new ListView("view1", j.jenkins);
+        j.jenkins.addView(view);
+ 
+        JenkinsRule.WebClient wc = j.createWebClient();
+        wc.login(CONFIGURATOR);
+        WebRequest req = new WebRequest(wc.createCrumbedUrl(String.format("%s/config.xml", view.getUrl())), HttpMethod.POST);
+        req.setAdditionalHeader("Content-Type", "application/xml");
+        req.setRequestBody(VALID_XML_BAD_FIELD_USER_XML);
+ 
+        try {
+            wc.getPage(req);
+            fail("Should have returned failure.");
+        } catch (FailingHttpStatusCodeException e) {
+            // This really shouldn't return 500, but that's what it does now.
+            assertThat(e.getStatusCode(), equalTo(500));
+        }
+ 
+        OldDataMonitor odm = ExtensionList.lookupSingleton(OldDataMonitor.class);
+        Map<Saveable, OldDataMonitor.VersionRange> data = odm.getData();
+ 
+        assertThat(data.size(), equalTo(0));
+ 
+        odm.doDiscard(null, null);
+ 
+        User.AllUsers.scanAll();
+        boolean createUser = false;
+        User badUser = User.getById("foo", createUser);
+ 
+        assertNull("Should not have created user.", badUser);
+    }
+ 
+    private static final String VALID_XML_BAD_FIELD_USER_XML =
+            "<hudson.model.User>\n" +
+                    "  <id>foo</id>\n" +
+                    "  <fullName>Foo User</fullName>\n" +
+                    "  <badField/>\n" +
+                    "</hudson.model.User>\n";
+ 
+    private static final String ORIGINAL_BAD_USER_XML =
+            "<hudson.model.User>\n" +
+                    "  <id>foo</id>\n" +
+                    "  <fullName>Foo User</fullName>\n" +
+                    "<hudson.model.User>\n";
+ 
+}

Improper handling of REST API XML deserialization errors in Jenkins

Affected versions

Details

The fix

References