I was using the "From users with Admin or Write" permission for "Discover pull requests from forks". A random user opened a PR from their fork against our github.com/keybase/kbfs repository, and Jenkins built it. I'd have expected Jenkins not to build it.

      The description:

      Pull requests forks will be treated as trusted if and only if the fork owner has either Admin or Write permissions on the origin repository. Note that this strategy requires the Review a user's permission level API, as a result on GitHub Enterprise Server versions before 2.12 this is the same as trusting Nobody.

       

      Our repositories are open source, so we allow anyone to see them. But only a small set of users have Admin or Write permissions. And yet we had a random user create a pull request, and it got built by Jenkins.

      I've now set the fork permission to "Nobody".

          [JENKINS-48848] Discover permissions check doesn't work

          Andrew Bayer added a comment -

          This is an interesting one - the implementation is actually intended to block running any changes in the Jenkinsfile from an untrusted fork, instead using the merge target's Jenkinsfile unchanged, rather than blocking building of an untrusted fork's PR entirely. But that does seem to be a worthwhile feature to consider adding.

          Andrew Bayer added a comment - This is an interesting one - the implementation is actually intended to block running any changes in the Jenkinsfile from an untrusted fork, instead using the merge target's Jenkinsfile unchanged, rather than blocking building of an untrusted fork's PR entirely. But that does seem to be a worthwhile feature to consider adding.

          John Zila added a comment -

          Yeah that makes sense. It has several problems, however:
          1. It's difficult to discover what is happening. It seems like it shouldn't build, but instead it builds? And it's not easy to determine whether it's building from the merge target's Jenkinsfile or the branch one.

          2. Jenkinsfiles are hardly the only security risk for forked builds. Just echoing an environment variable from a modified source file can be hugely damaging, potentially revealing secrets. Not to mention the ability to curl out and/or execute arbitrary code as the jenkins user on a build machine.

          John Zila added a comment - Yeah that makes sense. It has several problems, however: 1. It's difficult to discover what is happening. It seems like it shouldn't build, but instead it builds? And it's not easy to determine whether it's building from the merge target's Jenkinsfile or the branch one. 2. Jenkinsfiles are hardly the only security risk for forked builds. Just echoing an environment variable from a modified source file can be hugely damaging, potentially revealing secrets. Not to mention the ability to curl out and/or execute arbitrary code as the jenkins user on a build machine.

          Sam Schwarz added a comment -

          I made Jira issues to request this to be a feature and to change the documentation to be more clear about the actual functionality. https://issues.jenkins-ci.org/browse/JENKINS-53753 and https://issues.jenkins-ci.org/browse/JENKINS-53752

          Sam Schwarz added a comment - I made Jira issues to request this to be a feature and to change the documentation to be more clear about the actual functionality. https://issues.jenkins-ci.org/browse/JENKINS-53753 and https://issues.jenkins-ci.org/browse/JENKINS-53752

          John Zila added a comment -

          Thank you!

          John Zila added a comment - Thank you!

          Some of our repos were attacked yesterday by malicous PRs from Forks that modified our POM files (not the Jenkinsfile) to download and run a rogue shell script.  We used the default settings yet the PR build ran anyway.  

          The recommended setting should be "nobody" until this is fixed...

          Robert Patrick added a comment - Some of our repos were attacked yesterday by malicous PRs from Forks that modified our POM files (not the Jenkinsfile) to download and run a rogue shell script.  We used the default settings yet the PR build ran anyway.   The recommended setting should be "nobody" until this is fixed...

            Unassigned Unassigned
            jzila John Zila
            Votes:
            8 Vote for this issue
            Watchers:
            9 Start watching this issue

              Created:
              Updated: