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-22 12:37:59 +0100
Commit message: #000 - Validate stage counter during upload artifacts

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 c23d6d109d..943b3471a7 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
@@ -31,10 +31,11 @@ import com.thoughtworks.go.server.view.artifacts.LocalArtifactsView;
 import com.thoughtworks.go.server.web.ArtifactFolderViewFactory;
 import com.thoughtworks.go.server.web.FileModelAndView;
 import com.thoughtworks.go.server.web.ResponseCodeView;
 import com.thoughtworks.go.util.ArtifactLogUtil;
 import com.thoughtworks.go.util.SystemEnvironment;
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Controller;
 import org.springframework.web.bind.annotation.RequestMapping;
@@ -147,10 +148,13 @@ public class ArtifactsController {
                                      MultipartHttpServletRequest request) throws Exception {
         JobIdentifier jobIdentifier;
         if (!headerConstraint.isSatisfied(request)) {
             return ResponseCodeView.create(HttpServletResponse.SC_BAD_REQUEST, "Missing required header 'Confirm'");
         }
+        if (!isValidStageCounter(stageCounter)) {
+            return buildNotFound(pipelineName, pipelineCounter, stageName, stageCounter, buildName);
+        }
         try {
             jobIdentifier = restfulService.findJob(pipelineName, pipelineCounter, stageName, stageCounter,
                     buildName, buildId);
         } catch (Exception e) {
             return buildNotFound(pipelineName, pipelineCounter, stageName, stageCounter,
@@ -222,10 +226,14 @@ public class ArtifactsController {
     ) throws Exception {
         if (filePath.contains("..")) {
             return FileModelAndView.forbiddenUrl(filePath);
         }
 
+        if (!isValidStageCounter(stageCounter)) {
+            return buildNotFound(pipelineName, pipelineCounter, stageName, stageCounter, buildName);
+        }
+
         JobIdentifier jobIdentifier;
         try {
             jobIdentifier = restfulService.findJob(pipelineName, pipelineCounter, stageName, stageCounter, buildName, buildId);
         } catch (Exception e) {
             return buildNotFound(pipelineName, pipelineCounter, stageName, stageCounter, buildName);
@@ -339,6 +347,19 @@ public class ArtifactsController {
 
     private ModelAndView logsNotFound(JobIdentifier identifier) {
         String notFound = String.format("Console log for %s is unavailable as it may have been purged by Go or deleted externally.", identifier.toFullString());
         return ResponseCodeView.create(SC_NOT_FOUND, notFound);
     }
+
+    private boolean isValidStageCounter(String stageCounter) {
+        if (StringUtils.isEmpty(stageCounter)) {
+            return true;
+        }
+
+        try {
+            int value = Integer.parseInt(stageCounter);
+            return value > 0;
+        } catch (Exception e) {
+            return false;
+        }
+    }
 }
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 768f452e6a..7c64450bbb 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
@@ -37,10 +37,11 @@ import javax.servlet.http.HttpServletResponse;
 import java.io.File;
 import java.io.InputStream;
 
 import static com.thoughtworks.go.util.GoConstants.*;
 import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
+import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
 import static org.hamcrest.Matchers.*;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.*;
 
@@ -118,10 +119,25 @@ public class ArtifactsControllerTest {
         ResponseCodeView view = (ResponseCodeView) modelAndView.getView();
         assertThat(view.getStatusCode(), is(SC_INTERNAL_SERVER_ERROR));
         assertThat(view.getContent(), is("Error saving checksum file for the artifact at path 'some-path'"));
     }
 
+    @Test
+    void shouldFailToPostAndPutWhenStageCounterIsNotAPositiveInteger() throws Exception {
+        ModelAndView modelAndView = artifactsController.postArtifact("pipeline-1", "1", "stage-1", "NOT_AN_INTEGER", "job-1", 122L, "some-path", 1, null);
+        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));
+
+        modelAndView = artifactsController.postArtifact("pipeline-1", "1", "stage-1", "-123", "job-1", 122L, "some-path", 1, null);
+        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));
+
+        modelAndView = artifactsController.putArtifact("pipeline-1", "1", "stage-1", "NOT_AN_INTEGER", "job-1", 122L, "some-path", "1", null);
+        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));
+
+        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
     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: import org.apache.commons.lang3.StringUtils;
        if (!isValidStageCounter(stageCounter)) {
            return buildNotFound(pipelineName, pipelineCounter, stageName, stageCounter, buildName);
        }
        if (!isValidStageCounter(stageCounter)) {
            return buildNotFound(pipelineName, pipelineCounter, stageName, stageCounter, buildName);
        }


    private boolean isValidStageCounter(String stageCounter) {
        if (StringUtils.isEmpty(stageCounter)) {
            return true;
        }

        try {
            int value = Integer.parseInt(stageCounter);
            return value > 0;
        } catch (Exception e) {
            return false;
        }
    }
import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
    @Test
    void shouldFailToPostAndPutWhenStageCounterIsNotAPositiveInteger() throws Exception {
        ModelAndView modelAndView = artifactsController.postArtifact("pipeline-1", "1", "stage-1", "NOT_AN_INTEGER", "job-1", 122L, "some-path", 1, null);
        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));

        modelAndView = artifactsController.postArtifact("pipeline-1", "1", "stage-1", "-123", "job-1", 122L, "some-path", 1, null);
        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));

        modelAndView = artifactsController.putArtifact("pipeline-1", "1", "stage-1", "NOT_AN_INTEGER", "job-1", 122L, "some-path", "1", null);
        assertThat(((ResponseCodeView) modelAndView.getView()).getStatusCode(), is(SC_NOT_FOUND));

        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));
    }