Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-24513

Zero executors on master not well documented or enforced

    • 2.289.1, 2.286

      As described here:

      http://www.labofapenetrationtester.com/2014/08/script-execution-and-privilege-esc-jenkins.html

      A user with "configure" privileges can execute arbitrary code in the context of the application server running jenkins, and leverage this to bypass authentication and take full control of the jenkins server. This is only a problem because the security matrix seems to be designed to separate privileges, and the fact a user with "configure" privs for a single project can take over the whole server is non-obvious to administrators.

      Do you think this is something that constitutes a legitimate flaw to fix? Or more just something to be documented?

          [JENKINS-24513] Zero executors on master not well documented or enforced

          Daniel Beck added a comment -

          From JENKINS-33555 in response to the suggestion to make 0 master executors the default in Jenkins:

          But as to what you're suggesting, the problem is that actual default config changes in this area leave Jenkins non-functional as it comes out of the box. We've already disabled the JNLP port by default which causes some confusion ("Where's the JNLP Launcher?").

          Note that plugins such as Job Restrictions Plugin, Ownership Plugin, and Authorize Project Plugin (IIRC) can all be used to limit job execution on master even when executors are configured.

          We could probably achieve something resembling reasonable configurations by recommending build agents (first), and then, once they have set those up, recommend not building on master (second), via an admin monitor. Or at least tell people what they're doing isn't a great idea.

          Daniel Beck added a comment - From JENKINS-33555 in response to the suggestion to make 0 master executors the default in Jenkins: But as to what you're suggesting, the problem is that actual default config changes in this area leave Jenkins non-functional as it comes out of the box. We've already disabled the JNLP port by default which causes some confusion ("Where's the JNLP Launcher?"). Note that plugins such as Job Restrictions Plugin, Ownership Plugin, and Authorize Project Plugin (IIRC) can all be used to limit job execution on master even when executors are configured. We could probably achieve something resembling reasonable configurations by recommending build agents (first), and then, once they have set those up, recommend not building on master (second), via an admin monitor. Or at least tell people what they're doing isn't a great idea.

          Jesse Glick added a comment -

          Well what we would really want is something along the lines of

          diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
          index 2552718..9740635 100644
          --- a/core/src/main/java/jenkins/model/Jenkins.java
          +++ b/core/src/main/java/jenkins/model/Jenkins.java
          @@ -4713,13 +4713,26 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
                   }
           
                   @Override
          -        public boolean hasPermission(Permission permission) {
          -            // no one should be allowed to delete the master.
          -            // this hides the "delete" link from the /computer/(master) page.
          -            if(permission==Computer.DELETE)
          -                return false;
          -            // Configuration of master node requires ADMINISTER permission
          -            return super.hasPermission(permission==Computer.CONFIGURE ? Jenkins.ADMINISTER : permission);
          +        public ACL getACL() {
          +            final ACL base = super.getACL();
          +            return new ACL() {
          +                @Override
          +                public boolean hasPermission(Authentication a, Permission permission) {
          +                    // no one should be allowed to delete the master.
          +                    // this hides the "delete" link from the /computer/(master) page.
          +                    if (permission == Computer.DELETE) {
          +                        return false;
          +                    } else if (permission == Computer.CONFIGURE) {
          +                        // Configuration of master node requires ADMINISTER permission
          +                        return base.hasPermission(a, Jenkins.ADMINISTER);
          +                    } else if (permission == Computer.BUILD) {
          +                        // JENKINS-24513: do not allow user-configured builds on the master.
          +                        return base.hasPermission(a, Jenkins.RUN_SCRIPTS);
          +                    } else {
          +                        return base.hasPermission(a, permission);
          +                    }
          +                }
          +            };
                   }
           
                   @Override
          

          and then including Authorize Project in the default plugin set and having it preconfigure a sensible queue authenticator. Might also need something like

          diff --git a/core/src/main/java/hudson/model/Node.java b/core/src/main/java/hudson/model/Node.java
          index d7c4071..6742918 100644
          --- a/core/src/main/java/hudson/model/Node.java
          +++ b/core/src/main/java/hudson/model/Node.java
          @@ -386,7 +386,7 @@ public abstract class Node extends AbstractModelObject implements Reconfigurable
                   }
           
                   Authentication identity = item.authenticate();
          -        if (!getACL().hasPermission(identity,Computer.BUILD)) {
          +        if (!(item.task instanceof Queue.FlyweightTask) && !getACL().hasPermission(identity,Computer.BUILD)) {
                       // doesn't have a permission
                       return CauseOfBlockage.fromMessage(Messages._Node_LackingBuildPermission(identity.getName(),getNodeName()));
                   }
          diff --git a/core/src/main/java/hudson/model/queue/MappingWorksheet.java b/core/src/main/java/hudson/model/queue/MappingWorksheet.java
          index 4ef5af3..b8c2adf 100644
          --- a/core/src/main/java/hudson/model/queue/MappingWorksheet.java
          +++ b/core/src/main/java/hudson/model/queue/MappingWorksheet.java
          @@ -30,6 +30,7 @@ import hudson.model.Executor;
           import hudson.model.Label;
           import hudson.model.LoadBalancer;
           import hudson.model.Node;
          +import hudson.model.Queue;
           import hudson.model.Queue.BuildableItem;
           import hudson.model.Queue.Executable;
           import hudson.model.Queue.JobOffer;
          @@ -133,7 +134,7 @@ public class MappingWorksheet {
                       if (c.assignedLabel!=null && !c.assignedLabel.contains(node))
                           return false;   // label mismatch
           
          -            if (!nodeAcl.hasPermission(item.authenticate(), Computer.BUILD))
          +            if (!(item.task instanceof Queue.FlyweightTask) && !nodeAcl.hasPermission(item.authenticate(), Computer.BUILD))
                           return false;   // tasks don't have a permission to run on this node
           
                       return true;
          

          to ensure this is not applied overzealously.

          Jesse Glick added a comment - Well what we would really want is something along the lines of diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 2552718..9740635 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -4713,13 +4713,26 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve } @Override - public boolean hasPermission(Permission permission) { - // no one should be allowed to delete the master. - // this hides the "delete" link from the /computer/(master) page. - if (permission==Computer.DELETE) - return false ; - // Configuration of master node requires ADMINISTER permission - return super .hasPermission(permission==Computer.CONFIGURE ? Jenkins.ADMINISTER : permission); + public ACL getACL() { + final ACL base = super .getACL(); + return new ACL() { + @Override + public boolean hasPermission(Authentication a, Permission permission) { + // no one should be allowed to delete the master. + // this hides the "delete" link from the /computer/(master) page. + if (permission == Computer.DELETE) { + return false ; + } else if (permission == Computer.CONFIGURE) { + // Configuration of master node requires ADMINISTER permission + return base.hasPermission(a, Jenkins.ADMINISTER); + } else if (permission == Computer.BUILD) { + // JENKINS-24513: do not allow user-configured builds on the master. + return base.hasPermission(a, Jenkins.RUN_SCRIPTS); + } else { + return base.hasPermission(a, permission); + } + } + }; } @Override and then including Authorize Project in the default plugin set and having it preconfigure a sensible queue authenticator. Might also need something like diff --git a/core/src/main/java/hudson/model/Node.java b/core/src/main/java/hudson/model/Node.java index d7c4071..6742918 100644 --- a/core/src/main/java/hudson/model/Node.java +++ b/core/src/main/java/hudson/model/Node.java @@ -386,7 +386,7 @@ public abstract class Node extends AbstractModelObject implements Reconfigurable } Authentication identity = item.authenticate(); - if (!getACL().hasPermission(identity,Computer.BUILD)) { + if (!(item.task instanceof Queue.FlyweightTask) && !getACL().hasPermission(identity,Computer.BUILD)) { // doesn't have a permission return CauseOfBlockage.fromMessage(Messages._Node_LackingBuildPermission(identity.getName(),getNodeName())); } diff --git a/core/src/main/java/hudson/model/queue/MappingWorksheet.java b/core/src/main/java/hudson/model/queue/MappingWorksheet.java index 4ef5af3..b8c2adf 100644 --- a/core/src/main/java/hudson/model/queue/MappingWorksheet.java +++ b/core/src/main/java/hudson/model/queue/MappingWorksheet.java @@ -30,6 +30,7 @@ import hudson.model.Executor; import hudson.model.Label; import hudson.model.LoadBalancer; import hudson.model.Node; + import hudson.model.Queue; import hudson.model.Queue.BuildableItem; import hudson.model.Queue.Executable; import hudson.model.Queue.JobOffer; @@ -133,7 +134,7 @@ public class MappingWorksheet { if (c.assignedLabel!= null && !c.assignedLabel.contains(node)) return false ; // label mismatch - if (!nodeAcl.hasPermission(item.authenticate(), Computer.BUILD)) + if (!(item.task instanceof Queue.FlyweightTask) && !nodeAcl.hasPermission(item.authenticate(), Computer.BUILD)) return false ; // tasks don't have a permission to run on this node return true ; to ensure this is not applied overzealously.

          Daniel Beck added a comment -

          Seems to me that the core change should be fairly uncontroversial?

          Daniel Beck added a comment - Seems to me that the core change should be fairly uncontroversial?

          Jesse Glick added a comment -

          If you have not configured a queue item authenticator then builds which succeeded before would begin failing. Which could be considered a good thing—security is now being enforced—but on the other hand would be a nasty surprise in an upgrade.

          Jesse Glick added a comment - If you have not configured a queue item authenticator then builds which succeeded before would begin failing. Which could be considered a good thing—security is now being enforced—but on the other hand would be a nasty surprise in an upgrade.

          Jesse Glick added a comment -

          display an admin monitor when security is enabled yet there is no configured QueueItemAuthenticator

          This would be a reasonable follow-up to JENKINS-22949. For example:

          • include authorize-project in core/src/main/resources/jenkins/install/platform-plugins.json so people are guided to install it
          • display an admin monitor with varying messages when
            • there is no QueueItemAuthenticatorDescriptor; perhaps you need to install authorize-project?
            • there is no configured QueueItemAuthenticator; perhaps you need to configure it?
            • there is, but some jobs in the system are still set to run as SYSTEM; perhaps you need to configure a fallback authentication, such as to anonymous (or perhaps you deliberately set this job to be that way, in which case dismiss)

          Jesse Glick added a comment - display an admin monitor when security is enabled yet there is no configured QueueItemAuthenticator This would be a reasonable follow-up to  JENKINS-22949 . For example: include authorize-project in core/src/main/resources/jenkins/install/platform-plugins.json so people are guided to install it display an admin monitor with varying messages when there is no QueueItemAuthenticatorDescriptor ; perhaps you need to install authorize-project ? there is no configured QueueItemAuthenticator ; perhaps you need to configure it? there is, but some jobs in the system are still set to run as SYSTEM ; perhaps you need to configure a fallback authentication, such as to anonymous (or perhaps you deliberately set this job to be that way, in which case dismiss)

          Jesse Glick added a comment -

          danielbeck notes that flyweight Pipeline tasks are also currently checked for Computer.BUILD on master, which makes no sense.

          Jesse Glick added a comment - danielbeck notes that flyweight Pipeline tasks are also currently checked for Computer.BUILD on master, which makes no sense.

          Daniel Beck added a comment - - edited

          I got started on an admin monitor for Access Control for Builds.

          The problem I ran into was determining when to show it – on instances where everyone is an admin anyway, it's pointless.

          My first idea was a RunListener that checks its causes for a UserIdCause, and if that is not an admin, to trigger the admin monitor. But that seems too narrow of a condition, and we probably want to err on the side of showing it too often rather than too rarely.

          Perhaps it's good enough to check for the selected authorization strategy, and if it's not one of the three or so with clear privilege separation (and perhaps if there's at least two users), show it.

          Daniel Beck added a comment - - edited I got started on an admin monitor for Access Control for Builds . The problem I ran into was determining when to show it – on instances where everyone is an admin anyway, it's pointless. My first idea was a  RunListener that checks its causes for a UserIdCause, and if that is not an admin, to trigger the admin monitor. But that seems too narrow of a condition, and we probably want to err on the side of showing it too often rather than too rarely. Perhaps it's good enough to check for the selected authorization strategy, and if it's not one of the three or so with clear privilege separation (and perhaps if there's at least two users), show it.

          Oleg Nenashev added a comment -

          danielbeck's patches have been integrated to 2.168. Is it enough to close the issue?

          Oleg Nenashev added a comment - danielbeck 's patches have been integrated to 2.168. Is it enough to close the issue?

          Daniel Beck added a comment -

          Unsure. This is probably more of an epic, and I did it a disservice by referencing it directly for a specific change.

          Daniel Beck added a comment - Unsure. This is probably more of an epic, and I did it a disservice by referencing it directly for a specific change.

          With https://github.com/jenkinsci/jenkins/pull/5337, I think this ticket could be considered as done. If not, please re-open it

          Released in jenkins-2.289.1+ and jenkins-2.286+

          Wadeck Follonier added a comment - With https://github.com/jenkinsci/jenkins/pull/5337 , I think this ticket could be considered as done. If not, please re-open it Released in jenkins-2.289.1+ and jenkins-2.286+

            Unassigned Unassigned
            dfj David Jorm
            Votes:
            1 Vote for this issue
            Watchers:
            13 Start watching this issue

              Created:
              Updated:
              Resolved: