Skip to content

Commit da609e1

Browse files
holgparo-liver
holgpar
and
o-liver
authored
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]>
1 parent 183004a commit da609e1

12 files changed

+67
-57
lines changed

‎src/com/sap/piper/BashUtils.groovy‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ class BashUtils implements Serializable {
88
* Put string in single quotes and escape contained single quotes by putting them into a double quoted string
99
*/
1010
static String quoteAndEscape(String str) {
11+
if(str == null) {
12+
return 'null'
13+
}
1114
def escapedString = str.replace("'", ESCAPED_SINGLE_QUOTE)
1215
return "'${escapedString}'"
1316
}

‎src/com/sap/piper/tools/neo/NeoCommandHelper.groovy‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
package com.sap.piper.tools.neo
22

3-
import com.sap.piper.BashUtils
3+
import static com.sap.piper.BashUtils.quoteAndEscape as q
4+
45
import com.sap.piper.StepAssertions
56

7+
68
class NeoCommandHelper {
79

810
private Script step
@@ -87,7 +89,7 @@ class NeoCommandHelper {
8789

8890
private String source() {
8991
StepAssertions.assertFileExists(step, source)
90-
return "--source ${BashUtils.quoteAndEscape(source)}"
92+
return "--source ${q(source)}"
9193
}
9294

9395
private String extensions() {
@@ -96,19 +98,19 @@ class NeoCommandHelper {
9698
}
9799

98100
private String mainArgs() {
99-
String usernamePassword = "--user ${BashUtils.quoteAndEscape(user)} --password ${BashUtils.quoteAndEscape(password)}"
101+
String usernamePassword = "--user ${q(user)} --password ${q(password)}"
100102

101103
if (deployMode == DeployMode.WAR_PROPERTIES_FILE) {
102104
StepAssertions.assertFileIsConfiguredAndExists(step, deploymentConfiguration, 'propertiesFile')
103105
return "${deploymentConfiguration.propertiesFile} ${usernamePassword}"
104106
}
105107

106-
String targetArgs = "--host ${BashUtils.quoteAndEscape(deploymentConfiguration.host)}"
107-
targetArgs += " --account ${BashUtils.quoteAndEscape(deploymentConfiguration.account)}"
108+
String targetArgs = "--host ${q(deploymentConfiguration.host)}"
109+
targetArgs += " --account ${q(deploymentConfiguration.account)}"
108110

109111
if (deployMode == DeployMode.WAR_PARAMS) {
110112

111-
targetArgs += " --application ${BashUtils.quoteAndEscape(deploymentConfiguration.application)}"
113+
targetArgs += " --application ${q(deploymentConfiguration.application)}"
112114
}
113115

114116
return "${targetArgs} ${usernamePassword}"
@@ -120,11 +122,11 @@ class NeoCommandHelper {
120122
}
121123

122124
String args = ""
123-
args += " --runtime ${BashUtils.quoteAndEscape(deploymentConfiguration.runtime)}"
124-
args += " --runtime-version ${BashUtils.quoteAndEscape(deploymentConfiguration.runtimeVersion)}"
125+
args += " --runtime ${q(deploymentConfiguration.runtime)}"
126+
args += " --runtime-version ${q(deploymentConfiguration.runtimeVersion)}"
125127

126128
if (deploymentConfiguration.size) {
127-
args += " --size ${BashUtils.quoteAndEscape(deploymentConfiguration.size)}"
129+
args += " --size ${q(deploymentConfiguration.size)}"
128130
}
129131

130132
if (deploymentConfiguration.containsKey('environment')) {
@@ -139,17 +141,17 @@ class NeoCommandHelper {
139141
for (int i = 0; i < keys.size(); i++) {
140142
def key = keys[i]
141143
def value = environment.get(keys[i])
142-
args += " --ev ${BashUtils.quoteAndEscape(key)}=${BashUtils.quoteAndEscape(value)}"
144+
args += " --ev ${q(key)}=${q(value)}"
143145
}
144146
}
145147

146148

147149
if (deploymentConfiguration.containsKey('vmArguments')) {
148-
args += " --vm-arguments ${BashUtils.quoteAndEscape(deploymentConfiguration.vmArguments)}"
150+
args += " --vm-arguments ${q(deploymentConfiguration.vmArguments)}"
149151
}
150-
152+
151153
if (deploymentConfiguration.containsKey('azDistribution')) {
152-
args += " --az-distribution ${BashUtils.quoteAndEscape(deploymentConfiguration.azDistribution)}"
154+
args += " --az-distribution ${q(deploymentConfiguration.azDistribution)}"
153155
}
154156

155157
return args

‎test/groovy/ArtifactSetVersionTest.groovy‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
130130
assertThat(shellRule.shell.join(), stringContainsInOrder([
131131
"git add .",
132132
"git commit -m 'update version 1.2.3-20180101010203_testCommitId'",
133-
'git tag build_1.2.3-20180101010203_testCommitId',
134-
'git push myGitSshUrl build_1.2.3-20180101010203_testCommitId',
133+
"git tag 'build_1.2.3-20180101010203_testCommitId'",
134+
"git push 'myGitSshUrl' 'build_1.2.3-20180101010203_testCommitId'",
135135
]
136136
))
137137
}
@@ -173,8 +173,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
173173
assertThat(((Iterable)shellRule.shell).join(), stringContainsInOrder([
174174
"git add .",
175175
"git commit -m 'update version 1.2.3-20180101010203_testCommitId'",
176-
'git tag build_1.2.3-20180101010203_testCommitId',
177-
'git push https://me:[email protected]/myGitRepo build_1.2.3-20180101010203_testCommitId',
176+
"git tag 'build_1.2.3-20180101010203_testCommitId'",
177+
"git push https://me:[email protected]/myGitRepo 'build_1.2.3-20180101010203_testCommitId'",
178178
]
179179
))
180180
}
@@ -246,8 +246,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
246246
assertThat(((Iterable)shellRule.shell).join(), stringContainsInOrder([
247247
"git add .",
248248
"git commit -m 'update version 1.2.3-20180101010203_testCommitId'",
249-
'git tag build_1.2.3-20180101010203_testCommitId',
250-
'#!/bin/bash -e git push --quiet https://me:top%[email protected]/myGitRepo build_1.2.3-20180101010203_testCommitId &>/dev/null',
249+
"git tag 'build_1.2.3-20180101010203_testCommitId'",
250+
"#!/bin/bash -e git push --quiet https://me:top%[email protected]/myGitRepo 'build_1.2.3-20180101010203_testCommitId' &>/dev/null",
251251
]
252252
))
253253
}
@@ -278,8 +278,8 @@ class ArtifactSetVersionTest extends BasePiperTest {
278278
assertThat(((Iterable)shellRule.shell).join(), stringContainsInOrder([
279279
"git add .",
280280
"git commit -m 'update version 1.2.3-20180101010203_testCommitId'",
281-
'git tag build_1.2.3-20180101010203_testCommitId',
282-
'#!/bin/bash -e git push --quiet https://me:top%[email protected]/myGitRepo build_1.2.3-20180101010203_testCommitId &>/dev/null',
281+
"git tag 'build_1.2.3-20180101010203_testCommitId'",
282+
"#!/bin/bash -e git push --quiet https://me:top%[email protected]/myGitRepo 'build_1.2.3-20180101010203_testCommitId' &>/dev/null",
283283
]
284284
))
285285
}
@@ -301,7 +301,7 @@ class ArtifactSetVersionTest extends BasePiperTest {
301301
void testVersioningCustomGitUserAndEMail() {
302302
stepRule.step.artifactSetVersion(script: stepRule.step, juStabGitUtils: gitUtils, buildTool: 'maven', gitSshUrl: 'myGitSshUrl', gitUserEMail: '[email protected]', gitUserName: 'test')
303303

304-
assertThat(shellRule.shell, hasItem(containsString("git -c user.email=\"[email protected]\" -c user.name=\"test\" commit -m 'update version 1.2.3-20180101010203_testCommitId'")))
304+
assertThat(shellRule.shell, hasItem(containsString("git -c user.email='[email protected]' -c user.name='test' commit -m 'update version 1.2.3-20180101010203_testCommitId'")))
305305
}
306306

307307
@Test

‎test/groovy/ContainerPushToRegistryTest.groovy‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ class ContainerPushToRegistryTest extends BasePiperTest {
221221
assertThat(dockerMock.targetRegistry.credentials, is('testCredentialsId'))
222222
assertThat(dockerMock.targetRegistry.isAnonymous, is(false))
223223
assertThat(dockerMock.image, is('path/testImage:tag'))
224-
assertThat(shellCallRule.shell, hasItem('docker tag testRegistry:55555/path/testImage:tag path/testImage:tag'))
224+
assertThat(shellCallRule.shell, hasItem("docker tag 'testRegistry:55555'/'path/testImage:tag' 'path/testImage:tag'"))
225225
assertThat(dockerMockPull, is(true))
226226
}
227227

@@ -238,7 +238,7 @@ class ContainerPushToRegistryTest extends BasePiperTest {
238238
assertThat(dockerMock.sourceRegistry.url, is('http://testSourceRegistry'))
239239
assertThat(dockerMock.image, is('testSourceName:testSourceTag'))
240240
assertThat(dockerMock.sourceRegistry.isAnonymous, is(true))
241-
assertThat(shellCallRule.shell, hasItem('docker tag testSourceRegistry/testSourceName:testSourceTag testSourceName:testSourceTag'))
241+
assertThat(shellCallRule.shell, hasItem("docker tag 'testSourceRegistry'/'testSourceName:testSourceTag' 'testSourceName:testSourceTag'"))
242242
assertThat(dockerMockPull, is(true))
243243
}
244244

@@ -256,7 +256,7 @@ class ContainerPushToRegistryTest extends BasePiperTest {
256256
assertThat(dockerMock.sourceRegistry.url, is('http://testSourceRegistry'))
257257
assertThat(dockerMock.sourceRegistry.isAnonymous, is(true))
258258
assertThat(dockerMock.image, is('testSourceName:testSourceTag'))
259-
assertThat(shellCallRule.shell, hasItem('docker tag testSourceRegistry/testSourceName:testSourceTag testImage:tag'))
259+
assertThat(shellCallRule.shell, hasItem("docker tag 'testSourceRegistry'/'testSourceName:testSourceTag' 'testImage:tag'"))
260260
assertThat(dockerMockPull, is(true))
261261
}
262262

@@ -277,7 +277,7 @@ class ContainerPushToRegistryTest extends BasePiperTest {
277277
assertThat(dockerMock.targetRegistry.url, is('https://testRegistry'))
278278
assertThat(dockerMock.targetRegistry.credentials, is('testCredentialsId'))
279279
assertThat(dockerMock.image, is('testSourceName:testSourceTag'))
280-
assertThat(shellCallRule.shell, hasItem('docker tag testSourceRegistry/testSourceName:testSourceTag testImage:tag'))
280+
assertThat(shellCallRule.shell, hasItem("docker tag 'testSourceRegistry'/'testSourceName:testSourceTag' 'testImage:tag'"))
281281
assertThat(dockerMockPull, is(true))
282282
}
283283

‎test/groovy/HealthExecuteCheckTest.groovy‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ class HealthExecuteCheckTest extends BasePiperTest {
3232
@Before
3333
void init() throws Exception {
3434
// register Jenkins commands with mock values
35-
def command1 = "curl -so /dev/null -w '%{response_code}' http://testserver"
36-
def command2 = "curl -so /dev/null -w '%{response_code}' http://testserver/endpoint"
35+
def command1 = "curl -so /dev/null -w '%{response_code}' 'http://testserver'"
36+
def command2 = "curl -so /dev/null -w '%{response_code}' 'http://testserver/endpoint'"
3737
helper.registerAllowedMethod('sh', [Map.class], {map ->
3838
return map.script == command1 || map.script == command2 ? "200" : "404"
3939
})

‎test/groovy/NeoDeployTest.groovy‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -298,25 +298,24 @@ class NeoDeployTest extends BasePiperTest {
298298
nullScript.commonPipelineEnvironment.setMtarFilePath('archive.mtar')
299299

300300

301-
shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "https:\\/\\/api\\.test\\.com\\/oauth2\\/apitoken\\/v1", "{\"access_token\":\"xxx\"}")
302-
shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "https:\\/\\/slservice\\.test\\.host\\.com\\/slservice\\/v1\\/oauth\\/accounts\\/testUser123\\/mtars", "{\"id\":123}")
303-
shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "https:\\/\\/slservice\\.test\\.host\\.com\\/slservice\\/v1\\/oauth\\/accounts\\/testUser123\\/mtars", "{\"state\":\"DONE\"}")
304-
301+
shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "-XPOST.*/apitoken", "{\"access_token\":\"xxx\"}")
302+
shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "-XPOST.*https://slservice", "{\"id\":123}")
303+
shellRule.setReturnValue(JenkinsShellCallRule.Type.REGEX, "-XGET.*https://slservice", "{\"state\":\"DONE\"}")
305304

306305
stepRule.step.neoDeploy(
307306
script: nullScript,
308307
source: archiveName,
309308
deployMode: 'mta',
310-
neo: [
311-
host: 'test.host.com',
312-
account: 'testUser123',
313-
credentialsId: 'OauthDataFileId',
314-
credentialType: 'SecretFile'
315-
],
309+
neo: [
310+
host : 'test.host.com',
311+
account : 'testUser123',
312+
credentialsId : 'OauthDataFileId',
313+
credentialType: 'SecretFile'
314+
],
316315
)
317316

318-
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\""))
319-
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\""))
317+
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"))
318+
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"))
320319
}
321320

