diff --git a/.changes/nextrelease/enhance-path-validation.json b/.changes/nextrelease/enhance-path-validation.json new file mode 100644 index 0000000000..1078e1f031 --- /dev/null +++ b/.changes/nextrelease/enhance-path-validation.json @@ -0,0 +1,7 @@ +[ + { + "type": "enhancement", + "category": "S3/S3Transfer", + "description": "Enhance path validation in FileDownloadHandler and ResumableDownload implementation to avoid usage of `..` within the destination path." + } +] \ No newline at end of file diff --git a/src/S3/S3Transfer/Models/ResumableDownload.php b/src/S3/S3Transfer/Models/ResumableDownload.php index 90138747d3..a54487caff 100644 --- a/src/S3/S3Transfer/Models/ResumableDownload.php +++ b/src/S3/S3Transfer/Models/ResumableDownload.php @@ -157,6 +157,11 @@ public static function fromJson(string $json): self } } + self::validatePathSafety($data['destination'], 'destination'); + if ($data['temporaryFile'] !== null) { + self::validatePathSafety($data['temporaryFile'], 'temporaryFile'); + } + return new self( $data['resumeFilePath'], $data['requestArgs'], @@ -297,4 +302,22 @@ public function markPartCompleted(int $partNumber): void { $this->partsCompleted[$partNumber] = true; } + + /** + * Validate that a path does not contain directory traversal segments. + * + * @param string $path The path to validate + * @param string $fieldName The field name for error messaging + * + * @throws S3TransferException If path traversal is detected + */ + private static function validatePathSafety(string $path, string $fieldName): void + { + $segments = preg_split('/[\\/\\\\]/', $path); + if (in_array('..', $segments, true)) { + throw new S3TransferException( + "Invalid resume file: path traversal detected in '$fieldName'" + ); + } + } } diff --git a/src/S3/S3Transfer/Utils/FileDownloadHandler.php b/src/S3/S3Transfer/Utils/FileDownloadHandler.php index 13cd4881eb..3478e7cec1 100644 --- a/src/S3/S3Transfer/Utils/FileDownloadHandler.php +++ b/src/S3/S3Transfer/Utils/FileDownloadHandler.php @@ -16,7 +16,7 @@ final class FileDownloadHandler extends AbstractDownloadHandler private const MAX_UNIQUE_ID_ATTEMPTS = 100; /** @var string */ - private string $destination; + private readonly string $destination; /** @var bool */ private bool $failsWhenDestinationExists; @@ -57,6 +57,18 @@ public function __construct( $this->fixedPartSize = $fixedPartSize; $this->handle = null; $this->transferFailed = false; + + self::validatePathSafety($this->destination, 'destination'); + if ($this->temporaryFilePath !== null) { + // Validate temporary path has no directory traversal segments + self::validatePathSafety($this->temporaryFilePath, 'temporaryFilePath'); + + // Validate temporary path and destination are both on the same directory + self::validateTemporaryFileIsOnSameDestinationDirectory( + $this->destination, + $this->temporaryFilePath + ); + } } /** @@ -451,4 +463,47 @@ public function getFixedPartSize(): int { return $this->fixedPartSize; } + + /** + * Validates that a path does not contain directory traversal segments. + * + * @param string $path The path to validate + * @param string $fieldName The field name for error messaging + * + * @throws FileDownloadException If path traversal is detected + */ + private static function validatePathSafety( + string $path, + string $fieldName + ): void + { + $segments = preg_split('/[\\/\\\\]/', $path); + if (in_array('..', $segments, true)) { + throw new FileDownloadException( + "path traversal detected in '$fieldName'" + ); + } + } + + /** + * Validates that destination and temporary path are on the same directory. + * + * @param string $destinationPath + * @param string $temporaryFilePath + * + * @return void + */ + private static function validateTemporaryFileIsOnSameDestinationDirectory( + string $destinationPath, + string $temporaryFilePath + ): void + { + $destinationDir = dirname($destinationPath); + $temporaryFileDir = dirname($temporaryFilePath); + if ($destinationDir !== $temporaryFileDir) { + throw new FileDownloadException( + "The destination and temporary paths must be on same directory." + ); + } + } } diff --git a/tests/S3/S3Transfer/Models/ResumableDownloadTest.php b/tests/S3/S3Transfer/Models/ResumableDownloadTest.php index 7f13a54950..2ec64c9cc8 100644 --- a/tests/S3/S3Transfer/Models/ResumableDownloadTest.php +++ b/tests/S3/S3Transfer/Models/ResumableDownloadTest.php @@ -337,6 +337,140 @@ public function testIsResumeFileReturnsFalseForNonExistentFile(): void $this->assertFalse(ResumableDownload::isResumeFile('/non/existent.resume')); } + /** + * Preservation: fromJson() with legitimate paths (no '..' segments) must + * continue to return valid ResumableDownload objects. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('legitimateDestinationPathsProvider')] + public function testFromJsonPreservesLegitimateDestinationPaths( + string $destination, + ): void { + $json = $this->buildResumeJson([ + 'destination' => $destination, + ]); + + $result = ResumableDownload::fromJson($json); + + $this->assertEquals($destination, $result->getDestination()); + } + + public static function legitimateDestinationPathsProvider(): array + { + return [ + 'absolute path' => [ + '/home/user/downloads/file.dat', + 'Legitimate absolute path', + ], + 'relative path' => [ + 'downloads/file.dat', + 'Relative path without traversal', + ], + 'dots in filename' => [ + '/tmp/file.v2.0.tar.gz', + 'Dots in filename must NOT be falsely rejected', + ], + 'single-dot segment in path' => [ + '/tmp/./file.dat', + 'Single-dot segment must NOT be falsely rejected', + ], + ]; + } + + /** + * Preservation: fromJson() with null temporaryFile must continue to + * return a valid ResumableDownload object. + */ + public function testFromJsonPreservesNullTemporaryFile(): void + { + $json = json_encode([ + 'version' => '1.0', + 'resumeFilePath' => $this->tempDir . 'download.resume', + 'requestArgs' => ['Bucket' => 'my-bucket', 'Key' => 'my-key'], + 'config' => ['target_part_size_bytes' => 8388608], + 'currentSnapshot' => ['transferred_bytes' => 0, 'total_bytes' => 5000], + 'initialRequestResult' => ['ContentLength' => 5000, 'ETag' => '"abc123"'], + 'partsCompleted' => [], + 'totalNumberOfParts' => 3, + 'temporaryFile' => null, + 'eTag' => '"abc123"', + 'objectSizeInBytes' => 5000, + 'fixedPartSize' => 8388608, + 'destination' => '/tmp/destination.dat', + ]); + + $result = ResumableDownload::fromJson($json); + + $this->assertNull($result->getTemporaryFile()); + } + + /** + * fromJson() with destination containing '..' segments + * should throw S3TransferException with "path traversal" message. + */ + public function testFromJsonThrowsOnDestinationPathTraversal(): void + { + $json = $this->buildResumeJson([ + 'destination' => '/downloads/../../etc/malicious', + ]); + + $this->expectException(S3TransferException::class); + $this->expectExceptionMessage('path traversal'); + ResumableDownload::fromJson($json); + } + + /** + * fromJson() with temporaryFile containing '..' segments + * should throw S3TransferException with "path traversal" message. + */ + public function testFromJsonThrowsOnTemporaryFilePathTraversal(): void + { + $json = $this->buildResumeJson([ + 'temporaryFile' => '/tmp/../../var/www/shell.php', + ]); + + $this->expectException(S3TransferException::class); + $this->expectExceptionMessage('path traversal'); + ResumableDownload::fromJson($json); + } + + /** + * fromJson() with both destination and temporaryFile + * containing '..' segments should throw S3TransferException. + */ + public function testFromJsonThrowsOnBothFieldsPathTraversal(): void + { + $json = $this->buildResumeJson([ + 'destination' => '../../../var/www/shell.php', + 'temporaryFile' => '../../etc/passwd', + ]); + + $this->expectException(S3TransferException::class); + $this->expectExceptionMessage('path traversal'); + ResumableDownload::fromJson($json); + } + + /** + * Build a valid resume JSON string with optional field overrides. + */ + private function buildResumeJson(array $overrides = []): string + { + return json_encode([ + 'version' => '1.0', + 'resumeFilePath' => $overrides['resumeFilePath'] ?? $this->tempDir . 'download.resume', + 'requestArgs' => $overrides['requestArgs'] ?? ['Bucket' => 'my-bucket', 'Key' => 'my-key'], + 'config' => $overrides['config'] ?? ['target_part_size_bytes' => 8388608], + 'currentSnapshot' => $overrides['currentSnapshot'] ?? ['transferred_bytes' => 0, 'total_bytes' => 5000], + 'initialRequestResult' => $overrides['initialRequestResult'] ?? ['ContentLength' => 5000, 'ETag' => '"abc123"'], + 'partsCompleted' => $overrides['partsCompleted'] ?? [], + 'totalNumberOfParts' => $overrides['totalNumberOfParts'] ?? 3, + 'temporaryFile' => $overrides['temporaryFile'] ?? '/tmp/download.tmp', + 'eTag' => $overrides['eTag'] ?? '"abc123"', + 'objectSizeInBytes' => $overrides['objectSizeInBytes'] ?? 5000, + 'fixedPartSize' => $overrides['fixedPartSize'] ?? 8388608, + 'destination' => $overrides['destination'] ?? '/tmp/destination.dat', + ]); + } + private function createResumableDownload(array $overrides = []): ResumableDownload { return new ResumableDownload( diff --git a/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php b/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php index fb93b89c12..a08801a684 100644 --- a/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php +++ b/tests/S3/S3Transfer/Utils/FileDownloadHandlerTest.php @@ -8,9 +8,11 @@ use Aws\S3\S3Transfer\Utils\FileDownloadHandler; use Aws\Test\TestsUtility; use GuzzleHttp\Psr7\Utils; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +#[CoversClass(FileDownloadHandler::class)] final class FileDownloadHandlerTest extends TestCase { private string $tempDir; @@ -238,6 +240,82 @@ public function testFailsOnChecksumMismatch(): void $handler->bytesTransferred([AbstractTransferListener::PROGRESS_SNAPSHOT_KEY => $snapshot]); } + /** + * Preservation: FileDownloadHandler constructed with legitimate paths + * (no '..' segments) must succeed without throwing. + */ + #[DataProvider('legitimatePathsProvider')] + public function testConstructorAcceptsLegitimatePaths( + string $destination, + ?string $temporaryFilePath, + string $description + ): void { + $handler = new FileDownloadHandler( + $destination, + false, + false, + $temporaryFilePath + ); + + $this->assertEquals($destination, $handler->getDestination()); + } + + public static function legitimatePathsProvider(): array + { + return [ + 'absolute path' => [ + '/home/user/downloads/file.dat', + null, + 'Legitimate absolute path succeeds', + ], + 'relative path' => [ + 'downloads/file.dat', + null, + 'Relative path succeeds', + ], + 'dots in filename' => [ + '/tmp/file.v2.0.tar.gz', + null, + 'Dots in filename must NOT be falsely rejected', + ], + 'null temporaryFilePath' => [ + '/tmp/destination.dat', + null, + 'Null temporaryFilePath succeeds', + ], + ]; + } + + /** + * FileDownloadHandler with destination containing '..' + * segments should throw FileDownloadException with "path traversal" message. + */ + public function testConstructorThrowsOnDestinationPathTraversal(): void + { + $this->expectException(FileDownloadException::class); + $this->expectExceptionMessage('path traversal'); + new FileDownloadHandler( + '/tmp/../../etc/cron.d/job', + false + ); + } + + /** + * FileDownloadHandler with temporaryFilePath containing + * '..' segments should throw FileDownloadException with "path traversal" message. + */ + public function testConstructorThrowsOnTemporaryFilePathTraversal(): void + { + $this->expectException(FileDownloadException::class); + $this->expectExceptionMessage('path traversal'); + new FileDownloadHandler( + $this->tempDir . 'safe-destination.txt', + false, + true, + '../../var/malicious' + ); + } + public function testCleansUpResourcesAfterFailure(): void { $destination = $this->tempDir . 'file.txt'; @@ -260,4 +338,16 @@ public function testCleansUpResourcesAfterFailure(): void $this->assertFileDoesNotExist($tempFile); } + + public function testConstructorThrowsWhenDestinationAndTemporaryDirsAreNotTheSame(): void + { + $this->expectException(FileDownloadException::class); + $this->expectExceptionMessage('The destination and temporary paths must be on same directory.'); + new FileDownloadHandler( + '/opt/temp/myfile.txt', + false, + false, + '/opt/temp/anotherdir/myfile.s3tmp.12345678' + ); + } }