As a Jenkins user, I'll like to use sh semantics to execute commands with args.

      I believe Jenkins needs an args: parameter for the sh pipeline step, right now we have script:, returnStdout: and returnStatus: both useful to avoid messing around with shell redirection. However, is clearly impossible or at least difficult to have clean (and secure) executions with sh if they involve string interpolations, maybe Jenkins should do this automatically, but I'm not sure if that kind of magic could be counter productive, I rather depend on args.

      Like:

      sh(script: "echo", args: ["hello", "world", env.MY_ENV, my_other_def])

      or a new pipeline step to avoid overloading sh with different behaviour

      command(name: "echo", args: ["hello", "world", env.MY_ENV, my_other_def])

      On both cases echo the program, not the shell built-in should be executed, Jenkins should look for the program on the system's $PATH / %PATH% but also an absolute path should be supported too, like a regular Groovy execute() on a List.

      ["/bin/echo", "foo", "bar"].execute()

      Is not common to have Groovy's execute() allowed on Jenkins sandboxed environment, probably for very good reasons.

      Also even if execute() is allowed on the Jenkins sandbox, sh semantics are way more convenient.

      For reference:

      def proc = ['ls', '/meh'].execute()
      println proc.getText()
      println proc.exitValue() != 0

      Where is stderr?

          [JENKINS-44231] Safely pass args to sh step

          Jesse Glick added a comment -

          You are responsible for detainting any untrusted data before including it in a command. Or writing it to a file which a script could process securely.

          Jesse Glick added a comment - You are responsible for detainting any untrusted data before including it in a command. Or writing it to a file which a script could process securely.

          Aaron Curley added a comment -

          Hi jglick. Thank you for responding.  Yes, as things are now, the caller is responsible for properly escaping and quoting any string variables (whether trusted or not).

          In my opinion, this design is inappropriate.  It encourages defects (both security and otherwise) in user pipeline scripts.  Properly escaping and quoting string values stored in variables is a difficult thing to get right.  Furthermore, the documentation surrounding 'sh' doesn't discuss this concern and various articles online seem to indicate that escaping with 'sh' is done automatically.  Granted, the Jenkins project doesn't control all of these sources, but right now the conditions are not set for success (for most users).

          Most frameworks and languages have moved toward supplying a separate argument list.  Then - provided the caller makes use of that list - he/she can be assured that the arguments will be safely passed as data rather than shell commands.

          With that feature implemented, I would also suggest improving the documentation surrounding 'sh' to clearly state how using Groovy string substitution for argument handling is dangerous and should not be done (unless manually escaping and quoting the data).

           

            

           

          Aaron Curley added a comment - Hi jglick . Thank you for responding.  Yes, as things are now, the caller is responsible for properly escaping and quoting any string variables (whether trusted or not). In my opinion, this design is inappropriate.  It encourages defects (both security and otherwise) in user pipeline scripts.  Properly escaping and quoting string values stored in variables is a difficult thing to get right.  Furthermore, the documentation surrounding 'sh' doesn't discuss this concern and various articles online seem to indicate that escaping with 'sh' is done automatically.  Granted, the Jenkins project doesn't control all of these sources, but right now the conditions are not set for success (for most users). Most frameworks and languages have moved toward supplying a separate argument list.  Then - provided the caller makes use of that list - he/she can be assured that the arguments will be safely passed as data rather than shell commands. With that feature implemented, I would also suggest improving the documentation surrounding 'sh' to clearly state how using Groovy string substitution for argument handling is dangerous and should not be done (unless manually escaping and quoting the data).       

          Jesse Glick added a comment -

          I was not arguing with the desirability of having this feature, once someone has time to write it. In the meantime, untrusted data can be handled by writing to a file.

          Jesse Glick added a comment - I was not arguing with the desirability of having this feature, once someone has time to write it. In the meantime, untrusted data can be handled by writing to a file.

          Yuri Govorushchenko added a comment - - edited

          As a workaround I had to write this helper to produce safe shell string literals:

          // Given arbitrary string returns a strongly escaped shell string literal.
          // I.e. it will be in single quotes which turns off interpolation of $(...), etc.
          // E.g.: 1'2\3\'4 5"6 (groovy string) -> '1'\''2\3\'\''4 5"6' (groovy string which can be safely pasted into shell command).
          def shellString(s) {
              // Replace ' with '\'' (https://unix.stackexchange.com/a/187654/260156). Then enclose with '...'.
              // 1) Why not replace \ with \\? Because '...' does not treat backslashes in a special way.
              // 2) And why not use ANSI-C quoting? I.e. we could replace ' with \'
              // and enclose using $'...' (https://stackoverflow.com/a/8254156/4839573).
              // Because ANSI-C quoting is not yet supported by Dash (default shell in Ubuntu & Debian) (https://unix.stackexchange.com/a/371873).
              '\'' + s.replace('\'', '\'\\\'\'') + '\''
          }
          

          Yuri Govorushchenko added a comment - - edited As a workaround I had to write this helper to produce safe shell string literals: // Given arbitrary string returns a strongly escaped shell string literal. // I.e. it will be in single quotes which turns off interpolation of $(...), etc. // E.g.: 1'2\3\'4 5"6 (groovy string) -> '1'\''2\3\'\''4 5"6' (groovy string which can be safely pasted into shell command). def shellString(s) { // Replace ' with '\'' (https://unix.stackexchange.com/a/187654/260156). Then enclose with '...'. // 1) Why not replace \ with \\? Because '...' does not treat backslashes in a special way. // 2) And why not use ANSI-C quoting? I.e. we could replace ' with \' // and enclose using $'...' (https://stackoverflow.com/a/8254156/4839573). // Because ANSI-C quoting is not yet supported by Dash (default shell in Ubuntu & Debian) (https://unix.stackexchange.com/a/371873). '\'' + s.replace('\'', '\'\\\'\'') + '\'' }

          Brian Peiris added a comment -

          Thanks for the helpful script metametadata

          I agree this ought to be built into Jenkins somehow, even if it's just globally accessible helper function.

          Brian Peiris added a comment - Thanks for the helpful script metametadata I agree this ought to be built into Jenkins somehow, even if it's just globally accessible helper function.

          Jesse Glick added a comment -

          From a dev list conversation: this aspect of some Bourne shells means that an implementation which ultimately runs a shell, as opposed to Runtime.exec or the like, may omit certain environment variables.

          Jesse Glick added a comment - From a dev list conversation: this aspect of some Bourne shells means that an implementation which ultimately runs a shell, as opposed to Runtime.exec or the like, may omit certain environment variables.

          In a shared library add /var/shell.groovy:

          def run(args) {
              sh args.collect { "'" + it.replace("'","'\"'\"'") + "'" }.join(' ')
          }
          

          Usage in a pipeline:

          shell(['docker', 'ps'])
          

          (For more info see https://stackoverflow.com/a/1250279)

          Michael Slattery added a comment - In a shared library add /var/shell.groovy: def run(args) { sh args.collect { " '" + it.replace( "' " , " '\" ' \ " '" ) + "' " }.join( ' ' ) } Usage in a pipeline: shell([ 'docker' , 'ps' ]) (For more info see https://stackoverflow.com/a/1250279 )

          Jesse Glick added a comment -

          Yes for sh that would be the workaround, until the Jenkins step does something similar as a standard function. For bat it is a little trickier; as per this and this it might suffice to use something like the following (untested):

          bat args.collect {it.replaceAll('[^a-zA-Z0-9_.\r\n-]', '^$0').replaceAll('\r?\n', '^\r\n\r\n')}.join(' ')
          

          Jesse Glick added a comment - Yes for sh that would be the workaround, until the Jenkins step does something similar as a standard function. For bat it is a little trickier; as per this and this it might suffice to use something like the following ( untested ): bat args.collect {it.replaceAll( '[^a-zA-Z0-9_.\r\n-]' , '^$0' ).replaceAll( '\r?\n' , '^\r\n\r\n' )}.join( ' ' )

          mslattery That isn't sufficient for all cases, alas.

          It will work for pure ascii (I think). Depending on the locale, there are many other things that'll cause problems.

          That's why I use withEnv(){ ... } as a work around until we can pass an array to {{sh.}}

          Christian Höltje added a comment - mslattery That isn't sufficient for all cases, alas. It will work for pure ascii (I think). Depending on the locale, there are many other things that'll cause problems. That's why I use withEnv(){ ... } as a work around until we can pass an array to {{sh .}}

          Sami Korhonen added a comment - - edited

          If I got it correctly, implementing this would require changes to https://github.com/jenkinsci/durable-task-plugin/blob/master/src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java

          This algorithm seems to do following (there's also separate algorithm for binary execution path, but I think most of us do not care about that one):

          1. Write script to a file
          2. Create a launcher command (a command that invokes the script from file)
          3. Invoke launcher command using launcher (this causes launcher command to be parsed to a list of args)

          Launcher does support passing arguments and in fact launcher command actually uses arguments. Launcher command is sh -c '.... sh scriptfile ....'. Second sh is optional and depends on scriptfile beginning with #! .

          My initial thought was that, what if we just passed arguments by serializing them to launcher command. Serializing parameters (escaping) is the problem that we wanted to avoid, so this would move problem from one point to another.

          Then I actually had a look at {{sh }}documentation. Documentation clearly states that you can in pass arguments for command string (-c). With this new information I did few google searches and ended up with a simple test:

          sh -c '(echo numbers: "$@") > test' zero one two && cat test
          numbers: one two

          Test echos string numbers and arguments passed to sh command. Above test is executing script the same way that BourneShellScript is doing. Code:

          cmdString = 'command'
          cmd.addAll(Arrays.asList("sh", "-c", "(" + cmdString + ") >&- 2>&- &"));
          

          Results to:

          sh -c '(command) >&- 2>&- &'

          So, if we modified code to:

          // Pass all arguments to scriptfile (while loops)
          cmdString = 'command "$@"'
          
          // Pass arguments to launcher command
          cmd.addAll(Arrays.asList("sh", "-c", "(" + cmdString + ") >&- 2>&- &"));
          cmd.add('sh')
          cmd.addAll(argumentsPassedToSh)

          We would end up having:

          sh -c '(command "$@") >&- 2>&- &' sh arg1 arg... argN
          

          And as a result Jenkins might just be able to handle arguments properly. I'm too busy to test/implement this change myself, but I hope that someone can find this useful.

           

          Sami Korhonen added a comment - - edited If I got it correctly, implementing this would require changes to  https://github.com/jenkinsci/durable-task-plugin/blob/master/src/main/java/org/jenkinsci/plugins/durabletask/BourneShellScript.java This algorithm seems to do following (there's also separate algorithm for binary execution path, but I think most of us do not care about that one): Write script to a file Create a launcher command (a command that invokes the script from file) Invoke launcher command using launcher (this causes launcher command to be parsed to a list of args) Launcher does support passing arguments and in fact launcher command actually uses arguments. Launcher command is sh -c '.... sh scriptfile ....' . Second sh is optional and depends on scriptfile beginning with #! . My initial thought was that, what if we just passed arguments by serializing them to launcher command. Serializing parameters (escaping) is the problem that we wanted to avoid, so this would move problem from one point to another. Then I actually had a look at {{sh }}documentation. Documentation clearly states that you can in pass arguments for command string (-c). With this new information I did few google searches and ended up with a simple test: sh -c '(echo numbers: "$@" ) > test' zero one two && cat test numbers: one two Test echos string numbers and arguments passed to sh command. Above test is executing script the same way that BourneShellScript is doing. Code: cmdString = 'command' cmd.addAll(Arrays.asList( "sh" , "-c" , "(" + cmdString + ") >&- 2>&- &" )); Results to: sh -c '(command) >&- 2>&- &' So, if we modified code to: // Pass all arguments to scriptfile ( while loops) cmdString = 'command "$@" ' // Pass arguments to launcher command cmd.addAll(Arrays.asList( "sh" , "-c" , "(" + cmdString + ") >&- 2>&- &" )); cmd.add( 'sh' ) cmd.addAll(argumentsPassedToSh) We would end up having: sh -c '(command "$@" ) >&- 2>&- &' sh arg1 arg... argN And as a result Jenkins might just be able to handle arguments properly. I'm too busy to test/implement this change myself, but I hope that someone can find this useful.  

            Unassigned Unassigned
            andresvia Andres V
            Votes:
            20 Vote for this issue
            Watchers:
            23 Start watching this issue

              Created:
              Updated: