Merge pull request #85 from jenkinsci-cert/security-371-staplerproxy

[SECURITY-371] Secure all administrative monitors
jenkinsci · Dec 20, 2016 · 6efcf6c · 6efcf6c
2 parents ce8a2d5 + 17b98d6
commit 6efcf6c
Show file tree

Hide file tree

Showing 12 changed files with 86 additions and 15 deletions.
diff --git a/core/src/main/java/hudson/diagnosis/HudsonHomeDiskUsageMonitor.java b/core/src/main/java/hudson/diagnosis/HudsonHomeDiskUsageMonitor.java
@@ -24,14 +24,14 @@
 package hudson.diagnosis;
 
 import hudson.model.AdministrativeMonitor;
-import jenkins.model.Jenkins;
 import hudson.model.AbstractModelObject;
 import hudson.Extension;
 import hudson.ExtensionPoint;
 import hudson.ExtensionList;
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.HttpResponses;
 import org.kohsuke.stapler.QueryParameter;
+import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import java.io.IOException;
 import java.util.List;
@@ -64,6 +64,7 @@ public String getDisplayName() {
     /**
      * Depending on whether the user said "yes" or "no", send him to the right place.
      */
+    @RequirePOST
     public HttpResponse doAct(@QueryParameter String no) throws IOException {
         if(no!=null) {
             disable(true);

diff --git a/core/src/main/java/hudson/diagnosis/OldDataMonitor.java b/core/src/main/java/hudson/diagnosis/OldDataMonitor.java
@@ -52,6 +52,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
+
 import jenkins.model.Jenkins;
 import org.acegisecurity.context.SecurityContext;
 import org.acegisecurity.context.SecurityContextHolder;

diff --git a/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java b/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java
@@ -36,6 +36,7 @@
 import java.util.logging.Logger;
 import jenkins.model.Jenkins;
 import org.kohsuke.stapler.Stapler;
+import org.kohsuke.stapler.interceptor.RequirePOST;
 
 /**
  * Looks out for a broken reverse proxy setup that doesn't rewrite the location header correctly.
@@ -85,6 +86,7 @@ public void getTestForReverseProxySetup(String rest) {
     /**
      * Depending on whether the user said "yes" or "no", send him to the right place.
      */
+    @RequirePOST
     public HttpResponse doAct(@QueryParameter String no) throws IOException {
         if(no!=null) { // dismiss
             disable(true);

diff --git a/core/src/main/java/hudson/diagnosis/TooManyJobsButNoView.java b/core/src/main/java/hudson/diagnosis/TooManyJobsButNoView.java
@@ -28,6 +28,7 @@
 import hudson.Extension;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
+import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import java.io.IOException;
 
@@ -49,6 +50,7 @@ public boolean isActivated() {
     /**
      * Depending on whether the user said "yes" or "no", send him to the right place.
      */
+    @RequirePOST
     public void doAct(StaplerRequest req, StaplerResponse rsp) throws IOException {
         if(req.hasParameter("no")) {
             disable(true);

diff --git a/core/src/main/java/hudson/model/AdministrativeMonitor.java b/core/src/main/java/hudson/model/AdministrativeMonitor.java
@@ -34,8 +34,12 @@
 import java.io.IOException;
 
 import jenkins.model.Jenkins;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+import org.kohsuke.stapler.StaplerProxy;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
+import org.kohsuke.stapler.interceptor.RequirePOST;
 
 /**
  * Checks the health of a subsystem of Jenkins and if there's something
@@ -75,7 +79,7 @@
  * @see Jenkins#administrativeMonitors
  */
 @LegacyInstancesAreScopedToHudson
-public abstract class AdministrativeMonitor extends AbstractModelObject implements ExtensionPoint {
+public abstract class AdministrativeMonitor extends AbstractModelObject implements ExtensionPoint, StaplerProxy {
     /**
      * Human-readable ID of this monitor, which needs to be unique within the system.
      *
@@ -143,12 +147,21 @@ public boolean isEnabled() {
     /**
      * URL binding to disable this monitor.
      */
+    @RequirePOST
     public void doDisable(StaplerRequest req, StaplerResponse rsp) throws IOException {
-        Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);
         disable(true);
         rsp.sendRedirect2(req.getContextPath()+"/manage");
     }
 
+    /**
+     * Requires ADMINISTER permission for any operation in here.
+     */
+    @Restricted(NoExternalUse.class)
+    public Object getTarget() {
+        Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);
+        return this;
+    }
+
     /**
      * All registered {@link AdministrativeMonitor} instances.
      */

diff --git a/core/src/main/java/jenkins/diagnostics/SecurityIsOffMonitor.java b/core/src/main/java/jenkins/diagnostics/SecurityIsOffMonitor.java
@@ -5,6 +5,7 @@
 import jenkins.model.Jenkins;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
+import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import java.io.IOException;
 
@@ -27,6 +28,7 @@ public boolean isActivated() {
     /**
      * Depending on whether the user said "yes" or "no", send him to the right place.
      */
+    @RequirePOST
     public void doAct(StaplerRequest req, StaplerResponse rsp) throws IOException {
         if(req.hasParameter("no")) {
             disable(true);

diff --git a/core/src/main/java/jenkins/security/RekeySecretAdminMonitor.java b/core/src/main/java/jenkins/security/RekeySecretAdminMonitor.java
@@ -11,7 +11,6 @@
 import jenkins.model.Jenkins;
 import jenkins.util.io.FileBoolean;
 import org.kohsuke.stapler.HttpResponse;
-import org.kohsuke.stapler.StaplerProxy;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
@@ -29,7 +28,7 @@
  * @author Kohsuke Kawaguchi
  */
 @Extension
-public class RekeySecretAdminMonitor extends AsynchronousAdministrativeMonitor implements StaplerProxy {
+public class RekeySecretAdminMonitor extends AsynchronousAdministrativeMonitor {
 
     /**
      * Whether we detected a need to run the rewrite program.
@@ -62,14 +61,6 @@ &&  new FileBoolean(new File(j.getRootDir(),"secret.key.not-so-secret")).isOff()
             needed.on();
     }
 
-    /**
-     * Requires ADMINISTER permission for any operation in here.
-     */
-    public Object getTarget() {
-        Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);
-        return this;
-    }
-
     @Override
     public boolean isActivated() {
         return needed.isOn();

diff --git a/core/src/main/java/jenkins/security/s2m/AdminCallableMonitor.java b/core/src/main/java/jenkins/security/s2m/AdminCallableMonitor.java
@@ -8,6 +8,7 @@
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.HttpResponses;
 import org.kohsuke.stapler.QueryParameter;
+import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import javax.inject.Inject;
 import java.io.IOException;
@@ -49,6 +50,7 @@ public AdminWhitelistRule getRule() {
     /**
      * Depending on whether the user said "examin" or "dismiss", send him to the right place.
      */
+    @RequirePOST
     public HttpResponse doAct(@QueryParameter String dismiss) throws IOException {
         if(dismiss!=null) {
             disable(true);

diff --git a/core/src/main/java/jenkins/security/s2m/MasterKillSwitchWarning.java b/core/src/main/java/jenkins/security/s2m/MasterKillSwitchWarning.java
@@ -5,6 +5,7 @@
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.HttpResponses;
 import org.kohsuke.stapler.QueryParameter;
+import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import javax.inject.Inject;
 import java.io.IOException;
@@ -28,6 +29,7 @@ public boolean isActivated() {
         return rule.getMasterKillSwitch() && config.isRelevant();
     }
 
+    @RequirePOST
     public HttpResponse doAct(@QueryParameter String dismiss) throws IOException {
         if(dismiss!=null) {
             disable(true);

diff --git a/core/src/main/resources/hudson/diagnosis/HudsonHomeDiskUsageMonitor/index.jelly b/core/src/main/resources/hudson/diagnosis/HudsonHomeDiskUsageMonitor/index.jelly
@@ -24,7 +24,7 @@ THE SOFTWARE.
 
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt">
-	<l:layout title="${%JENKINS_HOME is almost full}" permission="${app.ADMINISTER}">
+	<l:layout title="${%JENKINS_HOME is almost full}">
 		<l:main-panel>
       <h1>
         <l:icon class="icon-warning icon-xlg"/>

diff --git a/core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly b/core/src/main/resources/hudson/diagnosis/OldDataMonitor/manage.jelly
@@ -24,7 +24,7 @@ THE SOFTWARE.
 
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
-<l:layout title="${%Manage Old Data}" permission="${app.ADMINISTER}">
+<l:layout title="${%Manage Old Data}">
   <st:include page="sidepanel.jelly" it="${app}"/>
   <l:main-panel>
     <h1>${%Manage Old Data}</h1>

diff --git a/test/src/test/java/hudson/diagnosis/HudsonHomeDiskUsageMonitorTest.java b/test/src/test/java/hudson/diagnosis/HudsonHomeDiskUsageMonitorTest.java
@@ -1,18 +1,31 @@
 package hudson.diagnosis;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import com.gargoylesoftware.htmlunit.util.NameValuePair;
+import hudson.model.User;
+import hudson.security.GlobalMatrixAuthorizationStrategy;
+import hudson.security.HudsonPrivateSecurityRealm;
+import hudson.security.Permission;
+import jenkins.model.Jenkins;
+import org.acegisecurity.context.SecurityContextHolder;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.xml.sax.SAXException;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.ElementNotFoundException;
 
 import java.io.IOException;
+import java.util.Collections;
 
 /**
  * @author Kohsuke Kawaguchi
@@ -45,6 +58,48 @@ public void flow() throws Exception {
         }
     }
 
+    @Issue("SECURITY-371")
+    @Test
+    public void noAccessForNonAdmin() throws Exception {
+        JenkinsRule.WebClient wc = j.createWebClient();
+
+        // TODO: Use MockAuthorizationStrategy in later versions
+        JenkinsRule.DummySecurityRealm realm = j.createDummySecurityRealm();
+        realm.addGroups("administrator", "admins");
+        realm.addGroups("bob", "users");
+        j.jenkins.setSecurityRealm(realm);
+        GlobalMatrixAuthorizationStrategy auth = new GlobalMatrixAuthorizationStrategy();
+        auth.add(Jenkins.ADMINISTER, "admins");
+        auth.add(Jenkins.READ, "users");
+        j.jenkins.setAuthorizationStrategy(auth);
+
+        WebRequest request = new WebRequest(wc.createCrumbedUrl("administrativeMonitor/hudsonHomeIsFull/act"), HttpMethod.POST);
+        NameValuePair param = new NameValuePair("no", "true");
+        request.setRequestParameters(Collections.singletonList(param));
+
+        HudsonHomeDiskUsageMonitor mon = HudsonHomeDiskUsageMonitor.get();
+
+        try {
+            wc.login("bob");
+            wc.getPage(request);
+        } catch (FailingHttpStatusCodeException e) {
+            assertEquals(403, e.getStatusCode());
+        }
+        assertTrue(mon.isEnabled());
+
+        try {
+            WebRequest getIndex = new WebRequest(wc.createCrumbedUrl("administrativeMonitor/hudsonHomeIsFull"), HttpMethod.GET);
+            wc.getPage(getIndex);
+        } catch (FailingHttpStatusCodeException e) {
+            assertEquals(403, e.getStatusCode());
+        }
+
+        wc.login("administrator");
+        wc.getPage(request);
+        assertFalse(mon.isEnabled());
+
+    }
+
     /**
      * Gets the warning form.
      */