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

Zero executors on master not well documented or enforced

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      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?

        Attachments

          Issue Links

            Activity

            dfj David Jorm created issue -
            Hide
            jglick Jesse Glick added a comment -

            Currently this is just something to be documented, that a secure installation should have zero executors on master (or otherwise have restricted jobs able to run on master to only those which users who already have Overall/RunScripts may configure).

            Better would be for this to be an administrative warning. Or Jenkins could actually check the authentication associated with a build done on master to verify that it has RUN_SCRIPTS.

            Show
            jglick Jesse Glick added a comment - Currently this is just something to be documented, that a secure installation should have zero executors on master (or otherwise have restricted jobs able to run on master to only those which users who already have Overall/RunScripts may configure). Better would be for this to be an administrative warning. Or Jenkins could actually check the authentication associated with a build done on master to verify that it has RUN_SCRIPTS .
            jglick Jesse Glick made changes -
            Field Original Value New Value
            Assignee Kohsuke Kawaguchi [ kohsuke ]
            Issue Type Bug [ 1 ] Improvement [ 4 ]
            Summary Potential privilege escalation issue Zero executors on master not well documented or enforced
            Hide
            kohsuke Kohsuke Kawaguchi added a comment -

            I agree with Jesse. Given also that the referenced post is already public, I'm moving this into to general JENKINS project.

            Show
            kohsuke Kohsuke Kawaguchi added a comment - I agree with Jesse. Given also that the referenced post is already public, I'm moving this into to general JENKINS project.
            kohsuke Kohsuke Kawaguchi made changes -
            Component/s core [ 15593 ]
            Component/s core [ 15738 ]
            Key SECURITY-156 JENKINS-24513
            Project Security Issues [ 10180 ] Jenkins [ 10172 ]
            Workflow Security v1.2 [ 157284 ] JNJira [ 157499 ]
            Status Untriaged [ 10001 ] Open [ 1 ]
            Hide
            danielbeck Daniel Beck added a comment -

            Jenkins allows users with configure privileges to run arbitrary shell or batch scripts as the user Jenkins runs as (bonus: When using the Windows installer, it runs as SYSTEM).

            How is this NOT obvious?

            Show
            danielbeck Daniel Beck added a comment - Jenkins allows users with configure privileges to run arbitrary shell or batch scripts as the user Jenkins runs as (bonus: When using the Windows installer, it runs as SYSTEM). How is this NOT obvious?
            Hide
            jglick Jesse Glick added a comment - - edited

            Well in practice it is not so obvious.

            Also there are some use cases for administrators to configure special projects that must run on the master, to do backups and the like. For these cases you want to leave some executor slots open, yet block regular projects from using them. The natural way to do that would be to have Jenkins.MasterComputer.checkPermission(Computer.BUILD) require RUN_SCRIPTS on Jenkins, so that only admin-authorized projects could run on it; and then display an admin monitor when security is enabled yet there is no configured QueueItemAuthenticator.

            Show
            jglick Jesse Glick added a comment - - edited Well in practice it is not so obvious. Also there are some use cases for administrators to configure special projects that must run on the master, to do backups and the like. For these cases you want to leave some executor slots open, yet block regular projects from using them. The natural way to do that would be to have Jenkins.MasterComputer.checkPermission(Computer.BUILD) require RUN_SCRIPTS on Jenkins , so that only admin-authorized projects could run on it; and then display an admin monitor when security is enabled yet there is no configured QueueItemAuthenticator .
            jglick Jesse Glick made changes -
            Labels security
            jglick Jesse Glick made changes -
            Link This issue is related to JENKINS-30749 [ JENKINS-30749 ]
            jglick Jesse Glick made changes -
            Labels security 2.0 security
            kohsuke Kohsuke Kawaguchi made changes -
            Labels 2.0 security security
            rtyler R. Tyler Croy made changes -
            Workflow JNJira [ 157499 ] JNJira + In-Review [ 179555 ]
            jglick Jesse Glick made changes -
            Link This issue is duplicated by JENKINS-33555 [ JENKINS-33555 ]
            Hide
            danielbeck 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.

            Show
            danielbeck 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.
            Hide
            jglick 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.

            Show
            jglick 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.
            Hide
            danielbeck Daniel Beck added a comment -

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

            Show
            danielbeck Daniel Beck added a comment - Seems to me that the core change should be fairly uncontroversial?
            Hide
            jglick 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.

            Show
            jglick 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.
            jglick Jesse Glick made changes -
            Link This issue is duplicated by SECURITY-480 [ SECURITY-480 ]
            Hide
            jglick 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)
            Show
            jglick 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)
            jglick Jesse Glick made changes -
            Link This issue relates to JENKINS-22949 [ JENKINS-22949 ]
            Hide
            jglick Jesse Glick added a comment -

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

            Show
            jglick Jesse Glick added a comment - Daniel Beck notes that flyweight Pipeline tasks are also currently checked for Computer.BUILD on master, which makes no sense.
            danielbeck Daniel Beck made changes -
            Link This issue is related to JENKINS-46652 [ JENKINS-46652 ]
            oleg_nenashev Oleg Nenashev made changes -
            Assignee Oleg Nenashev [ oleg_nenashev ]
            jamesdumay James Dumay made changes -
            Remote Link This issue links to "CloudBees Internal OSS-2267 (Web Link)" [ 18367 ]
            jglick Jesse Glick made changes -
            Link This issue is duplicated by JENKINS-49861 [ JENKINS-49861 ]
            jglick Jesse Glick made changes -
            Labels security essentials security
            rtyler R. Tyler Croy made changes -
            Labels essentials security security
            Hide
            danielbeck 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.

            Show
            danielbeck 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 Oleg Nenashev made changes -
            Assignee Oleg Nenashev [ oleg_nenashev ]
            oleg_nenashev Oleg Nenashev made changes -
            Labels security security user-experience
            jglick Jesse Glick made changes -
            Remote Link This issue links to "jenkins PR 3919 (Web Link)" [ 22436 ]
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

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

            Show
            oleg_nenashev Oleg Nenashev added a comment - Daniel Beck 's patches have been integrated to 2.168. Is it enough to close the issue?
            Hide
            danielbeck 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.

            Show
            danielbeck 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.
            jglick Jesse Glick made changes -
            Link This issue relates to JENKINS-56617 [ JENKINS-56617 ]

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              dfj David Jorm
              Votes:
              1 Vote for this issue
              Watchers:
              12 Start watching this issue

                Dates

                Created:
                Updated: