Fix more potential command injection via quoting (#5164)

* fix: make quoting null safe * fix: apply quoting in artifact set version * fix: add quoting to more shell step * refactor: use import alias * fix: further quoting --------- Co-authored-by: Oliver Feldmann <[email protected]>
SAP · Oct 30, 2024 · da609e1 · da609e1
1 parent 183004a
commit da609e1
Show file tree

Hide file tree

Showing 12 changed files with 67 additions and 57 deletions.
diff --git a/src/com/sap/piper/BashUtils.groovy b/src/com/sap/piper/BashUtils.groovy
@@ -8,6 +8,9 @@ class BashUtils implements Serializable {
      * Put string in single quotes and escape contained single quotes by putting them into a double quoted string
      */
     static String quoteAndEscape(String str) {
+        if(str == null) {
+            return 'null'
+        }
         def escapedString = str.replace("'", ESCAPED_SINGLE_QUOTE)
         return "'${escapedString}'"
     }

diff --git a/src/com/sap/piper/tools/neo/NeoCommandHelper.groovy b/src/com/sap/piper/tools/neo/NeoCommandHelper.groovy
@@ -1,8 +1,10 @@
 package com.sap.piper.tools.neo
 
-import com.sap.piper.BashUtils
+import static com.sap.piper.BashUtils.quoteAndEscape as q
+
 import com.sap.piper.StepAssertions
 
+
 class NeoCommandHelper {
 
     private Script step
@@ -87,7 +89,7 @@ class NeoCommandHelper {
 
     private String source() {
         StepAssertions.assertFileExists(step, source)
-        return "--source ${BashUtils.quoteAndEscape(source)}"
+        return "--source ${q(source)}"
     }
 
     private String extensions() {
@@ -96,19 +98,19 @@ class NeoCommandHelper {
     }
 
     private String mainArgs() {
-        String usernamePassword = "--user ${BashUtils.quoteAndEscape(user)} --password ${BashUtils.quoteAndEscape(password)}"
+        String usernamePassword = "--user ${q(user)} --password ${q(password)}"
 
         if (deployMode == DeployMode.WAR_PROPERTIES_FILE) {
             StepAssertions.assertFileIsConfiguredAndExists(step, deploymentConfiguration, 'propertiesFile')
             return "${deploymentConfiguration.propertiesFile} ${usernamePassword}"
         }
 
-        String targetArgs = "--host ${BashUtils.quoteAndEscape(deploymentConfiguration.host)}"
-        targetArgs += " --account ${BashUtils.quoteAndEscape(deploymentConfiguration.account)}"
+        String targetArgs = "--host ${q(deploymentConfiguration.host)}"
+        targetArgs += " --account ${q(deploymentConfiguration.account)}"
 
         if (deployMode == DeployMode.WAR_PARAMS) {
 
-            targetArgs += " --application ${BashUtils.quoteAndEscape(deploymentConfiguration.application)}"
+            targetArgs += " --application ${q(deploymentConfiguration.application)}"
         }
 
         return "${targetArgs} ${usernamePassword}"
@@ -120,11 +122,11 @@ class NeoCommandHelper {
         }
 
         String args = ""
-        args += " --runtime ${BashUtils.quoteAndEscape(deploymentConfiguration.runtime)}"
-        args += " --runtime-version ${BashUtils.quoteAndEscape(deploymentConfiguration.runtimeVersion)}"
+        args += " --runtime ${q(deploymentConfiguration.runtime)}"
+        args += " --runtime-version ${q(deploymentConfiguration.runtimeVersion)}"
 
         if (deploymentConfiguration.size) {
-            args += " --size ${BashUtils.quoteAndEscape(deploymentConfiguration.size)}"
+            args += " --size ${q(deploymentConfiguration.size)}"
         }
 
         if (deploymentConfiguration.containsKey('environment')) {
@@ -139,17 +141,17 @@ class NeoCommandHelper {
             for (int i = 0; i < keys.size(); i++) {
                 def key = keys[i]
                 def value = environment.get(keys[i])
-                args += " --ev ${BashUtils.quoteAndEscape(key)}=${BashUtils.quoteAndEscape(value)}"
+                args += " --ev ${q(key)}=${q(value)}"
             }
         }
 
 
         if (deploymentConfiguration.containsKey('vmArguments')) {
-            args += " --vm-arguments ${BashUtils.quoteAndEscape(deploymentConfiguration.vmArguments)}"
+            args += " --vm-arguments ${q(deploymentConfiguration.vmArguments)}"
         }
-        
+
         if (deploymentConfiguration.containsKey('azDistribution')) {
-            args += " --az-distribution ${BashUtils.quoteAndEscape(deploymentConfiguration.azDistribution)}"
+            args += " --az-distribution ${q(deploymentConfiguration.azDistribution)}"
         }
 
         return args

diff --git a/test/groovy/ArtifactSetVersionTest.groovy b/test/groovy/ArtifactSetVersionTest.groovy
@@ -130,8 +130,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
         assertThat(shellRule.shell.join(), stringContainsInOrder([
             "git add .",
             "git commit -m 'update version 1.2.3-20180101010203_testCommitId'",
-            'git tag build_1.2.3-20180101010203_testCommitId',
-            'git push myGitSshUrl build_1.2.3-20180101010203_testCommitId',
+            "git tag 'build_1.2.3-20180101010203_testCommitId'",
+            "git push 'myGitSshUrl' 'build_1.2.3-20180101010203_testCommitId'",
             ]
         ))
     }
@@ -173,8 +173,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
         assertThat(((Iterable)shellRule.shell).join(), stringContainsInOrder([
             "git add .",
             "git commit -m 'update version 1.2.3-20180101010203_testCommitId'",
-            'git tag build_1.2.3-20180101010203_testCommitId',
-            'git push https://me:[email protected]/myGitRepo build_1.2.3-20180101010203_testCommitId',
+            "git tag 'build_1.2.3-20180101010203_testCommitId'",
+            "git push https://me:[email protected]/myGitRepo 'build_1.2.3-20180101010203_testCommitId'",
             ]
         ))
     }
@@ -246,8 +246,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
         assertThat(((Iterable)shellRule.shell).join(), stringContainsInOrder([
             "git add .",
             "git commit -m 'update version 1.2.3-20180101010203_testCommitId'",
-            'git tag build_1.2.3-20180101010203_testCommitId',
-            '#!/bin/bash -e git push --quiet https://me:top%[email protected]/myGitRepo build_1.2.3-20180101010203_testCommitId &>/dev/null',
+            "git tag 'build_1.2.3-20180101010203_testCommitId'",
+            "#!/bin/bash -e git push --quiet https://me:top%[email protected]/myGitRepo 'build_1.2.3-20180101010203_testCommitId' &>/dev/null",
             ]
         ))
     }
@@ -278,8 +278,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
         assertThat(((Iterable)shellRule.shell).join(), stringContainsInOrder([
             "git add .",
             "git commit -m 'update version 1.2.3-20180101010203_testCommitId'",
-            'git tag build_1.2.3-20180101010203_testCommitId',
-            '#!/bin/bash -e git push --quiet https://me:top%[email protected]/myGitRepo build_1.2.3-20180101010203_testCommitId &>/dev/null',
+            "git tag 'build_1.2.3-20180101010203_testCommitId'",
+            "#!/bin/bash -e git push --quiet https://me:top%[email protected]/myGitRepo 'build_1.2.3-20180101010203_testCommitId' &>/dev/null",
             ]
         ))
     }
@@ -301,7 +301,7 @@ class ArtifactSetVersionTest extends BasePiperTest {
     void testVersioningCustomGitUserAndEMail() {
         stepRule.step.artifactSetVersion(script: stepRule.step, juStabGitUtils: gitUtils, buildTool: 'maven', gitSshUrl: 'myGitSshUrl', gitUserEMail: '[email protected]', gitUserName: 'test')
 
-        assertThat(shellRule.shell, hasItem(containsString("git -c user.email=\"[email protected]\" -c user.name=\"test\" commit -m 'update version 1.2.3-20180101010203_testCommitId'")))
+        assertThat(shellRule.shell, hasItem(containsString("git -c user.email='[email protected]' -c user.name='test' commit -m 'update version 1.2.3-20180101010203_testCommitId'")))
     }
 
     @Test

diff --git a/test/groovy/ContainerPushToRegistryTest.groovy b/test/groovy/ContainerPushToRegistryTest.groovy
@@ -221,7 +221,7 @@ class ContainerPushToRegistryTest extends BasePiperTest {
         assertThat(dockerMock.targetRegistry.credentials, is('testCredentialsId'))
         assertThat(dockerMock.targetRegistry.isAnonymous, is(false))
         assertThat(dockerMock.image, is('path/testImage:tag'))
-        assertThat(shellCallRule.shell, hasItem('docker tag testRegistry:55555/path/testImage:tag path/testImage:tag'))
+        assertThat(shellCallRule.shell, hasItem("docker tag 'testRegistry:55555'/'path/testImage:tag' 'path/testImage:tag'"))
         assertThat(dockerMockPull, is(true))
     }
 
@@ -238,7 +238,7 @@ class ContainerPushToRegistryTest extends BasePiperTest {
         assertThat(dockerMock.sourceRegistry.url, is('http://testSourceRegistry'))
         assertThat(dockerMock.image, is('testSourceName:testSourceTag'))
         assertThat(dockerMock.sourceRegistry.isAnonymous, is(true))
-        assertThat(shellCallRule.shell, hasItem('docker tag testSourceRegistry/testSourceName:testSourceTag testSourceName:testSourceTag'))
+        assertThat(shellCallRule.shell, hasItem("docker tag 'testSourceRegistry'/'testSourceName:testSourceTag' 'testSourceName:testSourceTag'"))
         assertThat(dockerMockPull, is(true))
     }
 
@@ -256,7 +256,7 @@ class ContainerPushToRegistryTest extends BasePiperTest {
         assertThat(dockerMock.sourceRegistry.url, is('http://testSourceRegistry'))
         assertThat(dockerMock.sourceRegistry.isAnonymous, is(true))
         assertThat(dockerMock.image, is('testSourceName:testSourceTag'))
-        assertThat(shellCallRule.shell, hasItem('docker tag testSourceRegistry/testSourceName:testSourceTag testImage:tag'))
+        assertThat(shellCallRule.shell, hasItem("docker tag 'testSourceRegistry'/'testSourceName:testSourceTag' 'testImage:tag'"))
         assertThat(dockerMockPull, is(true))
     }
 
@@ -277,7 +277,7 @@ class ContainerPushToRegistryTest extends BasePiperTest {
         assertThat(dockerMock.targetRegistry.url, is('https://testRegistry'))
         assertThat(dockerMock.targetRegistry.credentials, is('testCredentialsId'))
         assertThat(dockerMock.image, is('testSourceName:testSourceTag'))
-        assertThat(shellCallRule.shell, hasItem('docker tag testSourceRegistry/testSourceName:testSourceTag testImage:tag'))
+        assertThat(shellCallRule.shell, hasItem("docker tag 'testSourceRegistry'/'testSourceName:testSourceTag' 'testImage:tag'"))
         assertThat(dockerMockPull, is(true))
     }
 

diff --git a/test/groovy/HealthExecuteCheckTest.groovy b/test/groovy/HealthExecuteCheckTest.groovy
@@ -32,8 +32,8 @@ class HealthExecuteCheckTest extends BasePiperTest {
     @Before
     void init() throws Exception {
         // register Jenkins commands with mock values
-        def command1 = "curl -so /dev/null -w '%{response_code}' http://testserver"
-        def command2 = "curl -so /dev/null -w '%{response_code}' http://testserver/endpoint"
+        def command1 = "curl -so /dev/null -w '%{response_code}' 'http://testserver'"
+        def command2 = "curl -so /dev/null -w '%{response_code}' 'http://testserver/endpoint'"
         helper.registerAllowedMethod('sh', [Map.class], {map ->
             return map.script == command1 || map.script == command2 ? "200" : "404"
         })

diff --git a/test/groovy/NeoDeployTest.groovy b/test/groovy/NeoDeployTest.groovy
@@ -298,25 +298,24 @@ class NeoDeployTest extends BasePiperTest {
         nullScript.commonPipelineEnvironment.setMtarFilePath('archive.mtar')
 
 
-        shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "https:\\/\\/api\\.test\\.com\\/oauth2\\/apitoken\\/v1", "{\"access_token\":\"xxx\"}")
-        shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "https:\\/\\/slservice\\.test\\.host\\.com\\/slservice\\/v1\\/oauth\\/accounts\\/testUser123\\/mtars", "{\"id\":123}")
-        shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "https:\\/\\/slservice\\.test\\.host\\.com\\/slservice\\/v1\\/oauth\\/accounts\\/testUser123\\/mtars", "{\"state\":\"DONE\"}")
-
+        shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "-XPOST.*/apitoken", "{\"access_token\":\"xxx\"}")
+        shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "-XPOST.*https://slservice", "{\"id\":123}")
+        shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "-XGET.*https://slservice", "{\"state\":\"DONE\"}")
 
         stepRule.step.neoDeploy(
             script: nullScript,
             source: archiveName,
             deployMode: 'mta',
-              neo: [
-                  host: 'test.host.com',
-                  account: 'testUser123',
-                      credentialsId: 'OauthDataFileId',
-                      credentialType: 'SecretFile'
-                    ],
+            neo: [
+                host          : 'test.host.com',
+                account       : 'testUser123',
+                credentialsId : 'OauthDataFileId',
+                credentialType: 'SecretFile'
+            ],
         )
 
-        Assert.assertThat(shellRule.shell[0], containsString("#!/bin/bash curl --fail --silent --show-error --retry 12 -XPOST -u \"abc123:testclientsecret123\" \"https://api.test.com/oauth2/apitoken/v1?grant_type=client_credentials\""))
-        Assert.assertThat(shellRule.shell[1], containsString("#!/bin/bash curl --fail --silent --show-error --retry 12 -XPOST -H \"Authorization: Bearer xxx\" -F file=@\"archive.mtar\" \"https://slservice.test.host.com/slservice/v1/oauth/accounts/testUser123/mtars\""))
+        Assert.assertThat(shellRule.shell[0], containsString("#!/bin/bash curl --fail --silent --show-error --retry 12 -XPOST -u 'abc123':'testclientsecret123' 'https://api.test.com/oauth2'/apitoken/v1?grant_type=client_credentials"))
+        Assert.assertThat(shellRule.shell[1], containsString("#!/bin/bash curl --fail --silent --show-error --retry 12 -XPOST -H \"Authorization: Bearer xxx\" -F file=@'archive.mtar' https://slservice.'test.host.com'/slservice/v1/oauth/accounts/'testUser123'/mtars"))
     }
 
     @Test

