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

new Properties() requires approval also of Properties.<init>(Properties) overload

    • script-security 1.43

      What is the business impact?

      The plugin cannot approve new java.util.Properties() - it asks you again and over again to approve the plugin.

      What is the incorrect behaviour?

      You are asked more than once to approve the same script.

      What is the expected behaviour?

      You should be asked only once to approve and then just work.

      Step by step to reproduce

      Create a pipeline job with the following code.

      import java.util.Properties;
      Properties properties = new java.util.Properties();
      
      node {
          
         echo 'Hello World'
      }
      

      try to build the job
      Go to manage jenkins -> script approval and approve
      try to build again the pipeline and you will see that it still fails

      Workaround

      In my case, approving twice makes the work - but on a real environment, it seems the workaround might not work.

      Regression

      Not sure

      If so, what versions did this previously work in?

      Environment

      Jenkins 2.32.3.2
      script-security-plugin 1.33 and 1.31

          [JENKINS-46757] new Properties() requires approval also of Properties.<init>(Properties) overload

          Jesse Glick added a comment -

          The first approval is of new java.util.Properties (correct); the second, of new java.util.Properties java.util.Properties (incorrect, that is not the constructor being invoked).

          Jesse Glick added a comment - The first approval is of new java.util.Properties (correct); the second, of new java.util.Properties java.util.Properties (incorrect, that is not the constructor being invoked).

          Jesse Glick added a comment -

          Only happens when you use

          Properties p = new Properties()

          not when you use

          def p = new Properties()

          Seems to be a regression from the fix of SECURITY-580.

          Jesse Glick added a comment - Only happens when you use Properties p = new Properties() not when you use def p = new Properties() Seems to be a regression from the fix of SECURITY-580.

          Jesse Glick added a comment -
          org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use new java.util.Properties java.util.Properties
          	at org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.StaticWhitelist.rejectNew(StaticWhitelist.java:184)
          	at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxInterceptor.onNewInstance(SandboxInterceptor.java:148)
          	at org.kohsuke.groovy.sandbox.impl.Checker$14.call(Checker.java:574)
          	at org.kohsuke.groovy.sandbox.impl.Checker.checkedCast(Checker.java:579)
          	at org.kohsuke.groovy.sandbox.impl.Checker$checkedCast$0.callStatic(Unknown Source)
          	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:56)
          	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:194)
          	at Script1.run(Script1.groovy:1)
          

          where here exp is the new empty Properties. The sandbox is verifying that it would be safe to call the Properties constructor that takes Properties, since under some circumstances assigning a Map to a typed variable results in a constructor invocation.

          Probably the Groovy runtime has some special code that disabled this awful trick in cases where the RHS is in fact already assignable to the LHS, and in such a case we could do the same check in checkedCast.

          Or we could just say that this is an acknowledged but very minor issue, and the workaround—approving the second constructor—suffices. (Presumably we would want to just add both constructors to the generic whitelist to begin with.) This would only affect classes which are assignable to Map which have a non-whitelisted copy constructor, I think.

          Jesse Glick added a comment - org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: Scripts not permitted to use new java.util.Properties java.util.Properties at org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.StaticWhitelist.rejectNew(StaticWhitelist.java:184) at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxInterceptor.onNewInstance(SandboxInterceptor.java:148) at org.kohsuke.groovy.sandbox.impl.Checker$14.call(Checker.java:574) at org.kohsuke.groovy.sandbox.impl.Checker.checkedCast(Checker.java:579) at org.kohsuke.groovy.sandbox.impl.Checker$checkedCast$0.callStatic(Unknown Source) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:56) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:194) at Script1.run(Script1.groovy:1) where here exp is the new empty Properties . The sandbox is verifying that it would be safe to call the Properties constructor that takes Properties , since under some circumstances assigning a Map to a typed variable results in a constructor invocation. Probably the Groovy runtime has some special code that disabled this awful trick in cases where the RHS is in fact already assignable to the LHS, and in such a case we could do the same check in checkedCast . Or we could just say that this is an acknowledged but very minor issue, and the workaround—approving the second constructor—suffices. (Presumably we would want to just add both constructors to the generic whitelist to begin with.) This would only affect classes which are assignable to Map which have a non-whitelisted copy constructor, I think.

          Code changed in jenkins
          User: Jesse Glick
          Path:
          src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
          http://jenkins-ci.org/commit/script-security-plugin/3b585dfef229827a66bf0887e5c78b5eb234c365
          Log:
          JENKINS-46757 Reproduced in test.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java http://jenkins-ci.org/commit/script-security-plugin/3b585dfef229827a66bf0887e5c78b5eb234c365 Log: JENKINS-46757 Reproduced in test.

          Code changed in jenkins
          User: Andrew Bayer
          Path:
          src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
          http://jenkins-ci.org/commit/script-security-plugin/93ef475dc1e3ba4d6185d8f08335fac015c308d7
          Log:
          Merge pull request #149 from jglick/Checker-JENKINS-46757

          JENKINS-46757 Reproduced in test

          Compare: https://github.com/jenkinsci/script-security-plugin/compare/2ad18602c823...93ef475dc1e3

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java http://jenkins-ci.org/commit/script-security-plugin/93ef475dc1e3ba4d6185d8f08335fac015c308d7 Log: Merge pull request #149 from jglick/Checker- JENKINS-46757 JENKINS-46757 Reproduced in test Compare: https://github.com/jenkinsci/script-security-plugin/compare/2ad18602c823...93ef475dc1e3

          Andrew Bayer added a comment -

          Turns out this was the same thing as JENKINS-50380, and was fixed and released in script-security 1.43. Woo.

          Andrew Bayer added a comment - Turns out this was the same thing as JENKINS-50380 , and was fixed and released in script-security 1.43. Woo.

            abayer Andrew Bayer
            fbelzunc Félix Belzunce Arcos
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:
              Resolved: