CVE-2021-43286 (Java, published 2022-04-14T13:15Z)
Description: value: An issue was discovered in ThoughtWorks GoCD before 21.3.0. An attacker with privileges to create a new pipeline on a GoCD server can abuse a command-line injection in the Git URL "Test Connection" feature to execute arbitrary code.
Severity: MEDIUM (CVSS v3 base score: 8.8)
Weakness: CWE-77 \u2014 Improper Neutralization of Special Elements used in a Command ('Command Injection')
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.
Repository: https://github.com/gocd/gocd
Files changed:
config/config-api/src/main/java/com/thoughtworks/go/config/materials/ScmMaterialConfig.java
config/config-api/src/main/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfig.java
config/config-api/src/main/java/com/thoughtworks/go/util/command/UrlArgument.java
config/config-api/src/test/java/com/thoughtworks/go/config/materials/ScmMaterialConfigTest.java
config/config-api/src/test/java/com/thoughtworks/go/config/materials/git/GitMaterialConfigTest.java
config/config-api/src/test/java/com/thoughtworks/go/config/materials/mercurial/HgMaterialConfigTest.java
config/config-api/src/test/java/com/thoughtworks/go/config/materials/perforce/P4MaterialConfigTest.java
config/config-api/src/test/java/com/thoughtworks/go/config/materials/svn/SvnMaterialConfigTest.java
config/config-api/src/test/java/com/thoughtworks/go/config/materials/tfs/TfsMaterialConfigTest.java
config/config-api/src/test/java/com/thoughtworks/go/helper/MaterialConfigsMother.java
config/config-api/src/test/java/com/thoughtworks/go/util/command/UrlArgumentTest.java
domain/src/main/java/com/thoughtworks/go/domain/materials/git/GitCommand.java
Commit message: #000 - Validate URLs provided

- Disallow URLs which are obviously not URLs.


Vulnerable code:

        validateEncryptedPassword();
import static com.thoughtworks.go.config.materials.ScmMaterialConfig.AUTO_UPDATE;
import static com.thoughtworks.go.config.materials.ScmMaterialConfig.FOLDER;
import com.thoughtworks.go.config.materials.svn.SvnMaterialConfig;
        map.put(SvnMaterialConfig.PASSWORD, "Hehehe");
        map.put(SvnMaterialConfig.PASSWORD_CHANGED, "0");
        map.put(SvnMaterialConfig.PASSWORD, "");
        map.put(SvnMaterialConfig.PASSWORD_CHANGED, "1");
        final CommandLine commandLine = git().withArgs("ls-remote").withArg(repoUrl).withArg(ref);
        String[] args = new String[]{"add", fileToAdd.getName()};
        String[] addSubmoduleWithSameNameArgs = new String[]{"submodule", "add", repoUrl, folder};

Fixed code:

        if (System.getProperty("gocd.verify.url.correctness", "y").equalsIgnoreCase("y") && !url.isValidURLOrLocalPath()) {
            errors().add(URL, "URL does not seem to be valid.");
        }

        if (!new UrlArgument(getUrl()).isValidURLOrLocalPath()) {
            errors.add(SERVER_AND_PORT, "P4 port seems to be invalid.");
        }

        view.validate(validationContext);
import java.util.regex.Pattern;
    private static final String URL_DUMB_VALIDATION_REGEX = "^[a-zA-Z0-9/].*";
    private static final Pattern pattern = Pattern.compile(URL_DUMB_VALIDATION_REGEX);


    public boolean isValidURLOrLocalPath() {
        return pattern.matcher(url).matches();
    }
import static com.thoughtworks.go.config.materials.ScmMaterialConfig.*;
        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(git("-url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
            assertTrue(validating(git("_url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
            assertTrue(validating(git("@url-not-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));

            assertFalse(validating(git("url-starting-with-an-alphanumeric-character")).errors().containsKey(GitMaterialConfig.URL));
        }

import static com.thoughtworks.go.helper.MaterialConfigsMother.git;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(hg("-url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
            assertTrue(validating(hg("_url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
            assertTrue(validating(hg("@url-not-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));

            assertFalse(validating(hg("url-starting-with-an-alphanumeric-character", "folder")).errors().containsKey(HgMaterialConfig.URL));
        }

        private HgMaterialConfig validating(HgMaterialConfig hg) {
            hg.validate(new ConfigSaveValidationContext(null));
            return hg;
        }
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
        map.put(P4MaterialConfig.PASSWORD, "Hehehe");
        map.put(P4MaterialConfig.PASSWORD_CHANGED, "0");
        map.put(P4MaterialConfig.PASSWORD, "");
        map.put(P4MaterialConfig.PASSWORD_CHANGED, "1");
    @Nested
    class Validate {
        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(p4("-url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
            assertTrue(validating(p4("_url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
            assertTrue(validating(p4("@url-not-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));

            assertFalse(validating(p4("url-starting-with-an-alphanumeric-character", "view")).errors().containsKey(P4MaterialConfig.SERVER_AND_PORT));
        }

        private P4MaterialConfig validating(P4MaterialConfig p4) {
            p4.validate(new ConfigSaveValidationContext(null));
            return p4;
        }

    }
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(svn("-url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
            assertTrue(validating(svn("_url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
            assertTrue(validating(svn("@url-not-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));

            assertFalse(validating(svn("url-starting-with-an-alphanumeric-character", false)).errors().containsKey(SvnMaterialConfig.URL));
        }

        private SvnMaterialConfig validating(SvnMaterialConfig svn) {
            svn.validate(new ConfigSaveValidationContext(null));
            return svn;
        }
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

        @Test
        void rejectsObviouslyWrongURL() {
            assertTrue(validating(tfs("-url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
            assertTrue(validating(tfs("_url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
            assertTrue(validating(tfs("@url-not-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));

            assertFalse(validating(tfs("url-starting-with-an-alphanumeric-character")).errors().containsKey(TfsMaterialConfig.URL));
        }

        private TfsMaterialConfig validating(TfsMaterialConfig tfs) {
            tfs.validate(new ConfigSaveValidationContext(null));
            return tfs;
        }
    public static TfsMaterialConfig tfs(String url) {
        return tfs(new GoCipher(), new UrlArgument(url), null, null, "password");
    }


    @Nested
    @TestInstance(PER_CLASS)
    class isValidURL {
        Stream<Arguments> urls() {
            return Stream.of(
                    Arguments.of("http://my-site.com/abc", true),
                    Arguments.of("svn+ssh://my-site.com/def", true),
                    Arguments.of("file://my-site.com/def", true),
                    Arguments.of("/path/in/file/system", true),
                    Arguments.of("git@github.com:org/repo.git", true),
                    Arguments.of("user@my-site.com:org/repo.git", true),
                    Arguments.of("1a2b3c://abc", true),
                    Arguments.of("-xyz", false),
                    Arguments.of("_xyz", false),
                    Arguments.of("@xyz", false)
            );
        }

        @ParameterizedTest
        @MethodSource("urls")
        void shouldValidateURLs(String input, boolean expectedValidity) {
            final UrlArgument url = new UrlArgument(input);

            assertThat(url.isValidURLOrLocalPath()).as("Verify validity of '%s' as a URL", url.originalArgument()).isEqualTo(expectedValidity);
        }
    }
        final CommandLine commandLine = git().withArgs("ls-remote", "--").withArg(repoUrl).withArg(ref);
        String[] args = new String[]{"add", "--", fileToAdd.getName()};
        String[] addSubmoduleWithSameNameArgs = new String[]{"submodule", "add", "--", repoUrl, folder};