diff --git a/vars/artifactSetVersion.groovy b/vars/artifactSetVersion.groovy
@@ -1,4 +1,5 @@
 import static com.sap.piper.Prerequisites.checkScript
+import static com.sap.piper.BashUtils.quoteAndEscape as q
 
 import com.sap.piper.GenerateDocumentation
 import com.sap.piper.ConfigurationHelper
@@ -175,7 +176,7 @@ void call(Map parameters = [:], Closure body = null) {
             def gitConfig = []
 
             if(config.gitUserEMail) {
-                gitConfig.add("-c user.email=\"${config.gitUserEMail}\"")
+                gitConfig.add("-c user.email=${q(config.gitUserEMail)}")
             } else {
                 // in case there is no user.email configured on project level we might still
                 // be able to work in case there is a configuration available on plain git level.
@@ -184,7 +185,7 @@ void call(Map parameters = [:], Closure body = null) {
                 }
             }
             if(config.gitUserName) {
-                gitConfig.add("-c user.name=\"${config.gitUserName}\"")
+                gitConfig.add("-c user.name=${q(config.gitUserName)}")
             } else {
                 // in case there is no user.name configured on project level we might still
                 // be able to work in case there is a configuration available on plain git level.
@@ -199,7 +200,7 @@ void call(Map parameters = [:], Closure body = null) {
                     set -e
                     git add . --update
                     git ${gitConfig} commit -m 'update version ${newVersion}'
-                    git tag ${config.tagPrefix}${newVersion}"""
+                    git tag ${q(config.tagPrefix+newVersion)}"""
                 config.gitCommitId = gitUtils.getGitCommitIdOrNull()
             } catch (e) {
                 error "[${STEP_NAME}]git commit and tag failed: ${e}"
@@ -215,7 +216,7 @@ void call(Map parameters = [:], Closure body = null) {
                     .use()
 
                 sshagent([config.gitSshKeyCredentialsId]) {
-                    sh "git push ${config.gitSshUrl} ${config.tagPrefix}${newVersion}"
+                    sh "git push ${q(config.gitSshUrl)} ${q(config.tagPrefix+newVersion)}"
                 }
 
             } else if(gitPushMode == GitPushMode.HTTPS) {
@@ -259,7 +260,7 @@ void call(Map parameters = [:], Closure body = null) {
                     gitConfig = []
 
                     if(config.gitHttpProxy) {
-                        gitConfig.add("-c http.proxy=\"${config.gitHttpProxy}\"")
+                        gitConfig.add("-c http.proxy=${q(config.gitHttpProxy)}")
                     }
 
                     if(config.gitDisableSslVerification) {
@@ -288,7 +289,7 @@ void call(Map parameters = [:], Closure body = null) {
                     gitPushFlags = gitPushFlags.join(' ')
 
                     sh script:   """|#!/bin/bash ${hashbangFlags}
-                                    |${gitDebug}git ${gitConfig} push ${gitPushFlags} ${gitUrlWithCredentials} ${config.tagPrefix}${newVersion} ${streamhandling}""".stripMargin()
+                                    |${gitDebug}git ${gitConfig} push ${gitPushFlags} ${gitUrlWithCredentials} ${q(config.tagPrefix+newVersion)} ${streamhandling}""".stripMargin()
                 }
             } else {
                 echo "Git push mode: ${gitPushMode.toString()}. Git push to remote has been skipped."
@@ -313,5 +314,5 @@ def isAppContainer(config){
 }
 
 def getTimestamp(pattern){
-    return sh(returnStdout: true, script: "date --utc +'${pattern}'").trim()
+    return sh(returnStdout: true, script: "date --utc +${q(pattern)}").trim()
 }
diff --git a/vars/containerPushToRegistry.groovy b/vars/containerPushToRegistry.groovy
@@ -5,6 +5,7 @@ import com.sap.piper.DockerUtils
 import groovy.transform.Field
 
 import static com.sap.piper.Prerequisites.checkScript
+import static com.sap.piper.BashUtils.quoteAndEscape as q
 
 @Field String STEP_NAME = getClass().getName()
 @Field Set GENERAL_CONFIG_KEYS = [
@@ -101,7 +102,7 @@ void call(Map parameters = [:]) {
                 ) {
                     sourceBuildImage.pull()
                 }
-                sh "docker tag ${config.sourceRegistry}/${config.sourceImage} ${config.dockerImage}"
+                sh "docker tag ${q(config.sourceRegistry)}/${q(config.sourceImage)} ${q(config.dockerImage)}"
             }
 
             docker.withRegistry(

diff --git a/vars/dockerExecute.groovy b/vars/dockerExecute.groovy
@@ -1,6 +1,7 @@
 import com.sap.piper.SidecarUtils
 
 import static com.sap.piper.Prerequisites.checkScript
+import static com.sap.piper.BashUtils.quoteAndEscape as q
 
 import com.cloudbees.groovy.cps.NonCPS
 import com.sap.piper.ConfigurationHelper
@@ -242,7 +243,7 @@ void call(Map parameters = [:], body) {
                         }
                     } else {
                         def networkName = "sidecar-${UUID.randomUUID()}"
-                        sh "docker network create ${networkName}"
+                        sh "docker network create ${q(networkName)}"
                         try {
                             def sidecarImage = docker.image(config.sidecarImage)
                             pullWrapper(config.sidecarPullImage, sidecarImage, config.sidecarRegistryUrl, config.sidecarRegistryCredentialsId) {

diff --git a/vars/healthExecuteCheck.groovy b/vars/healthExecuteCheck.groovy
@@ -1,4 +1,5 @@
 import static com.sap.piper.Prerequisites.checkScript
+import static com.sap.piper.BashUtils.quoteAndEscape as q
 
 import com.sap.piper.GenerateDocumentation
 import com.sap.piper.ConfigurationHelper
@@ -70,6 +71,6 @@ void call(Map parameters = [:]) {
 def curl(url){
     return sh(
         returnStdout: true,
-        script: "curl -so /dev/null -w '%{response_code}' ${url}"
+        script: "curl -so /dev/null -w '%{response_code}' ${q(url)}"
     ).trim()
 }
diff --git a/vars/mailSendNotification.groovy b/vars/mailSendNotification.groovy
@@ -1,4 +1,5 @@
 import static com.sap.piper.Prerequisites.checkScript
+import static com.sap.piper.BashUtils.quoteAndEscape as q
 
 import com.sap.piper.ConfigurationHelper
 import com.sap.piper.GenerateDocumentation
@@ -198,8 +199,8 @@ def getCulprits(config, branch, numberOfCommits) {
                 def pullRequestID = branch.replaceAll('PR-', '')
                 def localBranchName = "pr" + pullRequestID
                 sh """git init
-    git fetch ${config.gitUrl} pull/${pullRequestID}/head:${localBranchName} > /dev/null 2>&1
-    git checkout -f ${localBranchName} > /dev/null 2>&1
+    git fetch ${q(config.gitUrl)} pull/${q(pullRequestID)}/head:${q(localBranchName)} > /dev/null 2>&1
+    git checkout -f ${q(localBranchName)} > /dev/null 2>&1
     """
             }
         } else {
@@ -210,8 +211,8 @@ def getCulprits(config, branch, numberOfCommits) {
                     credentials: [config.gitSshKeyCredentialsId],
                     ignoreMissing: true
                 ) {
-                    sh """git clone ${config.gitUrl} .
-    git checkout ${config.gitCommitId} > /dev/null 2>&1"""
+                    sh """git clone ${q(config.gitUrl)} .
+    git checkout ${q(config.gitCommitId)} > /dev/null 2>&1"""
                 }
             } else {
                 def retCode = sh(returnStatus: true, script: 'git log > /dev/null 2>&1')