Changes for Security check #98

sangamchitmugre · 2022-04-15T12:04:01Z

@Kevin-CB
We have observed

There few methods which are only for Global for which we have added a check :- Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);

There are few methods for only job level validation for which we have added a check with @AncestorInPath annotation :-
if(item==null)
{
Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);
}else {
item.checkPermission(Item.CONFIGURE);
}

There are few methods which is common for global and job level so for that we have added check with @AncestorInPath :-
if(item==null)
{
Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);
}else {
item.checkPermission(Item.CONFIGURE);
}

As per your documentation we have added these checks so please review these changes

Changes for Security check

Kevin-CB · 2022-04-19T07:48:50Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

@@ -2738,7 +2773,7 @@ public ListBoxModel doFillPresetItems(@QueryParameter final boolean useOwnServer
                                              @QueryParameter final String username, @QueryParameter final String password,
                                              @QueryParameter final String timestamp, @QueryParameter final String credentialsId,
                                              @QueryParameter final boolean isProxy, @AncestorInPath Item item) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+            item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if(item==null){

return FormValidation.ok();

}else {

item.checkPermission(Item.CONFIGURE);

}

Please, always check if the context is no null to avoid any error.

Kevin-CB · 2022-04-19T07:49:03Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

@@ -2774,7 +2809,7 @@ public ListBoxModel doFillPresetItems(@QueryParameter final boolean useOwnServer
         */
        @POST
        public FormValidation doCheckFullScanCycle(@QueryParameter final int value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+            item.checkPermission(Item.CONFIGURE);


Kevin-CB · 2022-04-19T07:49:09Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

@@ -2787,7 +2822,7 @@ public ListBoxModel doFillSourceEncodingItems(@QueryParameter final boolean useO
                                                      @QueryParameter final String username, @QueryParameter final String password,
                                                      @QueryParameter final String timestamp, @QueryParameter final String credentialsId,
                                                      @QueryParameter final boolean isProxy, @AncestorInPath Item item) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+            item.checkPermission(Item.CONFIGURE);


Kevin-CB · 2022-04-19T07:49:13Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

@@ -2827,7 +2862,7 @@ public ListBoxModel doFillGroupIdItems(@QueryParameter final boolean useOwnServe
                                               @QueryParameter final String username, @QueryParameter final String password,
                                               @QueryParameter final String timestamp, @QueryParameter final String credentialsId,
                                               @QueryParameter final boolean isProxy, @AncestorInPath Item item) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+            item.checkPermission(Item.CONFIGURE);


Kevin-CB · 2022-04-19T07:49:17Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public ListBoxModel doFillFailBuildOnNewSeverityItems() {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public ListBoxModel doFillFailBuildOnNewSeverityItems(@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Kevin-CB · 2022-04-19T07:49:34Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckLowThreshold(@QueryParameter final Integer value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckLowThreshold(@QueryParameter final Integer value,@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Kevin-CB · 2022-04-19T07:49:43Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckOsaHighThreshold(@QueryParameter final Integer value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckOsaHighThreshold(@QueryParameter final Integer value,@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Kevin-CB · 2022-04-19T07:49:48Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckOsaMediumThreshold(@QueryParameter final Integer value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckOsaMediumThreshold(@QueryParameter final Integer value,@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Kevin-CB · 2022-04-19T07:49:53Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckOsaLowThreshold(@QueryParameter final Integer value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckOsaLowThreshold(@QueryParameter final Integer value,@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Kevin-CB · 2022-04-19T07:51:42Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

@@ -2305,7 +2305,6 @@ public final void setHideDebugLogs(boolean hideDebugLogs) {
        @POST
        public FormValidation doCheckScanTimeoutDuration(@QueryParameter final Integer value) {
            Jenkins.getInstance().checkPermission(Item.CONFIGURE);


You're still checking the global Configure permission for this one, please add the @AncestorInPath annotation and check on the current context.

This field is on the global configuration page only. Do we have to add the context check ?

I have added the @AncestorInPath annotation and Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);

My bad on this one, I didn't check for the moment where they're called, but yeah, field called on global configuration page, only need Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER);.
No need for @AncestorInPath annotation or Item.Configure permission

Also, don't forget to push your commit, otherwise I can't review you changes 😄

Kevin-CB · 2022-04-20T13:27:13Z

Also you're calling FormValidation a lot of times without returning it.

Kevin-CB

You need to check if the context is not null

Kevin-CB · 2022-05-03T08:03:36Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckScaSASTProjectID(@QueryParameter String value, @QueryParameter String scaSASTProjectFullPath) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckScaSASTProjectID(@QueryParameter String value, @QueryParameter String scaSASTProjectFullPath,@AncestorInPath Item item) {
+                item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Please check if the context is not null

Kevin-CB · 2022-05-03T08:03:41Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckCustomFields(@QueryParameter String value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckCustomFields(@QueryParameter String value,@AncestorInPath Item item) {
+                item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Please check if the context is not null

@Kevin-CB , are you saying for all job level parameters , we need to add the context check like we have added for all common parameters? But when we debugged the code for job level parameters the item is never coming as null it has value either a FreeStyleJobProject or PipelineJobProject.

Hello @SubhadraSahoo, indeed to avoid any exception, we prefer that the case of a null context is always taken into account.
Because all web methods can be called from various Item.

Take for example this web method doCheckCustomFields, it can be called by its path: descriptorByName/com.checkmarx.jenkins.CxScanBuilder/checkCustomFields.

eg:
[JENKINS_INSTANCE]/descriptorByName/com.checkmarx.jenkins.CxScanBuilder/checkCustomFields (context is null) or
[JENKINS_INSTANCE]/job/[JOB_NAME]descriptorByName/com.checkmarx.jenkins.CxScanBuilder/checkCustomFields (the job is your context)

@Kevin-CB Two questions.

Isn't it an issue that Jenkins allowing access to doCheckCustomFields method via descriptor, when the function is only for the Job level field and there is no field by that name on the descriptor (global)?

Why an null pointer exception a concern for the security issue we are trying to fix?

Hello @umeshwaghode,

No this is not a issue, it's used for many features, it's just necessary to implement the right permission checks when using them.

We're just trying to spread the best practices to prevent bad ones from spreading throughout the ecosystem. Many maintainers copy already existing code.

"It is used in many features" does not explain how a different path should gives access to a function that is not meant for that path. I find that odd.

That's how Jenkins (in conjunction with Stapler) is designed. Multiple URLs route to descriptor lists, as they may need to be available in multiple places. Even your build step's descriptor may offer global configuration options (through global.jelly) with related form validation, so not having the descriptor available at a global level would be a problem.

Thanks Daniel for clarifying this further. We will be making suggested changes as without those I do not see we getting this PR approved. But to clarify, all these places where (item == null) check is suggested in review comments, exist only on job and we have no intention to expose those in global.jelly.

@Kevin-CB , Hi Can you please remove the security warning from the Jenkins checkmarx plugin. The latest plugin 2022.2.3 is now published in the market place. Please let us know if anything else we need to do.

Kevin-CB · 2022-05-03T08:04:06Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckForceScan(@QueryParameter boolean value, @QueryParameter boolean incremental) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckForceScan(@QueryParameter boolean value, @QueryParameter boolean incremental,@AncestorInPath Item item) {
+                item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Please check if the context is not null

Kevin-CB · 2022-05-03T08:04:40Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckIncremental(@QueryParameter boolean value, @QueryParameter boolean forceScan) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckIncremental(@QueryParameter boolean value, @QueryParameter boolean forceScan,@AncestorInPath Item item) {
+                item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Please check if the context is not null

Kevin-CB · 2022-05-03T08:07:37Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

@@ -2651,7 +2662,7 @@ public ListBoxModel doFillPostScanActionIdItems(@QueryParameter final boolean us
                                                        @QueryParameter final String username, @QueryParameter final String password,
                                                        @QueryParameter final String timestamp, @QueryParameter final String credentialsId,
                                                        @QueryParameter final boolean isProxy, @AncestorInPath Item item) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+                item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Please check if the context is not null

Kevin-CB · 2022-05-03T08:13:42Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckLowThreshold(@QueryParameter final Integer value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckLowThreshold(@QueryParameter final Integer value,@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Kevin-CB · 2022-05-03T08:17:48Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckOsaHighThreshold(@QueryParameter final Integer value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckOsaHighThreshold(@QueryParameter final Integer value,@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Kevin-CB · 2022-05-03T08:18:00Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckOsaMediumThreshold(@QueryParameter final Integer value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckOsaMediumThreshold(@QueryParameter final Integer value,@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Kevin-CB · 2022-05-03T08:18:13Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

-        public FormValidation doCheckOsaLowThreshold(@QueryParameter final Integer value) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+        public FormValidation doCheckOsaLowThreshold(@QueryParameter final Integer value,@AncestorInPath Item item) {
+            item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

Kevin-CB · 2022-05-03T08:22:05Z

src/main/java/com/checkmarx/jenkins/CxScanBuilder.java

@@ -2787,7 +2798,7 @@ public ListBoxModel doFillSourceEncodingItems(@QueryParameter final boolean useO
                                                      @QueryParameter final String username, @QueryParameter final String password,
                                                      @QueryParameter final String timestamp, @QueryParameter final String credentialsId,
                                                      @QueryParameter final boolean isProxy, @AncestorInPath Item item) {
-            Jenkins.getInstance().checkPermission(Item.CONFIGURE);
+            item.checkPermission(Item.CONFIGURE);


Suggested change

item.checkPermission(Item.CONFIGURE);

if (item == null) {

return FormValidation.ok();

}

item.checkPermission(Item.CONFIGURE);

… check for job level parameters also.

Kevin-CB

I tested your fix locally and everything seems OK.

You can release it and we'll remove the warning.

SubhadraSahoo · 2022-05-06T09:41:29Z

I tested your fix locally and everything seems OK.

You can merge it and we'll remove the warning.

@Kevin-CB , Thank you Kevin .

…i/checkmarx-plugin into Jenkins_Security_Checks

…jenkinsci/checkmarx-plugin into Jenkins_Security_Checks" This reverts commit 7fda164, reversing changes made to c23cf37.

Changes for version upgrade

* Add validation for descriptor.getDependencyScanConfig()!= null * Add print of SCa info Co-authored-by: Margarital <maragrita.levitm@checkmarx.com>

Version Increment

SubhadraSahoo

Looks good

Changes for Security check

1728e93

Changes for Security check

sangamchitmugre requested review from daniel-beck, Wadeck, Kevin-CB and umeshwaghode April 15, 2022 12:04

Kevin-CB suggested changes Apr 19, 2022

View reviewed changes

Committing

d8a555c

Kevin-CB suggested changes May 3, 2022

View reviewed changes

Addressed the further review comments from Jenkins team , adding null…

fc33767

… check for job level parameters also.

Kevin-CB approved these changes May 6, 2022

View reviewed changes

sangamchitmugre added 3 commits May 10, 2022 16:08

Merge branch 'master' into Jenkins_Security_Checks

c23cf37

Merge branch 'master' into Jenkins_Security_Checks

842befc

Merge branch 'Jenkins_Security_Checks' of https://github.com/jenkinsc…

7fda164

…i/checkmarx-plugin into Jenkins_Security_Checks

daniel-beck removed their request for review May 10, 2022 11:57

sangamchitmugre and others added 4 commits May 10, 2022 18:50

Revert "Merge branch 'Jenkins_Security_Checks' of https://github.com/…

81f9ba3

…jenkinsci/checkmarx-plugin into Jenkins_Security_Checks" This reverts commit 7fda164, reversing changes made to c23cf37.

Changes for version upgrade

c52a202

Changes for version upgrade

Sca npe fix (#101)

7e5a845

* Add validation for descriptor.getDependencyScanConfig()!= null * Add print of SCa info Co-authored-by: Margarital <maragrita.levitm@checkmarx.com>

Version Increment

f23980f

Version Increment

SubhadraSahoo approved these changes May 17, 2022

View reviewed changes

sangamchitmugre merged commit 0f765e7 into master May 17, 2022

Kevin-CB mentioned this pull request May 23, 2022

Remove warning from checkmarx jenkins-infra/update-center2#595

Merged

umeshwaghode deleted the Jenkins_Security_Checks branch September 16, 2022 15:32

Changes for Security check #98

Changes for Security check #98

Conversation

sangamchitmugre commented Apr 15, 2022 • edited Loading

Kevin-CB Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin-CB commented Apr 20, 2022

Kevin-CB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin-CB May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kevin-CB left a comment • edited Loading

Choose a reason for hiding this comment

SubhadraSahoo commented May 6, 2022

SubhadraSahoo left a comment

Choose a reason for hiding this comment

sangamchitmugre commented Apr 15, 2022 •

edited

Loading

Kevin-CB Apr 19, 2022 •

edited

Loading

Kevin-CB May 3, 2022 •

edited

Loading

Kevin-CB left a comment •

edited

Loading