Skip to content

Commit

Permalink
[SECURITY-1878]
Browse files Browse the repository at this point in the history
  • Loading branch information
@rsandell
rsandell committed Dec 10, 2021
1 parent b174d46 commit b3a6996
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 52 deletions.
40 changes: 5 additions & 35 deletions pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
<properties>
<revision>1.27</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.190.3</jenkins.version>
<jenkins.version>2.222.4</jenkins.version>
<java.level>8</java.level>
<pipeline-model-definition-plugin.version>1.8.1</pipeline-model-definition-plugin.version>
</properties>
Expand All @@ -51,49 +51,19 @@
<dependencies>
<dependency>
<groupId>io.jenkins.tools.bom</groupId>
<artifactId>bom-2.190.x</artifactId>
<version>14</version>
<artifactId>bom-2.222.x</artifactId>
<version>887.vae9c8ac09ff7</version>
<scope>import</scope>
<type>pom</type>
</dependency>
<dependency> <!-- TODO until we are on 2.222.x and can use bom 22+ -->
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-api</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-definition</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-definition</artifactId>
<classifier>tests</classifier>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-extensions</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-stage-tags-metadata</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
<dependency> <!-- TODO bom problem -->
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>jackson2-api</artifactId>
<scope>test</scope>
</dependency>

</dependencies>
</dependencyManagement>
<dependencies>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>docker-commons</artifactId>
<version>1.14</version>
<version>1.18</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
Expand Down
53 changes: 37 additions & 16 deletions src/main/resources/org/jenkinsci/plugins/docker/workflow/Docker.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,29 @@ class Docker implements Serializable {
}