322321
@Test

‎vars/artifactSetVersion.groovy‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import static com.sap.piper.Prerequisites.checkScript
2+
import static com.sap.piper.BashUtils.quoteAndEscape as q
23

34
import com.sap.piper.GenerateDocumentation
45
import com.sap.piper.ConfigurationHelper
@@ -175,7 +176,7 @@ void call(Map parameters = [:], Closure body = null) {
175176
def gitConfig = []
176177

177178
if(config.gitUserEMail) {
178-
gitConfig.add("-c user.email=\"${config.gitUserEMail}\"")
179+
gitConfig.add("-c user.email=${q(config.gitUserEMail)}")
179180
} else {
180181
// in case there is no user.email configured on project level we might still
181182
// 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) {
184185
}
185186
}
186187
if(config.gitUserName) {
187-
gitConfig.add("-c user.name=\"${config.gitUserName}\"")
188+
gitConfig.add("-c user.name=${q(config.gitUserName)}")
188189
} else {
189190
// in case there is no user.name configured on project level we might still
190191
// 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) {
199200
set -e
200201
git add . --update
201202
git ${gitConfig} commit -m 'update version ${newVersion}'
202-
git tag ${config.tagPrefix}${newVersion}"""
203+
git tag ${q(config.tagPrefix+newVersion)}"""
203204
config.gitCommitId = gitUtils.getGitCommitIdOrNull()
204205
} catch (e) {
205206
error "[${STEP_NAME}]git commit and tag failed: ${e}"
@@ -215,7 +216,7 @@ void call(Map parameters = [:], Closure body = null) {
215216
.use()
216217

217218
sshagent([config.gitSshKeyCredentialsId]) {
218-
sh "git push ${config.gitSshUrl} ${config.tagPrefix}${newVersion}"
219+
sh "git push ${q(config.gitSshUrl)} ${q(config.tagPrefix+newVersion)}"
219220
}
220221

221222
} else if(gitPushMode == GitPushMode.HTTPS) {
@@ -259,7 +260,7 @@ void call(Map parameters = [:], Closure body = null) {
259260
gitConfig = []
260261

261262
if(config.gitHttpProxy) {
262-
gitConfig.add("-c http.proxy=\"${config.gitHttpProxy}\"")
263+
gitConfig.add("-c http.proxy=${q(config.gitHttpProxy)}")
263264
}
264265

265266
if(config.gitDisableSslVerification) {
@@ -288,7 +289,7 @@ void call(Map parameters = [:], Closure body = null) {
288289
gitPushFlags = gitPushFlags.join(' ')
289290

290291
sh script: """|#!/bin/bash ${hashbangFlags}
291-
|${gitDebug}git ${gitConfig} push ${gitPushFlags} ${gitUrlWithCredentials} ${config.tagPrefix}${newVersion} ${streamhandling}""".stripMargin()
292+
|${gitDebug}git ${gitConfig} push ${gitPushFlags} ${gitUrlWithCredentials} ${q(config.tagPrefix+newVersion)} ${streamhandling}""".stripMargin()
292293
}
293294
} else {
294295
echo "Git push mode: ${gitPushMode.toString()}. Git push to remote has been skipped."
@@ -313,5 +314,5 @@ def isAppContainer(config){
313314
}
314315

315316
def getTimestamp(pattern){
316-
return sh(returnStdout: true, script: "date --utc +'${pattern}'").trim()
317+
return sh(returnStdout: true, script: "date --utc +${q(pattern)}").trim()
317318
}

‎vars/containerPushToRegistry.groovy‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import com.sap.piper.DockerUtils
55
import groovy.transform.Field
66

77
import static com.sap.piper.Prerequisites.checkScript
8+
import static com.sap.piper.BashUtils.quoteAndEscape as q
89

910
@Field String STEP_NAME = getClass().getName()
1011
@Field Set GENERAL_CONFIG_KEYS = [
@@ -101,7 +102,7 @@ void call(Map parameters = [:]) {
101102
) {
102103
sourceBuildImage.pull()
103104
}
104-
sh "docker tag ${config.sourceRegistry}/${config.sourceImage} ${config.dockerImage}"
105+
sh "docker tag ${q(config.sourceRegistry)}/${q(config.sourceImage)} ${q(config.dockerImage)}"
105106
}
106107

107108
docker.withRegistry(

‎vars/dockerExecute.groovy‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import com.sap.piper.SidecarUtils
22

33
import static com.sap.piper.Prerequisites.checkScript
4+
import static com.sap.piper.BashUtils.quoteAndEscape as q
45

56
import com.cloudbees.groovy.cps.NonCPS
67
import com.sap.piper.ConfigurationHelper
@@ -242,7 +243,7 @@ void call(Map parameters = [:], body) {
242243
}
243244
} else {
244245
def networkName = "sidecar-${UUID.randomUUID()}"
245-
sh "docker network create ${networkName}"
246+
sh "docker network create ${q(networkName)}"
246247
try {
247248
def sidecarImage = docker.image(config.sidecarImage)
248249
pullWrapper(config.sidecarPullImage, sidecarImage, config.sidecarRegistryUrl, config.sidecarRegistryCredentialsId) {

‎vars/healthExecuteCheck.groovy‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import static com.sap.piper.Prerequisites.checkScript
2+
import static com.sap.piper.BashUtils.quoteAndEscape as q
23

34
import com.sap.piper.GenerateDocumentation
45
import com.sap.piper.ConfigurationHelper
@@ -70,6 +71,6 @@ void call(Map parameters = [:]) {
7071
def curl(url){
7172
return sh(
7273
returnStdout: true,
73-
script: "curl -so /dev/null -w '%{response_code}' ${url}"
74+
script: "curl -so /dev/null -w '%{response_code}' ${q(url)}"
7475
).trim()
7576
}

0 commit comments

Comments
 (0)