CVE-2022-29184: Improper Neutralization of Special Elements used in a Command ('Command Injection') (CWE-77)
value: GoCD is a continuous delivery server. In GoCD versions prior to 22.1.0, it is possible for existing authenticated users who have permissions to edit or create pipeline materials or pipeline configuration repositories to get remote code execution capability on the GoCD server via configuring a malicious branch name which abuses Mercurial hooks/aliases to exploit a command injection weakness. An attacker would require access to an account with existing GoCD administration permissions to either create/edit (`hg`-based) configuration repositories; create/edit pipelines and their (`hg`-based) materials; or, where "pipelines-as-code" configuration repositories are used, to commit malicious configuration to such an external repository which will be automatically parsed into a pipeline configuration and (`hg`) material definition by the GoCD server. This issue is fixed in GoCD 22.1.0. As a workaround, users who do not use/rely upon Mercurial materials can uninstall/remove the `hg`/Mercurial binary from the underlying GoCD Server operating system or Docker image.
CWE description: The product constructs all or part of a command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended command when it is sent to a downstream component.
CVSS v3 base score: 8.8
Published: 2022-05-20T20:15Z
Repository: https://github.com/gocd/gocd
Language: Java
Files changed: common/src/test/java/com/thoughtworks/go/domain/materials/mercurial/HgCommandTest.java, domain/src/main/java/com/thoughtworks/go/domain/materials/mercurial/HgCommand.java
Commit date: 2022-03-04 20:29:44 +0800
Commit message: Improve escaping of arguments when constructing Hg command calls