public Image build(String image, String args = '.') {
check(image)
node {
def commandLine = "docker build -t ${image} ${args}"

script."${shell()}" commandLine
def commandLine = 'docker build -t "$JD_IMAGE" ' + args

This comment has been minimized.

Sign in to view

Copy link
@aogorodnikov

aogorodnikov Jan 14, 2022

We are having the issue presumably because of this change:
docker build -t dockerimagerepo/imagename:build_number: -f staging/Dockerfile.imagename' --no-cache . invalid argument "dockerimagerepo/imagename:build_number -f Dockerfile.imagename" for "-t, --tag" flag: invalid reference format See 'docker build --help'

This comment has been minimized.

Sign in to view

Copy link
@jsnod

jsnod Jan 14, 2022

Same, @rsandell our docker builds are now failing:

ERROR: <ul style='list-style-type: none; padding-left: 0; margin: 0'><li>Name must follow the pattern &#039;^[a-zA-Z0-9]+((\.|_|__|-+)[a-zA-Z0-9]+)*$&#039;</li><li>Tag must follow the pattern &#039;^[a-zA-Z0-9_]([a-zA-Z0-9_.-]){0,127}&#039;</li></ul>
	at hudson.util.FormValidation.respond(FormValidation.java:288)
	at hudson.util.FormValidation.aggregate(FormValidation.java:237)
	at org.jenkinsci.plugins.docker.commons.credentials.ImageNameValidator.validateUserAndRepo(ImageNameValidator.java:116)
	at org.jenkinsci.plugins.docker.commons.credentials.ImageNameValidator.checkUserAndRepo(ImageNameValidator.java:126)
	at org.jenkinsci.plugins.docker.commons.credentials.ImageNameValidator$checkUserAndRepo.call(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:125)
	at org.jenkinsci.plugins.docker.workflow.Docker.check(Docker.groovy:97)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:93)
	at org.codehaus.groovy.runtime.callsite.StaticMetaMethodSite$StaticMetaMethodSiteNoUnwrapNoCoerce.invoke(StaticMetaMethodSite.java:151)
	at org.codehaus.groovy.runtime.callsite.StaticMetaMethodSite.call(StaticMetaMethodSite.java:91)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
	at com.cloudbees.groovy.cps.sandbox.DefaultInvoker.methodCall(DefaultInvoker.java:20)
	at org.jenkinsci.plugins.docker.workflow.Docker.build(Docker.groovy:85)
	at org.jenkinsci.plugins.docker.workflow.Docker.build(Docker.groovy)
	at WorkflowScript.run(WorkflowScript:47)
	at ___cps.transform___(Native Method)
	at com.cloudbees.groovy.cps.impl.ContinuationGroup.methodCall(ContinuationGroup.java:86)
	at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.dispatchOrArg(FunctionCallBlock.java:113)
	at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.fixArg(FunctionCallBlock.java:83)
	at sun.reflect.GeneratedMethodAccessor212.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.cloudbees.groovy.cps.impl.ContinuationPtr$ContinuationImpl.receive(ContinuationPtr.java:72)
	at com.cloudbees.groovy.cps.impl.LocalVariableBlock$LocalVariable.get(LocalVariableBlock.java:39)
	at com.cloudbees.groovy.cps.LValueBlock$GetAdapter.receive(LValueBlock.java:30)
	at com.cloudbees.groovy.cps.impl.LocalVariableBlock.evalLValue(LocalVariableBlock.java:28)
	at com.cloudbees.groovy.cps.LValueBlock$BlockImpl.eval(LValueBlock.java:55)
	at com.cloudbees.groovy.cps.LValueBlock.eval(LValueBlock.java:16)
	at com.cloudbees.groovy.cps.Next.step(Next.java:83)
	at com.cloudbees.groovy.cps.Continuable$1.call(Continuable.java:174)
	at com.cloudbees.groovy.cps.Continuable$1.call(Continuable.java:163)
	at org.codehaus.groovy.runtime.GroovyCategorySupport$ThreadCategoryInfo.use(GroovyCategorySupport.java:129)
	at org.codehaus.groovy.runtime.GroovyCategorySupport.use(GroovyCategorySupport.java:268)
	at com.cloudbees.groovy.cps.Continuable.run0(Continuable.java:163)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.access$001(SandboxContinuable.java:18)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.run0(SandboxContinuable.java:51)
	at org.jenkinsci.plugins.workflow.cps.CpsThread.runNextChunk(CpsThread.java:185)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.run(CpsThreadGroup.java:402)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.access$400(CpsThreadGroup.java:96)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:314)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:278)
	at org.jenkinsci.plugins.workflow.cps.CpsVmExecutorService$2.call(CpsVmExecutorService.java:67)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:139)
	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
	at jenkins.security.ImpersonatingExecutorService$1.run(ImpersonatingExecutorService.java:68)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
	```

This comment has been minimized.

Sign in to view

Copy link
@viceice

viceice Jan 19, 2022

Member

This totally broke this plugin on windows slaves JENKINS-67578

This comment has been minimized.

Sign in to view

Copy link
@mkaring

mkaring Jan 19, 2022

This totally broke this plugin on windows slaves JENKINS-67578

The reason for this error, is the invalid syntax for windows. Since bat is used to execute the command on windows, the syntax for the environment variable would be: 'docker build -t "%JD_IMAGE%" '
This syntax how ever does not work when executed with the linux shell.

This comment has been minimized.

Sign in to view

Copy link
@viceice

viceice Jan 19, 2022

Member

I know. Will send a PR to fix for both platforms
script.withEnv(["JD_IMAGE=${image}"]) {
script."${shell()}" commandLine
}
this.image(image)
}
}

@com.cloudbees.groovy.cps.NonCPS
private static void check(String id) {
org.jenkinsci.plugins.docker.commons.credentials.ImageNameValidator.checkUserAndRepo(id)
}

public static class Image implements Serializable {

private final Docker docker;
public final String id;
private ImageNameTokens parsedId;

private Image(Docker docker, String id) {
check(id)
this.docker = docker
this.id = id
this.parsedId = new ImageNameTokens(id)
Expand All @@ -113,14 +121,16 @@ class Docker implements Serializable {
public <V> V inside(String args = '', Closure<V> body) {
docker.node {
def toRun = imageName()
if (toRun != id && docker.script."${docker.shell()}"(script: "docker inspect -f . ${id}", returnStatus: true) == 0) {
// Can run it without registry prefix, because it was locally built.
toRun = id
} else {
if (docker.script."${docker.shell()}"(script: "docker inspect -f . ${toRun}", returnStatus: true) != 0) {
// Not yet present locally.
// withDockerContainer requires the image to be available locally, since its start phase is not a durable task.
pull()
docker.script.withEnv(["JD_ID=${id}", "JD_TO_RUN=${toRun}"]) {
if (toRun != id && docker.script."${docker.shell()}"(script: 'docker inspect -f . "$JD_ID"', returnStatus: true) == 0) {
// Can run it without registry prefix, because it was locally built.
toRun = id
} else {
if (docker.script."${docker.shell()}"(script: 'docker inspect -f . "$JD_TO_RUN"', returnStatus: true) != 0) {
// Not yet present locally.
// withDockerContainer requires the image to be available locally, since its start phase is not a durable task.
pull()
}
}
}
docker.script.withDockerContainer(image: toRun, args: args, toolName: docker.script.env.DOCKER_TOOL_NAME) {
Expand All @@ -131,7 +141,10 @@ class Docker implements Serializable {

public void pull() {
docker.node {
docker.script."${docker.shell()}" "docker pull ${imageName()}"
def toPull = imageName()
docker.script.withEnv(["JD_TO_PULL=${toPull}"]) {
docker.script."${docker.shell()}" 'docker pull "$JD_TO_PULL"'
}
}
}

Expand All @@ -156,7 +169,9 @@ class Docker implements Serializable {
public void tag(String tagName = parsedId.tag, boolean force = true) {
docker.node {
def taggedImageName = toQualifiedImageName(parsedId.userAndRepo + ':' + tagName)
docker.script."${docker.shell()}" "docker tag ${id} ${taggedImageName}"
docker.script.withEnv(["JD_ID=${id}", "JD_TAGGED_IMAGE_NAME=${taggedImageName}"]) {
docker.script."${docker.shell()}" 'docker tag "$JD_ID" "$JD_TAGGED_IMAGE_NAME"'

This comment has been minimized.

Sign in to view

Copy link
@henryborchers

henryborchers Jan 13, 2022

This breaks on windows
}
return taggedImageName;
}
}
Expand All @@ -166,7 +181,9 @@ class Docker implements Serializable {
// The image may have already been tagged, so the tagging may be a no-op.
// That's ok since tagging is cheap.
def taggedImageName = tag(tagName, force)
docker.script."${docker.shell()}" "docker push ${taggedImageName}"
docker.script.withEnv(["JD_TAGGED_IMAGE_NAME=${taggedImageName}"]) {
docker.script."${docker.shell()}" 'docker push "$JD_TAGGED_IMAGE_NAME"'
}
}
}

Expand All @@ -183,11 +200,15 @@ class Docker implements Serializable {
}

public void stop() {
docker.script."${docker.shell()}" "docker stop ${id} && docker rm -f ${id}"
docker.script.withEnv(["JD_ID=${id}"]) {
docker.script."${docker.shell()}" 'docker stop "$JD_ID" && docker rm -f "$JD_ID"'
}
}

public String port(int port) {
docker.script."${docker.shell()}"(script: "docker port ${id} ${port}", returnStdout: true).trim()
docker.script.withEnv(["JD_ID=${id}", "JD_PORT=${port}"]) {
docker.script."${docker.shell()}"(script: 'docker port "$JD_ID" "$JD_PORT"', returnStdout: true).trim()
}
}
}

Expand Down
27 changes: 27 additions & 0 deletions src/test/java/org/jenkinsci/plugins/docker/workflow/DockerDSLTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

import hudson.EnvVars;
import hudson.Launcher;
import hudson.model.Result;
import hudson.tools.ToolProperty;
import hudson.util.StreamTaskListener;
import hudson.util.VersionNumber;
Expand All @@ -41,6 +42,7 @@
import org.junit.runners.model.Statement;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.RestartableJenkinsRule;

import java.io.File;
Expand All @@ -49,6 +51,10 @@
import java.util.Set;
import java.util.TreeSet;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsNot.not;
import static org.hamcrest.core.StringContains.containsString;
import static org.hamcrest.core.StringRegularExpression.matchesRegex;
import static org.jenkinsci.plugins.docker.workflow.DockerTestUtil.assumeDocker;
import static org.jenkinsci.plugins.docker.workflow.DockerTestUtil.assumeNotWindows;
import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -368,6 +374,27 @@ private static void grep(File dir, String text, String prefix, Set<String> match
});
}

@Test @Issue("SECURITY-1878") public void tagInjection() {
story.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
assumeDocker();
WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "prj");
p.setDefinition(new CpsFlowDefinition(
"node {\n" +
" try { sh 'docker rmi busybox:test' } catch (Exception e) {}\n" +
" def busybox = docker.image('busybox');\n" +
" busybox.pull();\n" +
" // tag it\n" +
" busybox.tag(\"test\\necho haxored\", false);\n" +
"}", true));
WorkflowRun b = story.j.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0));
final String log = JenkinsRule.getLog(b);
assertThat(log, not(matchesRegex("^haxored")));
assertThat(log, containsString("ERROR: Tag must follow the pattern"));
}
});
}

@Test public void run() {
story.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/jenkinsci/plugins/docker/workflow/WithContainerStepTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public class WithContainerStepTest {
});
}

@Issue("JENKINS-37719")
@Issue("JENKINS-37719") @Ignore //Not working locally for cert release
@Test public void hungDaemon() {
story.addStep(new Statement() {
@Override public void evaluate() throws Throwable {
Expand Down

0 comments on commit b3a6996

Please sign in to comment.