CVE-2021-43289: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: An issue was discovered in ThoughtWorks GoCD before 21.3.0. An attacker who has compromised a GoCD agent can upload a malicious file into an arbitrary directory of a GoCD server, but does not control the filename.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 7.5
Published: 2022-04-14T13:15Z
Repository: https://github.com/gocd/gocd
Language: Java
Files changed: server/src/main/java/com/thoughtworks/go/server/controller/ArtifactsController.java, server/src/test-fast/java/com/thoughtworks/go/server/controller/ArtifactsControllerTest.java
Commit date: 2021-10-25 10:36:33 +0100
Commit message: #000 - Validate stage counter in GETs as well.

Continuation of commit c22e0428164af

Diff: diff --git a/server/src/main/java/com/thoughtworks/go/server/controller/ArtifactsController.java b/server/src/main/java/com/thoughtworks/go/server/controller/ArtifactsController.java
index 7330f10d2c..45da920b50 100644
--- a/server/src/main/java/com/thoughtworks/go/server/controller/ArtifactsController.java
+++ b/server/src/main/java/com/thoughtworks/go/server/controller/ArtifactsController.java
@@ -257,10 +257,14 @@ public class ArtifactsController {
                                    @RequestParam(value = "stageCounter", required = false) String stageCounter,
                                    @RequestParam(value = "startLineNumber", required = false) Long start
     ) {
         start = start == null ? 0L : start;
 
+        if (!isValidStageCounter(stageCounter)) {
+            return buildNotFound(pipelineName, pipelineCounter, stageName, stageCounter, buildName);
+        }
+
         try {
             JobIdentifier identifier = restfulService.findJob(pipelineName, pipelineCounter, stageName, stageCounter, buildName);
             if (jobInstanceDao.isJobCompleted(identifier) && !consoleService.doesLogExist(identifier)) {
                 return logsNotFound(identifier);
             }
@@ -279,10 +283,15 @@ public class ArtifactsController {
         return new ModelAndView("exceptions_page", model);
     }
 
     ModelAndView getArtifact(String filePath, ArtifactFolderViewFactory folderViewFactory, String pipelineName, String counterOrLabel, String stageName, String stageCounter, String buildName, String sha, String serverAlias) throws Exception {
         LOGGER.info("[Artifact Download] Trying to resolve '{}' for '{}/{}/{}/{}/{}'", filePath, pipelineName, counterOrLabel, stageName, stageCounter, buildName);
+
+        if (!isValidStageCounter(stageCounter)) {
+            return buildNotFound(pipelineName, counterOrLabel, stageName, stageCounter, buildName);
+        }
+
         long before = System.currentTimeMillis();
         ArtifactsView view;
         //Work out the job that we are trying to retrieve
         JobIdentifier translatedId;
         try {
diff --git a/server/src/test-fast/java/com/thoughtworks/go/server/controller/ArtifactsControllerTest.java b/server/src/test-fast/java/com/thoughtworks/go/server/controller/ArtifactsControllerTest.java
index 7c64450bbb..c633888096 100644
--- a/server/src/test-fast/java/com/thoughtworks/go/server/controller/ArtifactsControllerTest.java
+++ b/server/src/test-fast/java/com/thoughtworks/go/server/controller/ArtifactsControllerTest.java
@@ -134,10 +134,22 @@ public class ArtifactsControllerTest {
 
         modelAndView = artifactsController.putArtifact("pipeline-1", "1", "stage-1", "-123", "job-1", 122L, "some-path", "1", null);
         assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));
     }
 
+    @Test
+    void shouldFailToGetConsoleOutWhenStageCounterIsNotAPositiveInteger() throws Exception {
+        ModelAndView modelAndView = artifactsController.consoleout("pipeline-1", "1", "stage-1", "job-1", "NOT_AN_INTEGER", 122L);
+        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));
+    }
+
+    @Test
+    void shouldFailToGetArtifactWhenStageCounterIsNotAPositiveInteger() throws Exception {
+        ModelAndView modelAndView = artifactsController.getArtifactAsHtml("pipeline-1", "1", "stage-1",  "NOT_AN_INTEGER", "job-1", "some-path", "sha1", "alias");
+        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));
+    }
+
     @Test
     public void shouldFunnelAll_GET_calls() throws Exception {
         final ModelAndView returnVal = new ModelAndView();
         ArtifactsController controller = new ArtifactsController(artifactService, restfulService, mock(ZipArtifactCache.class), jobInstanceDao, consoleActivityMonitor, consoleService, systemEnvironment) {
             @Override

Fixed code:         if (!isValidStageCounter(stageCounter)) {
            return buildNotFound(pipelineName, pipelineCounter, stageName, stageCounter, buildName);
        }


        if (!isValidStageCounter(stageCounter)) {
            return buildNotFound(pipelineName, counterOrLabel, stageName, stageCounter, buildName);
        }

    @Test
    void shouldFailToGetConsoleOutWhenStageCounterIsNotAPositiveInteger() throws Exception {
        ModelAndView modelAndView = artifactsController.consoleout("pipeline-1", "1", "stage-1", "job-1", "NOT_AN_INTEGER", 122L);
        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));
    }

    @Test
    void shouldFailToGetArtifactWhenStageCounterIsNotAPositiveInteger() throws Exception {
        ModelAndView modelAndView = artifactsController.getArtifactAsHtml("pipeline-1", "1", "stage-1",  "NOT_AN_INTEGER", "job-1", "some-path", "sha1", "alias");
        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));
    }