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

Safely pass args to sh step

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      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?

        Attachments

          Issue Links

            Activity

            Hide
            jglick Jesse Glick added a comment -

            String.execute and friends are out of the question; these execute on the master.

            In principle sh could take a List<String> though it may be tricky to process these since we do need to ultimately create a textual script, so any metacharacters would need to be escaped by Jenkins.

            Show
            jglick Jesse Glick added a comment - String.execute and friends are out of the question; these execute on the master. In principle sh could take a List<String> though it may be tricky to process these since we do need to ultimately create a textual script, so any metacharacters would need to be escaped by Jenkins.
            Hide
            jglick Jesse Glick added a comment -

            This could also automatically or at user direction select a DurableTask implementation (sh, bat, powershell).

            Show
            jglick Jesse Glick added a comment - This could also automatically or at user direction select a DurableTask implementation ( sh , bat , powershell ).
            Hide
            jglick Jesse Glick added a comment -

            (See JENKINS-26169 for that.)

            Show
            jglick Jesse Glick added a comment - (See  JENKINS-26169 for that.)
            Hide
            aaron312 Aaron Curley added a comment -

            Just to reiterate one of the OP's points – if your Jenkinsfile needs to supply a variable containing untrusted data as an argument to a program run by 'sh', as far as I can tell right now there is no secure way to do this unless one writes their own shell script escaping logic.

            For instance, in the following example:

            sh "echo \"$untrustedString\""

            the variable 'untrustedString' could contain a double-quote, thus breaking out of the parameter context into the control context.  Am I mistaken?

            If I am correct, this seems like a pretty serious design flaw and maybe the priority on implementing this ticket should be increased?  Also, it seems like the current form of invoking 'sh' should be immediately deprecated since it is dangerous.

            Show
            aaron312 Aaron Curley added a comment - Just to reiterate one of the OP's points – if your Jenkinsfile needs to supply a variable containing untrusted data as an argument to a program run by 'sh', as far as I can tell right now there is no secure way to do this unless one writes their own shell script escaping logic. For instance, in the following example: sh "echo \" $untrustedString\"" the variable 'untrustedString' could contain a double-quote, thus breaking out of the parameter context into the control context.  Am I mistaken? If I am correct, this seems like a pretty serious design flaw and maybe the priority on implementing this ticket should be increased?  Also, it seems like the current form of invoking 'sh' should be immediately deprecated since it is dangerous.
            Hide
            jglick 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.

            Show
            jglick 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.
            Hide
            aaron312 Aaron Curley added a comment -

            Hi Jesse Glick. 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).

             

              

             

            Show
            aaron312 Aaron Curley added a comment - Hi Jesse Glick . 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).       
            Hide
            jglick 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.

            Show
            jglick 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.
            Hide
            metametadata 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('\'', '\'\\\'\'') + '\''
            }
            
            Show
            metametadata 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('\'', '\'\\\'\'') + '\'' }
            Hide
            brianpeiris Brian Peiris added a comment -

            Thanks for the helpful script Yuri Govorushchenko

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

            Show
            brianpeiris Brian Peiris added a comment - Thanks for the helpful script Yuri Govorushchenko I agree this ought to be built into Jenkins somehow, even if it's just globally accessible helper function.
            Hide
            jglick 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.

            Show
            jglick 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.
            Hide
            mslattery 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)

            Show
            mslattery 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 )
            Hide
            jglick 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(' ')
            
            Show
            jglick 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( ' ' )
            Hide
            docwhat Christian Höltje added a comment -

            Michael Slattery 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.}}

            Show
            docwhat Christian Höltje added a comment - Michael Slattery 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 .}}
            Hide
            skorhone 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.

             

            Show
            skorhone 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.  

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              andresvia Andres V
              Votes:
              17 Vote for this issue
              Watchers:
              20 Start watching this issue

                Dates

                Created:
                Updated: