[SECURITY-1923]

Co-authored-by: NDaniel Beck <daniel-beck@users.noreply.github.com>

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

@@ -57,7 +57,12 @@ import java.util.logging.Level;
 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 @@ import jenkins.util.xstream.CriticalXStreamException;
 @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 class RobustReflectionConverter implements Converter {
             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 class RobustReflectionConverter implements Converter {
         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

+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

+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

+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

+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";
+
+}