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

              People

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

                Dates

                • Created:
                  Updated: