Correct permission checks to add

[Tim Van Holder]

Hi,

I added the Jenkins security scan to my plugin on GitHub and resolved all the issues.Or, so I thought.

One of the main classes of reported issues (aside from adding @POST to pretty much all Stapler doXXX methods) was the "no access check done".

While most of the methods I have (mostly doCheck and DoFill...Items) are very simple (no file/net/... access), I figured it would not hurt to add a basic check either.

For the bulk of the methods, typically involved in configuring a step, I did the following:

- add an @AncestorInPath Item item parameter to the method

- add if (item != null) item.checkPermission(Permission.CONFIGURE); near the top of the method.

This seemed to make sense - you'd need to have access to the Configure menu item to see the interface that calls those methods anyway

For those bits related to the global tool setup, I simply use Jenkins.get().checkPermission(Jenkins.MANAGE); instead. Again, this seems to make sense, given the tool setup lives among the Manage Jenkins options.

However, when I set up users and activate the matrix strategy, then a user who has the Job/Configure permission will get an Unauthorized error under one of the drop-downs, saying they do not have the N/A/GenericConfigure permission.

Does this mean I should be testing for FreeStyleProject.CONFIGURE instead (these Permission things seem rather hard to discover, I must say)? Or will that prevent using the same UI/methods in other contexts (e.g. what permissions make the pipeline syntax generator available? that uses the same interface as the freestyle project configuration)?

Am I right in using checkPermission, or should I be using checkAnyPermission to check for multiples (say Permission.CONFIGURE, FreeStyleProject,CONFIGURE, Jenkins.MANAGE)?

Or should I be using has(Any)Permission() and simply fail silently (i.e. return OK validation, or filling no items)?

If the suggestion is "just don't do any checking if there's no real need" then I would really appreciate a way to be able to declare that in code so that the security scan will not raise an issue for it. E.g. a @Unsecured annotation or an empty Jenkins.noPermissionsNeeded() method that will satisfy the "Stapler methods must check access rights" requirement.

Come to that, if a method doesn't really need @POST, is there any harm (e.g. a perf hit) in adding it? If so, maybe a @GET/@NoPOST annotation to say "this does not need @POST, stop bugging me" would be nice too.

Security scans become less useful if they mostly produce false positives.

Thanks in advance for any guidance.

[wfoll...@cloudbees.com]

Hello, 

Superficial read => I imagine you should use Item.CONFIGURE and not the generic Permission.CONFIGURE.

Wadeck

[Daniel Beck]

On Thu, Apr 28, 2022 at 1:12 AM Tim Van Holder <tim.va...@gmail.com> wrote:

For those bits related to the global tool setup, I simply use Jenkins.get().checkPermission(Jenkins.MANAGE); instead. Again, this seems to make sense, given the tool setup lives among the Manage Jenkins options.

Just in case, please note that Manage is a lesser permission than Administer. Most stuff in Jenkins is (still) requiring Jenkins.ADMINISTER, and the UI needs to be adapted too for options to be available to users with "only" Manage. Some options are also unsafe to make available this way and it's not always obviou. See https://github.com/jenkinsci/jep/tree/master/jep/223 

 

Does this mean I should be testing for FreeStyleProject.CONFIGURE instead (these Permission things seem rather hard to discover, I must say)? Or will that prevent using the same UI/methods in other contexts (e.g. what permissions make the pipeline syntax generator available? that uses the same interface as the freestyle project configuration)?

Yes, that's correct (well, I'd use Item.CONFIGURE but it's the same thing).

Generic permissions can be granted, but should never be checked. A generic permission is any permission never shown in the matrix-auth table. Note that some permissions may be missing from there, like Item/Artifacts or Overall/Manage, which are optional and disabled-by-default permissions, and show up once enabled.

 

Am I right in using checkPermission, or should I be using checkAnyPermission to check for multiples (say Permission.CONFIGURE, FreeStyleProject,CONFIGURE, Jenkins.MANAGE)?

Probably not, because Overall/Manage is granted globally (i.e. you'd check it on Jenkins.get() ), while Item/Configure is granted on an Item, which you'd get from AncestorInPath in the usual Descriptor form validation methods. But there are cases where the check/hasAnyPermission methods make sense. JEP-223/224 define global permissions that make some previously admin-only stuff available to users with lesser permissions, and so core has some code to show UI to users that have any of Overall/Manage or Overall/SystemRead.

 

Or should I be using has(Any)Permission() and simply fail silently (i.e. return OK validation, or filling no items)?

Yes, this is frequently the better UX especially for doCheck/doTest/doFill methods.

One related complication for permission checks is for Pipeline-compatible steps (which you mentioned before). If someone opens the Snippet Generator, there may not be an @AncestorInPath Item, or have the permission you expect, but you'd still want the user to be able to generate something, because ultimately it ends up in the Jenkinsfile anyway. Fully JCasC configured instances come to mind here, where no actual user may be an admin or allowed to configure jobs. No need to return real data though if you don't have to.

 

If the suggestion is "just don't do any checking if there's no real need" then I would really appreciate a way to be able to declare that in code so that the security scan will not raise an issue for it. E.g. a @Unsecured annotation or an empty Jenkins.noPermissionsNeeded() method that will satisfy the "Stapler methods must check access rights" requirement.

This is a known limitation, tracked in https://github.com/jenkins-infra/jenkins-codeql/issues/4

Meanwhile, you can mark findings as invalid through the GitHub UI (unfortunately without an explanation).

 

Come to that, if a method doesn't really need @POST, is there any harm (e.g. a perf hit) in adding it?

If the UI element sends POST anyway, there's no drawback to adding it (besides it being more annoying to trigger manually e.g. during development -- choose RequirePOST if that's a likely use case).

 

If so, maybe a @GET/@NoPOST annotation to say "this does not need @POST, stop bugging me" would be nice too.

Security scans become less useful if they mostly produce false positives.

It's very difficult to distinguish between methods that need protections (permission checks or POST requirement) from those that don't. I attempted to exclude some obvious cases of harmless methods, but it can always be improved! See https://github.com/jenkins-infra/jenkins-codeql/blob/main/jenkins/java/ql/src/declarations/NonTrivialInvocation.qll and feel free to suggest additions (via PR or issues)