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

script-security doesn't handle a non-standard Collection without a constructor taking an array properly

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Minor Minor
    • script-security-plugin
    • Jenkins 2.89.4.2
      Script Security 1.39
      Pipeline 2.5

      Pipeline job fails after upgrading Jenkins from 2.32.1.1 to 2.89.4.2. The following error is returned although Script approval list is empty:

      org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: unclassified new common.EnvsToBuild common.Env
       at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxInterceptor.onNewInstance(SandboxInterceptor.java:144)
       at org.kohsuke.groovy.sandbox.impl.Checker$14.call(Checker.java:580)
       at org.kohsuke.groovy.sandbox.impl.Checker.checkedCast(Checker.java:585)
       at org.kohsuke.groovy.sandbox.impl.Checker$checkedCast.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 org.kohsuke.groovy.sandbox.impl.Checker$2.call(Checker.java:188)
       at org.kohsuke.groovy.sandbox.GroovyInterceptor.onStaticCall(GroovyInterceptor.java:35)
       at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxInterceptor.onStaticCall(SandboxInterceptor.java:158)
       at org.kohsuke.groovy.sandbox.impl.Checker$2.call(Checker.java:186)
       at org.kohsuke.groovy.sandbox.impl.Checker.checkedStaticCall(Checker.java:190)
       at org.kohsuke.groovy.sandbox.impl.Checker.checkedCall(Checker.java:97)
       at com.cloudbees.groovy.cps.sandbox.SandboxInvoker.methodCall(SandboxInvoker.java:17)
       at WorkflowScript.run(WorkflowScript:5)
       at __cps.transform__(Native Method)
       at com.cloudbees.groovy.cps.impl.ContinuationGroup.methodCall(ContinuationGroup.java:57)
       at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.dispatchOrArg(FunctionCallBlock.java:109)
       at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.fixArg(FunctionCallBlock.java:82)
       at sun.reflect.GeneratedMethodAccessor224.invoke(Unknown Source)
       at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
       at java.lang.reflect.Method.invoke(Method.java:498)
       at com.cloudbees.groovy.cps.impl.ContinuationPtr$ContinuationImpl.receive(ContinuationPtr.java:72)
       at com.cloudbees.groovy.cps.impl.ConstantBlock.eval(ConstantBlock.java:21)
       at com.cloudbees.groovy.cps.Next.step(Next.java:83)
       at com.cloudbees.groovy.cps.Continuable$1.call(Continuable.java:174)
       at com.cloudbees.groovy.cps.Continuable$1.call(Continuable.java:163)
       at org.codehaus.groovy.runtime.GroovyCategorySupport$ThreadCategoryInfo.use(GroovyCategorySupport.java:122)
       at org.codehaus.groovy.runtime.GroovyCategorySupport.use(GroovyCategorySupport.java:261)
       at com.cloudbees.groovy.cps.Continuable.run0(Continuable.java:163)
       at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.access$001(SandboxContinuable.java:19)
       at org.jenkinsci.plugins.workflow.cps.SandboxContinuable$1.call(SandboxContinuable.java:35)
       at org.jenkinsci.plugins.workflow.cps.SandboxContinuable$1.call(SandboxContinuable.java:32)
       at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.runInSandbox(GroovySandbox.java:108)
       at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.run0(SandboxContinuable.java:32)
       at org.jenkinsci.plugins.workflow.cps.CpsThread.runNextChunk(CpsThread.java:174)
       at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.run(CpsThreadGroup.java:331)
       at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.access$200(CpsThreadGroup.java:82)
       at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:243)
       at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:231)
       at org.jenkinsci.plugins.workflow.cps.CpsVmExecutorService$2.call(CpsVmExecutorService.java:64)
       at java.util.concurrent.FutureTask.run(FutureTask.java:266)
       at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:112)
       at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
       at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
       at java.util.concurrent.FutureTask.run(FutureTask.java:266)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
       at java.lang.Thread.run(Thread.java:745)
       Finished: FAILURE

      Pipeline code that's not working:

      import common.EnvsToBuild
      @Library('jenkins-groovy-error-reproduction-library') _
      EnvsToBuild envsToBuild = new EnvsToBuild(true, false) 
      println envsToBuild
      //println "${new EnvsToBuild(true, false)}"

      Workaround:

      import common.EnvsToBuild
      @Library('jenkins-groovy-error-reproduction-library') _
      //EnvsToBuild envsToBuild = new EnvsToBuild(true, false) 
      //println envsToBuild
      println "${new EnvsToBuild(true, false)}"
      
      import common.EnvsToBuild
      @Library('jenkins-groovy-error-reproduction-library') _
      EnvsToBuild envsToBuild;
      envsToBuild = new EnvsToBuild(true, false) 
      println envsToBuild
      
      import common.EnvsToBuild
      @Library('jenkins-groovy-error-reproduction-library') _
      def envsToBuild = new EnvsToBuild(true, false) 
      println envsToBuild
      

       

          [JENKINS-50380] script-security doesn't handle a non-standard Collection without a constructor taking an array properly

          Andrew Bayer added a comment -

          So: this was introduced in script-security 1.31 as part of one of a number of security fixes relating to casting. There are actually two things going on here - first is that the logic for actually doing a sandbox-checked cast still tries to do a cast even when the target class and the class of the object to cast are exactly the same. That's obviously goofy and shouldn't happen. The second is that the logic for doing the sandbox-checked cast sees any Collection that isn't in java.util (those are handled natively by Groovy so we don't worry about them) and, rather than simply casting it, it does a new ClassInQuestion(original.toArray()). Which normally works for a Collection since most will have a constructor taking an array of objects, but doesn't work in this case, where there is no such constructor.

          While I do believe the cast-to-same-class case should be fixed in general, and I am not pleased with the array constructor for a Collection thing, the bigger issue here is that I don't think we should be ending up in a sandbox-checked cast in this situation in the first place. However, I can't figure out how to prevent that without reopening the casting related issues this is dealing with in the first place...so it may be that we need to accept the potentially pointless sandbox-checked cast, but ensure that said sandbox-checked cast will check at runtime if a straight Class#cast call would suffice and do that instead of handing off to Groovy's casting magic...

          Andrew Bayer added a comment - So: this was introduced in script-security 1.31 as part of one of a number of security fixes relating to casting. There are actually two things going on here - first is that the logic for actually doing a sandbox-checked cast still tries to do a cast even when the target class and the class of the object to cast are exactly the same. That's obviously goofy and shouldn't happen. The second is that the logic for doing the sandbox-checked cast sees any Collection that isn't in java.util (those are handled natively by Groovy so we don't worry about them) and, rather than simply casting it, it does a new ClassInQuestion(original.toArray()) . Which normally works for a Collection since most will have a constructor taking an array of objects, but doesn't work in this case, where there is no such constructor. While I do believe the cast-to-same-class case should be fixed in general, and I am not pleased with the array constructor for a Collection thing, the bigger issue here is that I don't think we should be ending up in a sandbox-checked cast in this situation in the first place. However, I can't figure out how to prevent that without reopening the casting related issues this is dealing with in the first place...so it may be that we need to accept the potentially pointless sandbox-checked cast, but ensure that said sandbox-checked cast will check at runtime if a straight Class#cast call would suffice and do that instead of handing off to Groovy's casting magic...

          Andrew Bayer added a comment -

          Ok, I've got PRs up for groovy-sandbox and script-security that fix this by making sure Checker.checkedCast doesn't bother jumping through the DefaultTypeTransformation.castToType hoops if a simple clazz.cast will do the trick. Let's see what the reviewers think.

          Andrew Bayer added a comment - Ok, I've got PRs up for groovy-sandbox and script-security that fix this by making sure Checker.checkedCast doesn't bother jumping through the DefaultTypeTransformation.castToType hoops if a simple clazz.cast will do the trick. Let's see what the reviewers think.

          Code changed in jenkins
          User: Andrew Bayer
          Path:
          pom.xml
          src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
          http://jenkins-ci.org/commit/script-security-plugin/c307bcc62f6170ec6e3f1a71b986c723567f42e1
          Log:
          JENKINS-50380 checkedCast should use clazz.cast when assignable

          Downstream of https://github.com/jenkinsci/groovy-sandbox/pull/45

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: pom.xml src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java http://jenkins-ci.org/commit/script-security-plugin/c307bcc62f6170ec6e3f1a71b986c723567f42e1 Log: JENKINS-50380 checkedCast should use clazz.cast when assignable Downstream of https://github.com/jenkinsci/groovy-sandbox/pull/45

          Code changed in jenkins
          User: Andrew Bayer
          Path:
          pom.xml
          src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java
          http://jenkins-ci.org/commit/script-security-plugin/c9ecc7957e742b042326173e314a9e6619a8def4
          Log:
          Merge pull request #192 from abayer/jenkins-50380

          JENKINS-50380 checkedCast should just return object when assignable

          Compare: https://github.com/jenkinsci/script-security-plugin/compare/2fa618d6f534...c9ecc7957e74

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: pom.xml src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java http://jenkins-ci.org/commit/script-security-plugin/c9ecc7957e742b042326173e314a9e6619a8def4 Log: Merge pull request #192 from abayer/jenkins-50380 JENKINS-50380 checkedCast should just return object when assignable Compare: https://github.com/jenkinsci/script-security-plugin/compare/2fa618d6f534...c9ecc7957e74

          Andrew Bayer added a comment -

          Merged, releasing momentarily as part of script-security 1.43.

          Andrew Bayer added a comment - Merged, releasing momentarily as part of script-security 1.43.

            abayer Andrew Bayer
            rzhou Ronnie Zhou
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: