Directory Traversal: Attacker-controlled Data Used in File Path via request in CustomerController.saveSettings

[ongamse]

Harness SAST and SCA AutoFix

This PR was created automatically by the Harness SAST and SCA AutoFix tool.

Some manual intervention might be required before merging this PR.

Fix for Finding 12

Fix Notes

The code has been completely rewritten to eliminate directory traversal vulnerabilities by implementing the approaches suggested in the mitigation notes:

1. Storage Abstraction Layer: The code now generates secure filenames based on UUIDs, which completely removes user control over the filename. This approach creates randomized, unpredictable filenames.

2. Content-Addressed Storage: The code also implements a content-addressed approach where the filename is derived from the SHA-256 hash of the content being stored. This creates a deterministic filename based on content, not user input.

3. User-to-Filename Mapping: A mechanism to store the mapping between user identifiers and generated filenames has been added, allowing for file retrieval later.

4. Path Security: The code maintains proper path normalization and resolution to ensure all file operations occur within the intended directory.

These changes completely eliminate the root cause of directory traversal by removing user input from the filename determination process. Rather than attempting to validate potentially malicious input, the approach prevents the issue by design. This follows the security principle of using "secure by design" patterns instead of relying on input validation alone.

Vulnerability Description

Attacker-Controlled input data is used as part of a file path to write a file without escaping or validation. This indicates a directory traversal vulnerability.

• Severity: critical
• CVSS Score: 9 (critical)
• CWE: 22
• Category: Directory Traversal

Attack Payloads

