Merge pull request #2012 from aws/zoewang/tmFix

Fixed possible security issue in s  operation where files could be do…

Author: zoewangg

.changes/next-release/bugfix-S3TransferManagerDeveloperPreview-9174314.json

@@ -0,0 +1,6 @@
+{
+    "category": "S3 Transfer Manager (Developer Preview)", 
+    "contributor": "", 
+    "type": "bugfix", 
+    "description": "Fixed possible security issue in `S3TransferManager`s `downloadDirectory` operation where files could be downloaded to a sibling directory of the destination directory if the key contained relative paths."
+}

services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java

@@ -121,7 +121,7 @@ private void doDownloadDirectory(CompletableFuture<CompletedDirectoryDownload> r
 
         allOfFutures.whenComplete((r, t) -> {
             if (t != null) {
-                returnFuture.completeExceptionally(SdkClientException.create("Failed to call ListObjectsV2", t));
+                returnFuture.completeExceptionally(SdkClientException.create("Failed to send request", t));
             } else {
                 returnFuture.complete(CompletedDirectoryDownload.builder()
                                                                 .failedTransfers(failedFileDownloads)
@@ -150,9 +150,18 @@ private DownloadFileContext determineDestinationPath(DownloadDirectoryRequest do
         String key = normalizeKey(downloadDirectoryRequest, s3Object, delimiter);
         String relativePath = getRelativePath(fileSystem, delimiter, key);
         Path destinationPath = downloadDirectoryRequest.destinationDirectory().resolve(relativePath);
+
+        validatePath(downloadDirectoryRequest.destinationDirectory(), destinationPath, s3Object.key());
         return new DefaultDownloadFileContext(s3Object, destinationPath);
     }
 
+    private void validatePath(Path destinationDirectory, Path targetPath, String key) {
+        if (!targetPath.toAbsolutePath().normalize().startsWith(destinationDirectory.toAbsolutePath().normalize())) {
+            throw SdkClientException.create("Cannot download key " + key +
+                                            ", its relative path resolves outside the parent directory.");
+        }
+    }
+
     private CompletableFuture<CompletedFileDownload> doDownloadSingleFile(DownloadDirectoryRequest downloadDirectoryRequest,
                                                                           Collection<FailedFileDownload> failedFileDownloads,
                                                                           DownloadFileContext downloadContext) {

services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperTest.java

@@ -27,6 +27,7 @@
 import com.google.common.jimfs.Jimfs;
 import java.io.IOException;
 import java.nio.file.FileSystem;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.UUID;
@@ -38,6 +39,8 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.ArgumentCaptor;
 import software.amazon.awssdk.core.exception.SdkClientException;
 import software.amazon.awssdk.services.s3.model.EncodingType;
@@ -114,6 +117,31 @@ void downloadDirectory_allDownloadsSucceed_failedDownloadsShouldBeEmpty() throws
             "key2"));
     }
 
+    @ParameterizedTest
+    @ValueSource(strings = {"/blah",
+                            "../blah/object.dat",
+                            "blah/../../object.dat",
+                            "blah/../object/../../blah/another/object.dat",
+                            "../{directory-name}-2/object.dat"})
+    void invalidKey_shouldThrowException(String testingString) throws Exception {
+        assertExceptionThrownForInvalidKeys(testingString);
+    }
+
+    private void assertExceptionThrownForInvalidKeys(String key) throws IOException {
+        Path destinationDirectory = Files.createTempDirectory("test");
+        String lastElement = destinationDirectory.getName(destinationDirectory.getNameCount() - 1).toString();
+        key = key.replace("{directory-name}", lastElement);
+        stubSuccessfulListObjects(listObjectsHelper, key);
+        DirectoryDownload downloadDirectory =
+            downloadDirectoryHelper.downloadDirectory(DownloadDirectoryRequest.builder()
+                                                                              .destinationDirectory(destinationDirectory)
+                                                                              .bucket("bucket")
+                                                                              .build());
+
+        assertThatThrownBy(() -> downloadDirectory.completionFuture().get(5, TimeUnit.SECONDS))
+            .hasCauseInstanceOf(SdkClientException.class).getRootCause().hasMessageContaining("Cannot download key");
+    }
+
     @Test
     void downloadDirectory_cancel_shouldCancelAllFutures() throws Exception {
         stubSuccessfulListObjects(listObjectsHelper, "key1", "key2");