Diff: diff --git a/common/src/test/java/com/thoughtworks/go/domain/materials/mercurial/HgCommandTest.java b/common/src/test/java/com/thoughtworks/go/domain/materials/mercurial/HgCommandTest.java
index be9a5d62d26..6c8ac66f998 100644
--- a/common/src/test/java/com/thoughtworks/go/domain/materials/mercurial/HgCommandTest.java
+++ b/common/src/test/java/com/thoughtworks/go/domain/materials/mercurial/HgCommandTest.java
@@ -33,55 +33,77 @@ import java.util.ArrayList;
 import java.util.List;
 
 import static com.thoughtworks.go.util.command.ProcessOutputStreamConsumer.inMemoryConsumer;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.*;
 
 public class HgCommandTest {
+
+    private static final String REVISION_0 = "b61d12de515d82d3a377ae3aae6e8abe516a2651";
+    private static final String REVISION_1 = "35ff2159f303ecf986b3650fc4299a6ffe5a14e1";
+    private static final String REVISION_2 = "ca3ebb67f527c0ad7ed26b789056823d8b9af23f";
+
     private File serverRepo;
     private File clientRepo;
 
     private HgCommand hgCommand;
 
     private InMemoryStreamConsumer outputStreamConsumer = inMemoryConsumer();
     private File workingDirectory;
-    private static final String REVISION_0 = "b61d12de515d82d3a377ae3aae6e8abe516a2651";
-    private static final String REVISION_1 = "35ff2159f303ecf986b3650fc4299a6ffe5a14e1";
-    private static final String REVISION_2 = "ca3ebb67f527c0ad7ed26b789056823d8b9af23f";
     private File secondBranchWorkingCopy;
 
     @BeforeEach
     public void setUp(@TempDir Path tempDir) throws IOException {
         serverRepo = TempDirUtils.createTempDirectoryIn(tempDir, "testHgServerRepo").toFile();
-        clientRepo =  TempDirUtils.createTempDirectoryIn(tempDir, "testHgClientRepo").toFile();
-        secondBranchWorkingCopy =  TempDirUtils.createTempDirectoryIn(tempDir, "second").toFile();
+        clientRepo = TempDirUtils.createTempDirectoryIn(tempDir, "testHgClientRepo").toFile();
+        secondBranchWorkingCopy = TempDirUtils.createTempDirectoryIn(tempDir, "second").toFile();
 
         setUpServerRepoFromHgBundle(serverRepo, new File("../common/src/test/resources/data/hgrepo.hgbundle"));
         workingDirectory = new File(clientRepo.getPath());
         hgCommand = new HgCommand(null, workingDirectory, "default", serverRepo.getAbsolutePath(), null);
         hgCommand.clone(outputStreamConsumer, new UrlArgument(serverRepo.getAbsolutePath()));
     }
 
     @Test
     public void shouldCloneFromRemoteRepo() {
-        assertThat(clientRepo.listFiles().length > 0, is(true));
+        assertThat(clientRepo.listFiles().length, is(2));
+    }
+
+    @Test
+    public void shouldCloneWithEscapedRepoUrl() {
+        hgCommand.clone(outputStreamConsumer, new UrlArgument(echoingAliasFor("clone")));
+        assertNoUnescapedEcho();
     }
 
     @Test
-    public void shouldGetLatestModifications() throws Exception {
+    public void shouldCloneWithEscapedBranch() {
+        hgCommand = new HgCommand(null, workingDirectory, echoingAliasFor("clone"), serverRepo.getAbsolutePath(), null);
+        hgCommand.clone(outputStreamConsumer, new UrlArgument(serverRepo.getAbsolutePath()));
+        assertNoUnescapedEcho();
+    }
+
+    private String echoingAliasFor(String command) {
+        return String.format("--config=alias.%s=!echo hello world", command);
+    }
+
+    private void assertNoUnescapedEcho() {
+        assertThat(outputStreamConsumer.getAllOutput(), not(containsString("\nhello world\n")));
+    }
+
+    @Test
+    public void shouldGetLatestModifications() {
         List<Modification> actual = hgCommand.latestOneModificationAsModifications();
         assertThat(actual.size(), is(1));
         final Modification modification = actual.get(0);
         assertThat(modification.getComment(), is("test"));
         assertThat(modification.getUserName(), is("cruise"));
         assertThat(modification.getModifiedFiles().size(), is(1));
     }
 
     @Test
-    public void shouldNotIncludeCommitFromAnotherBranchInGetLatestModifications() throws Exception {
+    public void shouldNotIncludeCommitFromAnotherBranchInGetLatestModifications() {
         Modification lastCommit = hgCommand.latestOneModificationAsModifications().get(0);
 
         makeACommitToSecondBranch();
         hg(workingDirectory, "pull").runOrBomb(null);
         Modification actual = hgCommand.latestOneModificationAsModifications().get(0);
@@ -96,11 +118,11 @@ public class HgCommandTest {
         assertThat(actual.get(0).getRevision(), is(REVISION_2));
         assertThat(actual.get(1).getRevision(), is(REVISION_1));
     }
 
     @Test
-    public void shouldNotGetModificationsFromOtherBranches() throws Exception {
+    public void shouldNotGetModificationsFromOtherBranches() {
         makeACommitToSecondBranch();
         hg(workingDirectory, "pull").runOrBomb(null);
 
         List<Modification> actual = hgCommand.modificationsSince(new StringRevision(REVISION_0));
         assertThat(actual.size(), is(2));
@@ -129,31 +151,45 @@ public class HgCommandTest {
         hgCommand.updateTo(new StringRevision("tip"), output);
         assertThat(newFile.exists(), is(false));
     }
 
     @Test
-    public void shouldThrowExceptionIfUpdateFails() throws Exception {
+    public void shouldThrowExceptionIfUpdateFails() {
         InMemoryStreamConsumer output =
                 ProcessOutputStreamConsumer.inMemoryConsumer();
 
         // delete repository in order to fail the hg pull command
         assertThat(FileUtils.deleteQuietly(serverRepo), is(true));
 
         // now hg pull will fail and throw an exception
         assertThatThrownBy(() -> hgCommand.updateTo(new StringRevision("tip"), output))
-                .isExactlyInstanceOf(RuntimeException.class);
+                .isExactlyInstanceOf(RuntimeException.class)
+                .hasMessageContaining("Unable to update to revision [StringRevision[tip]]");
     }
 
     @Test
     public void shouldGetWorkingUrl() {
         String workingUrl = hgCommand.workingRepositoryUrl().outputAsString();
 
         assertThat(workingUrl, is(serverRepo.getAbsolutePath()));
     }
 
     @Test
-    public void shouldThrowExceptionForBadConnection() throws Exception {
+    public void shouldCheckConnection() {
+        hgCommand.checkConnection(new UrlArgument(serverRepo.getAbsolutePath()));
+    }
+
+    @Test
+    public void shouldCheckConnectionWithEscapedRepoUrl() {
+        assertThatThrownBy(() -> hgCommand.checkConnection(new UrlArgument(echoingAliasFor("id"))))
+                .isExactlyInstanceOf(CommandLineException.class)
+                .hasMessageContaining("repository --config")
+                .hasMessageContaining("not found");
+    }
+
+    @Test
+    public void shouldThrowExceptionForBadConnection() {
         String url = "http://not-exists";
         HgCommand hgCommand = new HgCommand(null, null, null, null, null);
 
         assertThatThrownBy(() -> hgCommand.checkConnection(new UrlArgument(url)))
                 .isExactlyInstanceOf(CommandLineException.class);
diff --git a/domain/src/main/java/com/thoughtworks/go/domain/materials/mercurial/HgCommand.java b/domain/src/main/java/com/thoughtworks/go/domain/materials/mercurial/HgCommand.java
index bf92af253b1..a4e614c5e8c 100644
--- a/domain/src/main/java/com/thoughtworks/go/domain/materials/mercurial/HgCommand.java
+++ b/domain/src/main/java/com/thoughtworks/go/domain/materials/mercurial/HgCommand.java
@@ -22,12 +22,12 @@ import com.thoughtworks.go.util.NamedProcessTag;
 import com.thoughtworks.go.util.command.*;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.File;
-import java.io.UnsupportedEncodingException;
 import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.List;
 
 import static com.thoughtworks.go.util.ExceptionUtils.bomb;
 import static com.thoughtworks.go.util.ExceptionUtils.bombUnless;
@@ -50,39 +50,48 @@ public class HgCommand extends SCMCommand {
         this.branch = branch;
         this.url = url;
         this.secrets = secrets != null ? secrets : new ArrayList<>();
     }
 
-
-    private boolean pull(ConsoleOutputStreamConsumer outputStreamConsumer) {
-        CommandLine hg = hg("pull", "-b", branch, "--config", String.format("paths.default=%s", url));
-        return execute(hg, outputStreamConsumer) == 0;
-    }
-
     public HgVersion version() {
-        CommandLine hg = createCommandLine("hg").withArgs("version").withEncoding("utf-8");
+        CommandLine hg = createCommandLine("hg").withArgs("version").withEncoding("UTF-8");
         String hgOut = execute(hg, new NamedProcessTag("hg version check")).outputAsString();
         return HgVersion.parse(hgOut);
     }
 
-
     public int clone(ConsoleOutputStreamConsumer outputStreamConsumer, UrlArgument repositoryUrl) {
-        CommandLine hg = createCommandLine("hg").withArgs("clone").withArg("-b").withArg(branch).withArg(repositoryUrl)
-                .withArg(workingDir.getAbsolutePath()).withNonArgSecrets(secrets).withEncoding("utf-8");
+        CommandLine hg = createCommandLine("hg")
+                .withArgs("clone")
+                .withArg(branchArg())
+                .withArg("--")
+                .withArg(repositoryUrl)
+                .withArg(workingDir.getAbsolutePath())
+                .withNonArgSecrets(secrets)
+                .withEncoding("UTF-8");
         return execute(hg, outputStreamConsumer);
     }
 
     public void checkConnection(UrlArgument repositoryURL) {
-        execute(createCommandLine("hg").withArgs("id", "--id").withArg(repositoryURL).withNonArgSecrets(secrets).withEncoding("utf-8"), new NamedProcessTag(repositoryURL.forDisplay()));
+        CommandLine hg = createCommandLine("hg")
+                .withArgs("id", "--id", "--")
+                .withArg(repositoryURL)
+                .withNonArgSecrets(secrets)
+                .withEncoding("UTF-8");
+        execute(hg, new NamedProcessTag(repositoryURL.forDisplay()));
     }
 
     public void updateTo(Revision revision, ConsoleOutputStreamConsumer outputStreamConsumer) {
         if (!pull(outputStreamConsumer) || !update(revision, outputStreamConsumer)) {
             bomb(format("Unable to update to revision [%s]", revision));
         }
     }
 
+    private boolean pull(ConsoleOutputStreamConsumer outputStreamConsumer) {
+        CommandLine hg = hg("pull", branchArg(), "--config", String.format("paths.default=%s", url));
+        return execute(hg, outputStreamConsumer) == 0;
+    }
+
     private boolean update(Revision revision, ConsoleOutputStreamConsumer outputStreamConsumer) {
         CommandLine hg = hg("update", "--clean", "-r", revision.getRevision());
         return execute(hg, outputStreamConsumer) == 0;
     }
 
@@ -103,49 +112,46 @@ public class HgCommand extends SCMCommand {
 
     public List<Modification> latestOneModificationAsModifications() {
         return findRecentModifications(1);
     }
 
-    private String templatePath() {
-        if (templatePath == null) {
-            String file = HgCommand.class.getResource("/hg.template").getFile();
-            try {
-                templatePath = URLDecoder.decode(new File(file).getAbsolutePath(), "UTF-8");
-            } catch (UnsupportedEncodingException e) {
-                templatePath = URLDecoder.decode(new File(file).getAbsolutePath());
-            }
-        }
-        return templatePath;
-    }
-
-    List<Modification> findRecentModifications(int count) {
+    private List<Modification> findRecentModifications(int count) {
         // Currently impossible to check modifications on a remote repository.
         InMemoryStreamConsumer consumer = inMemoryConsumer();
         bombUnless(pull(consumer), "Failed to run hg pull command: " + consumer.getAllOutput());
-        CommandLine hg = hg("log", "--limit", String.valueOf(count), "-b", branch, "--style", templatePath());
+        CommandLine hg = hg("log", "--limit", String.valueOf(count), branchArg(), "--style", templatePath());
         return new HgModificationSplitter(execute(hg)).modifications();
     }
 
     public List<Modification> modificationsSince(Revision revision) {
         InMemoryStreamConsumer consumer = inMemoryConsumer();
         bombUnless(pull(consumer), "Failed to run hg pull command: " + consumer.getAllOutput());
-        CommandLine hg = hg("log",
-                "-r", "tip:" + revision.getRevision(),
-                "-b", branch,
-                "--style", templatePath());
+        CommandLine hg = hg("log", "-r", "tip:" + revision.getRevision(), branchArg(), "--style", templatePath());
         return new HgModificationSplitter(execute(hg)).filterOutRevision(revision);
     }
 
+    private String templatePath() {
+        if (templatePath == null) {
+            String file = HgCommand.class.getResource("/hg.template").getFile();
+            templatePath = URLDecoder.decode(new File(file).getAbsolutePath(), StandardCharsets.UTF_8);
+        }
+        return templatePath;
+    }
+
     public ConsoleResult workingRepositoryUrl() {
         CommandLine hg = hg("showconfig", "paths.default");
 
         final ConsoleResult result = execute(hg);
         LOGGER.trace("Current repository url of [{}]: {}", workingDir, result.outputForDisplayAsString());
         LOGGER.trace("Target repository url: {}", url);
         return result;
     }
 
+    private String branchArg() {
+        return "--branch=" + branch;
+    }
+
     private CommandLine hg(String... arguments) {
         return createCommandLine("hg").withArgs(arguments).withNonArgSecrets(secrets).withWorkingDir(workingDir).withEncoding("UTF-8");
     }
 
     private static ConsoleResult execute(CommandLine hgCmd, NamedProcessTag processTag) {

Vulnerable code: import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
    private static final String REVISION_0 = "b61d12de515d82d3a377ae3aae6e8abe516a2651";
    private static final String REVISION_1 = "35ff2159f303ecf986b3650fc4299a6ffe5a14e1";
    private static final String REVISION_2 = "ca3ebb67f527c0ad7ed26b789056823d8b9af23f";
        clientRepo =  TempDirUtils.createTempDirectoryIn(tempDir, "testHgClientRepo").toFile();
        secondBranchWorkingCopy =  TempDirUtils.createTempDirectoryIn(tempDir, "second").toFile();
        assertThat(clientRepo.listFiles().length > 0, is(true));
    public void shouldGetLatestModifications() throws Exception {
    public void shouldNotIncludeCommitFromAnotherBranchInGetLatestModifications() throws Exception {
    public void shouldNotGetModificationsFromOtherBranches() throws Exception {
    public void shouldThrowExceptionIfUpdateFails() throws Exception {
                .isExactlyInstanceOf(RuntimeException.class);
    public void shouldThrowExceptionForBadConnection() throws Exception {
import java.io.UnsupportedEncodingException;

    private boolean pull(ConsoleOutputStreamConsumer outputStreamConsumer) {
        CommandLine hg = hg("pull", "-b", branch, "--config", String.format("paths.default=%s", url));
        return execute(hg, outputStreamConsumer) == 0;
    }

        CommandLine hg = createCommandLine("hg").withArgs("version").withEncoding("utf-8");

        CommandLine hg = createCommandLine("hg").withArgs("clone").withArg("-b").withArg(branch).withArg(repositoryUrl)
                .withArg(workingDir.getAbsolutePath()).withNonArgSecrets(secrets).withEncoding("utf-8");
        execute(createCommandLine("hg").withArgs("id", "--id").withArg(repositoryURL).withNonArgSecrets(secrets).withEncoding("utf-8"), new NamedProcessTag(repositoryURL.forDisplay()));
    private String templatePath() {
        if (templatePath == null) {
            String file = HgCommand.class.getResource("/hg.template").getFile();
            try {
                templatePath = URLDecoder.decode(new File(file).getAbsolutePath(), "UTF-8");
            } catch (UnsupportedEncodingException e) {
                templatePath = URLDecoder.decode(new File(file).getAbsolutePath());
            }
        }
        return templatePath;
    }

    List<Modification> findRecentModifications(int count) {
        CommandLine hg = hg("log", "--limit", String.valueOf(count), "-b", branch, "--style", templatePath());
        CommandLine hg = hg("log",
                "-r", "tip:" + revision.getRevision(),
                "-b", branch,
                "--style", templatePath());
Fixed code: import static org.hamcrest.Matchers.*;

    private static final String REVISION_0 = "b61d12de515d82d3a377ae3aae6e8abe516a2651";
    private static final String REVISION_1 = "35ff2159f303ecf986b3650fc4299a6ffe5a14e1";
    private static final String REVISION_2 = "ca3ebb67f527c0ad7ed26b789056823d8b9af23f";

        clientRepo = TempDirUtils.createTempDirectoryIn(tempDir, "testHgClientRepo").toFile();
        secondBranchWorkingCopy = TempDirUtils.createTempDirectoryIn(tempDir, "second").toFile();
        assertThat(clientRepo.listFiles().length, is(2));
    }

    @Test
    public void shouldCloneWithEscapedRepoUrl() {
        hgCommand.clone(outputStreamConsumer, new UrlArgument(echoingAliasFor("clone")));
        assertNoUnescapedEcho();
    public void shouldCloneWithEscapedBranch() {
        hgCommand = new HgCommand(null, workingDirectory, echoingAliasFor("clone"), serverRepo.getAbsolutePath(), null);
        hgCommand.clone(outputStreamConsumer, new UrlArgument(serverRepo.getAbsolutePath()));
        assertNoUnescapedEcho();
    }

    private String echoingAliasFor(String command) {
        return String.format("--config=alias.%s=!echo hello world", command);
    }

    private void assertNoUnescapedEcho() {
        assertThat(outputStreamConsumer.getAllOutput(), not(containsString("\nhello world\n")));
    }

    @Test
    public void shouldGetLatestModifications() {
    public void shouldNotIncludeCommitFromAnotherBranchInGetLatestModifications() {
    public void shouldNotGetModificationsFromOtherBranches() {
    public void shouldThrowExceptionIfUpdateFails() {
                .isExactlyInstanceOf(RuntimeException.class)
                .hasMessageContaining("Unable to update to revision [StringRevision[tip]]");
    public void shouldCheckConnection() {
        hgCommand.checkConnection(new UrlArgument(serverRepo.getAbsolutePath()));
    }

    @Test
    public void shouldCheckConnectionWithEscapedRepoUrl() {
        assertThatThrownBy(() -> hgCommand.checkConnection(new UrlArgument(echoingAliasFor("id"))))
                .isExactlyInstanceOf(CommandLineException.class)
                .hasMessageContaining("repository --config")
                .hasMessageContaining("not found");
    }

    @Test
    public void shouldThrowExceptionForBadConnection() {
import java.nio.charset.StandardCharsets;
        CommandLine hg = createCommandLine("hg").withArgs("version").withEncoding("UTF-8");
        CommandLine hg = createCommandLine("hg")
                .withArgs("clone")
                .withArg(branchArg())
                .withArg("--")
                .withArg(repositoryUrl)
                .withArg(workingDir.getAbsolutePath())
                .withNonArgSecrets(secrets)
                .withEncoding("UTF-8");
        CommandLine hg = createCommandLine("hg")
                .withArgs("id", "--id", "--")
                .withArg(repositoryURL)
                .withNonArgSecrets(secrets)
                .withEncoding("UTF-8");
        execute(hg, new NamedProcessTag(repositoryURL.forDisplay()));
    private boolean pull(ConsoleOutputStreamConsumer outputStreamConsumer) {
        CommandLine hg = hg("pull", branchArg(), "--config", String.format("paths.default=%s", url));
        return execute(hg, outputStreamConsumer) == 0;
    }

    private List<Modification> findRecentModifications(int count) {
        CommandLine hg = hg("log", "--limit", String.valueOf(count), branchArg(), "--style", templatePath());
        CommandLine hg = hg("log", "-r", "tip:" + revision.getRevision(), branchArg(), "--style", templatePath());
    private String templatePath() {
        if (templatePath == null) {
            String file = HgCommand.class.getResource("/hg.template").getFile();
            templatePath = URLDecoder.decode(new File(file).getAbsolutePath(), StandardCharsets.UTF_8);
        }
        return templatePath;
    }

    private String branchArg() {
        return "--branch=" + branch;
    }