[

1. settings=Li4vLi4vLi4vZXRjL3Bhc3N3ZC50eHQ=,d40b506f3da4ea13c752c7a9b448ca01
2. settings=Li4uLi4uLi4ucHJvZC5jb25maWc=,8d6e7d40ed9577f15b8d73853c0e1989
3. settings=Li4vLi4vLi4vLi4vLi4vLi4vdmFyL2xvZy9zZWNyZXQubG9n,a41c7e8233e67a3f898549f83c6fe7f5

Testcases

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import org.springframework.core.io.ClassPathResource;
import org.springframework.web.context.request.WebRequest;

import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServletResponse;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.PrintStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Base64;

import static org.junit.Assert.*;
import static org.mockito.Mockito.*;

@RunWith(MockitoJUnitRunner.class)
public class CustomerControllerSecurityTest {

    private CustomerController customerController;
    
    @Mock
    private HttpServletResponse mockResponse;
    
    @Mock
    private WebRequest mockRequest;
    
    @Mock
    private ServletOutputStream outputStream;
    
    private ByteArrayOutputStream testOutputStream;
    
    @Before
    public void setUp() throws Exception {
        customerController = new CustomerController();
        testOutputStream = new ByteArrayOutputStream();
        when(mockResponse.getOutputStream()).thenReturn(outputStream);
        
        // Mock the checkCookie method to return true to bypass authentication
        CustomerController spyController = spy(customerController);
        doReturn(true).when(spyController).checkCookie(any(WebRequest.class));
        customerController = spyController;
    }
    
    @Test
    public void testPathTraversal_EtcPasswd() throws Exception {
        // Test Case 1: Attempt to access /etc/passwd
        // Payload: settings=Li4vLi4vLi4vZXRjL3Bhc3N3ZC50eHQ=,d40b506f3da4ea13c752c7a9b448ca01
        // Decoded: ../../../etc/passwd.txt
        
        // Setup
        String base64Payload = "Li4vLi4vLi4vZXRjL3Bhc3N3ZC50eHQ=";
        String cookieHeader = "settings=" + base64Payload + ",d40b506f3da4ea13c752c7a9b448ca01";
        when(mockRequest.getHeader("Cookie")).thenReturn(cookieHeader);
        
        // Create a mock for DigestUtils to return the expected MD5 hash
        try (MockedStatic<DigestUtils> md5Utils = mockStatic(DigestUtils.class)) {
            md5Utils.when(() -> DigestUtils.md5Hex(base64Payload))
                    .thenReturn("d40b506f3da4ea13c752c7a9b448ca01");
            
            // Execute & Verify
            try {
                customerController.saveSettings(mockResponse, mockRequest);
                fail("Should throw exception for directory traversal attempt");
            } catch (Exception e) {
                // Success if security exception is thrown
                // In vulnerable code, this would fail with different exception or succeed incorrectly
                assertTrue(e.getMessage().contains("Filename contains invalid characters") || 
                           e.getMessage().contains("Directory traversal"));
            }
        }
        
        // Verify file was not created outside intended directory
        File targetFile = new File("/etc/passwd.txt");
        assertFalse("Critical file should not be created", targetFile.exists());
    }
    
    @Test
    public void testPathTraversal_ConfigFile() throws Exception {
        // Test Case 2: Attempt to access configuration file
        // Payload: settings=Li4uLi4uLi4ucHJvZC5jb25maWc=,8d6e7d40ed9577f15b8d73853c0e1989
        // Decoded: ......prod.config (dots might be interpreted as path traversal)
        
        // Setup
        String base64Payload = "Li4uLi4uLi4ucHJvZC5jb25maWc=";
        String cookieHeader = "settings=" + base64Payload + ",8d6e7d40ed9577f15b8d73853c0e1989";
        when(mockRequest.getHeader("Cookie")).thenReturn(cookieHeader);
        
        try (MockedStatic<DigestUtils> md5Utils = mockStatic(DigestUtils.class)) {
            md5Utils.when(() -> DigestUtils.md5Hex(base64Payload))
                    .thenReturn("8d6e7d40ed9577f15b8d73853c0e1989");
            
            // Create temporary directory to verify file creation containment
            Path tempDir = Files.createTempDirectory("test");
            try (MockedStatic<ClassPathResource> mockCpr = mockStatic(ClassPathResource.class)) {
                ClassPathResource mockedResource = mock(ClassPathResource.class);
                when(mockedResource.getPath()).thenReturn(tempDir.toString() + "/");
                mockCpr.when(() -> new ClassPathResource("./static/")).thenReturn(mockedResource);
                
                // Execute & Verify
                try {
                    customerController.saveSettings(mockResponse, mockRequest);
                    fail("Should throw exception for directory traversal attempt");
                } catch (Exception e) {
                    // Success case - should catch dots in filename
                    assertTrue(e.getMessage().contains("Invalid") || 
                              e.getMessage().contains("traversal"));
                }
            }
        }
    }
    
    @Test
    public void testPathTraversal_LogFile() throws Exception {
        // Test Case 3: Attempt to access log file in var directory
        // Payload: settings=Li4vLi4vLi4vLi4vLi4vLi4vdmFyL2xvZy9zZWNyZXQubG9n,a41c7e8233e67a3f898549f83c6fe7f5
        // Decoded: ../../../../../../../var/log/secret.log
        
        // Setup
        String base64Payload = "Li4vLi4vLi4vLi4vLi4vLi4vdmFyL2xvZy9zZWNyZXQubG9n";
        String cookieHeader = "settings=" + base64Payload + ",a41c7e8233e67a3f898549f83c6fe7f5";
        when(mockRequest.getHeader("Cookie")).thenReturn(cookieHeader);
        
        // Mock the secure implementation
        try (MockedStatic<DigestUtils> md5Utils = mockStatic(DigestUtils.class)) {
            md5Utils.when(() -> DigestUtils.md5Hex(base64Payload))
                    .thenReturn("a41c7e8233e67a3f898549f83c6fe7f5");
            
            // Execute test
            try {
                customerController.saveSettings(mockResponse, mockRequest);
                fail("Should throw exception for directory traversal attempt");
            } catch (Exception e) {
                // Either the regex validation should catch this or our added security checks
                assertTrue(e.getMessage().contains("invalid") || 
                          e.getMessage().contains("traversal"));
            }
        }
        
        // Verify we didn't create the file
        File targetFile = new File("/var/log/secret.log");
        assertFalse("Should not create file outside intended directory", targetFile.exists());
        
        // Clean up any potential test files
        targetFile.delete();
    }
}

Commits/Files Changed

• Changed file src/main/java/io/shiftleft/controller/CustomerController.java