From d09223d43567517fa4bfa1038e0a1ac77df19c39 Mon Sep 17 00:00:00 2001 From: jfcarroll Date: Thu, 4 Aug 2022 18:11:20 -0700 Subject: [PATCH] Prevent escaping the users Downloads folder by removing all occurances of /.. from the path passed in. PiperOrigin-RevId: 465450615 --- .../implementation/windows/platform.cc | 28 ++- .../implementation/windows/platform_test.cc | 214 ++++++++++++------ 2 files changed, 171 insertions(+), 71 deletions(-) diff --git a/internal/platform/implementation/windows/platform.cc b/internal/platform/implementation/windows/platform.cc index cd5085b9..e6ae413b 100644 --- a/internal/platform/implementation/windows/platform.cc +++ b/internal/platform/implementation/windows/platform.cc @@ -14,14 +14,14 @@ #include "internal/platform/implementation/platform.h" +#include +#include #include #include #include #include #include #include -#include -#include #include #include @@ -56,6 +56,8 @@ namespace nearby { namespace api { namespace { +constexpr absl::string_view kUpOneLevel("/.."); + std::string GetDownloadPathInternal(std::string& parent_folder, std::string& file_name) { PWSTR basePath; @@ -129,13 +131,27 @@ std::string GetDownloadPathInternal(std::string& parent_folder, return retVal; } +void SanitizePath(std::string& path) { + size_t pos = std::string::npos; + // Search for the substring in string in a loop until nothing is found + while ((pos = path.find(kUpOneLevel.data())) != std::string::npos) { + // If found then erase it from string + path.erase(pos, kUpOneLevel.size()); + } +} + // If the file already exists we add " (x)", where x is an incrementing number, // starting at 1, using the next non-existing number, to the file name, just // before the first dot, or at the end if no dot. The absolute path is returned. std::string CreateOutputFileWithRename(absl::string_view path) { - auto last_separator = path.find_last_of('/'); - std::string folder(path.substr(0, last_separator)); - std::string file_name(path.substr(last_separator)); + // Remove any /.. + std::string sanitized_path(path); + std::replace(sanitized_path.begin(), sanitized_path.end(), '\\', '/'); + SanitizePath(sanitized_path); + + auto last_separator = sanitized_path.find_last_of('/'); + std::string folder(sanitized_path.substr(0, last_separator)); + std::string file_name(sanitized_path.substr(last_separator)); int count = 0; @@ -151,7 +167,7 @@ std::string CreateOutputFileWithRename(absl::string_view path) { auto file_name2 = file_name.substr(first); // Construct the target file name - std::string target(path); + std::string target(sanitized_path); std::fstream file; file.open(target, std::fstream::binary | std::fstream::in); diff --git a/internal/platform/implementation/windows/platform_test.cc b/internal/platform/implementation/windows/platform_test.cc index 9501f439..5259f0dc 100644 --- a/internal/platform/implementation/windows/platform_test.cc +++ b/internal/platform/implementation/windows/platform_test.cc @@ -14,9 +14,9 @@ #include "internal/platform/implementation/platform.h" +#include #include #include -#include #include #include @@ -37,11 +37,18 @@ constexpr absl::string_view kOneIterationNoDotsFileName( constexpr absl::string_view kMultipleDotsFileName("/increment.file.test.txt"); constexpr absl::string_view kOneIterationMultipleDotsFileName( "/increment (1).file.test.txt"); +constexpr absl::string_view kImmediateEscape("../"); +constexpr absl::string_view kLongEscapeBackSlash("..\\test\\..\\..\\test"); +constexpr absl::string_view kTwoLevelFolder("/test/test"); +constexpr absl::string_view kLongEscapeSlash("../test/../../test"); +constexpr absl::string_view kLongEscapeMixedSlash("../test\\..\\../test"); +constexpr absl::string_view kLongEscapeEndingEscape("../test/../../test/.."); +constexpr absl::string_view kLongEscapeEndingEscapeWithSlash( + "../test/../../test/../../../"); } // namespace // Can't run on google 3, I presume the SHGetKnownFolderPath // fails. -#if 0 class ImplementationPlatformTests : public testing::Test { protected: // You can define per-test set-up logic as usual. @@ -78,8 +85,8 @@ class ImplementationPlatformTests : public testing::Test { std::string default_download_path_; }; -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithEmptyStringArgumentsShouldReturnBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithEmptyString\ +ArgumentsShouldReturnBaseDownloadPath) { // Arrange std::string parent_folder(""); std::string file_name(""); @@ -92,9 +99,8 @@ TEST_F(ImplementationPlatformTests, EXPECT_EQ(result, default_download_path_); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithSlashParentFolderArgumentsShouldReturn\ -BaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithSlashParent\ +FolderArgumentsShouldReturnBaseDownloadPath) { // Arrange std::string parent_folder("/"); std::string file_name(""); @@ -107,9 +113,8 @@ BaseDownloadPath) { EXPECT_EQ(result, default_download_path_); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithBackslashParentFolderArgumentsShouldReturn\ -BaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithBackslashParent\ +FolderArgumentsShouldReturnBaseDownloadPath) { // Arrange std::string parent_folder("\\"); std::string file_name(""); @@ -122,8 +127,97 @@ BaseDownloadPath) { EXPECT_EQ(result, default_download_path_); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithSlashFileNameArgumentsShouldReturnBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithAttemptToEscape\ +UsersDownloadFolderShouldReturnDownloadPathNotEscapingUsersDownloadFolder) { + // Arrange + std::string parent_folder(kImmediateEscape); + std::string file_name(""); + + // Act + auto result = location::nearby::api::ImplementationPlatform::GetDownloadPath( + parent_folder, file_name); + + // Assert + EXPECT_EQ(result, default_download_path_); +} + +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithMultiple\ +AttemptsToEscapeUsersDownloadFolderWithBackslashShouldReturnDownloadPath\ +NotEscapingUsersDownloadFolder) { + // Arrange + std::string parent_folder(kLongEscapeBackSlash); + std::string file_name(""); + + // Act + auto result = location::nearby::api::ImplementationPlatform::GetDownloadPath( + parent_folder, file_name); + + // Assert + EXPECT_EQ(result, default_download_path_ + kTwoLevelFolder.data()); +} + +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithMultiple\ +AttemptsToEscapeUsersDownloadFolderShouldReturnDownloadPathNotEscapingUsers\ +DownloadFolder) { + // Arrange + std::string parent_folder(kLongEscapeSlash); + std::string file_name(""); + + // Act + auto result = location::nearby::api::ImplementationPlatform::GetDownloadPath( + parent_folder, file_name); + + // Assert + EXPECT_EQ(result, default_download_path_ + kTwoLevelFolder.data()); +} + +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithMultiple\ +AttemptsToEscapeUsersDownloadFolderWithMixedSlashShouldReturnDownloadPath\ +NotEscapingUsersDownloadFolder) { + // Arrange + std::string parent_folder(kLongEscapeMixedSlash); + std::string file_name(""); + + // Act + auto result = location::nearby::api::ImplementationPlatform::GetDownloadPath( + parent_folder, file_name); + + // Assert + EXPECT_EQ(result, default_download_path_ + kTwoLevelFolder.data()); +} + +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithMultiple\ +AttemptsToEscapeUsersDownloadFolderWithEndingEscapeShouldReturnDownload\ +PathNotEscapingUsersDownloadFolder) { + // Arrange + std::string parent_folder(kLongEscapeEndingEscape); + std::string file_name(""); + + // Act + auto result = location::nearby::api::ImplementationPlatform::GetDownloadPath( + parent_folder, file_name); + + // Assert + EXPECT_EQ(result, default_download_path_ + kTwoLevelFolder.data()); +} + +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithMultiple\ +AttemptsToEscapeUsersDownloadFolderWithEndingSlashShouldReturnDownloadPathNot\ +EscapingUsersDownloadFolder) { + // Arrange + std::string parent_folder(kLongEscapeEndingEscapeWithSlash); + std::string file_name(""); + + // Act + auto result = location::nearby::api::ImplementationPlatform::GetDownloadPath( + parent_folder, file_name); + + // Assert + EXPECT_EQ(result, default_download_path_ + kTwoLevelFolder.data()); +} + +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithSlashFileName\ +ArgumentsShouldReturnBaseDownloadPath) { // Arrange std::string parent_folder(""); std::string file_name("/"); @@ -136,9 +230,8 @@ TEST_F(ImplementationPlatformTests, EXPECT_EQ(result, default_download_path_); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithBackslashFileNameArgumentsShouldReturn\ -BaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithBackslashFile\ +NameArgumentsShouldReturnBaseDownloadPath) { // Arrange std::string parent_folder(""); std::string file_name("\\"); @@ -154,9 +247,8 @@ BaseDownloadPath) { EXPECT_EQ(result, default_download_path_); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithParentFolderShouldReturnParentFolder\ -AppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithParentFolder\ +ShouldReturnParentFolderAppendedToBaseDownloadPath) { // Arrange std::string parent_folder("test_parent_folder"); std::string file_name(""); @@ -175,9 +267,8 @@ AppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithParentFolderStartingWithSlashArgumentsShouldReturn\ -ParentFolderAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithParentFolder\ +StartingWithSlashArgumentsShouldReturnParentFolderAppendedToBaseDownloadPath) { // Arrange std::string parent_folder("/test_parent_folder"); std::string file_name(""); @@ -196,9 +287,9 @@ ParentFolderAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithParentFolderStartingWithBackslashArguments\ -ShouldReturnParentFolderAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithParentFolder\ +StartingWithBackslashArgumentsShouldReturnParentFolderAppendedToBase\ +DownloadPath) { // Arrange std::string parent_folder("\\test_parent_folder"); std::string file_name(""); @@ -217,9 +308,8 @@ ShouldReturnParentFolderAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithParentFolderEndingWithSlashArgumentsShouldReturn\ -ParentFolderAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithParentFolder\ +EndingWithSlashArgumentsShouldReturnParentFolderAppendedToBaseDownloadPath) { // Arrange std::string parent_folder("test_parent_folder/"); std::string file_name(""); @@ -238,9 +328,9 @@ ParentFolderAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithParentFolderEndingWithBackslashArguments\ -ShouldReturnParentFolderAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithParentFolder\ +EndingWithBackslashArgumentsShouldReturnParentFolderAppendedToBaseDownloadPath) +{ // Arrange std::string parent_folder("test_parent_folder\\"); std::string file_name(""); @@ -259,9 +349,8 @@ ShouldReturnParentFolderAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithFileNameBeginningWithSlashArgumentsShouldReturn\ -FileNameAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithFileName\ +BeginningWithSlashArgumentsShouldReturnFileNameAppendedToBaseDownloadPath) { // Arrange std::string parent_folder(""); std::string file_name("/test_file_name.name"); @@ -280,9 +369,8 @@ FileNameAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithFileNameBeginningWithBackslashArgumentsShouldReturn\ -FileNameAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithFileName\ +BeginningWithBackslashArgumentsShouldReturnFileNameAppendedToBaseDownloadPath) { // Arrange std::string parent_folder(""); std::string file_name("\\test_file_name.name"); @@ -299,9 +387,8 @@ FileNameAppendedToBaseDownloadPath) { EXPECT_EQ(result, path.str().c_str()); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithFileNameEndingWithSlashArgumentsShouldReturnFileName\ -AppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithFileNameEnding\ +WithSlashArgumentsShouldReturnFileNameAppendedToBaseDownloadPath) { // Arrange std::string parent_folder(""); std::string file_name("test_file_name.name/"); @@ -320,9 +407,8 @@ AppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithFileNameEndingWithBackslashArgumentsShouldReturn\ -FileNameAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithFileNameEnding\ +WithBackslashArgumentsShouldReturnFileNameAppendedToBaseDownloadPath) { // Arrange std::string parent_folder(""); std::string file_name("test_file_name.name\\"); @@ -341,9 +427,9 @@ FileNameAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithParentFolderAndFileNameArgumentsShouldReturn\ -ParentFolderAndFileNameAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithParentFolderAnd\ +FileNameArgumentsShouldReturnParentFolderAndFileNameAppendedToBaseDownloadPath) +{ // Arrange std::string parent_folder("test_parent_folder"); std::string file_name("test_file_name.name"); @@ -364,9 +450,9 @@ ParentFolderAndFileNameAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithParentFolderEndingWithBackslashAndFileNameArguments\ -ShouldReturnParentFolderAndFileNameAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithParentFolder\ +EndingWithBackslashAndFileNameArgumentsShouldReturnParentFolderAndFileName\ +AppendedToBaseDownloadPath) { // Arrange std::string parent_folder("test_parent_folder\\"); std::string file_name("test_file_name.name"); @@ -387,9 +473,9 @@ ShouldReturnParentFolderAndFileNameAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPathWithFileNameStartingWithBackslashAndParentFolderArguments\ -ShouldReturnParentFolderAndFileNameAppendedToBaseDownloadPath) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPathWithFileName\ +StartingWithBackslashAndParentFolderArgumentsShouldReturnParentFolderAnd\ +FileNameAppendedToBaseDownloadPath) { // Arrange std::string parent_folder("test_parent_folder"); std::string file_name("\\test_file_name.name"); @@ -410,8 +496,8 @@ ShouldReturnParentFolderAndFileNameAppendedToBaseDownloadPath) { EXPECT_EQ(result, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPath_FileDoesntExistReturnsFileWithPassedName) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPath_FileDoesntExist\ +ReturnsFileWithPassedName) { // Arrange std::string file_name(kFileName); std::string parent_folder(""); @@ -428,8 +514,8 @@ TEST_F(ImplementationPlatformTests, EXPECT_EQ(actual, expected); } -TEST_F(ImplementationPlatformTests, - GetDownloadPath_FileExistsReturnsFileWithIncrementedName) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPath_FileExistsReturns\ +FileWithIncrementedName) { // Arrange std::string file_name(kFileName); std::string renamed_file_name(kFirstIterationFileName); @@ -468,8 +554,8 @@ TEST_F(ImplementationPlatformTests, ASSERT_FALSE(input_file.rdstate() == std::ifstream::goodbit); } -TEST_F(ImplementationPlatformTests, - GetDownloadPath_MultipleFilesExistReturnsNextIncrementedFileName) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPath_MultipleFilesExist\ +ReturnsNextIncrementedFileName) { // Arrange std::ofstream output_file; std::ifstream input_file; @@ -525,9 +611,8 @@ TEST_F(ImplementationPlatformTests, ASSERT_FALSE(input_file.rdstate() == std::ifstream::goodbit); } -TEST_F( - ImplementationPlatformTests, - GetDownloadPath_FileNameContainsMultipleDotsReturnsIncrementBeforeFirstDot){ +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPath_FileNameContains\ +MultipleDotsReturnsIncrementBeforeFirstDot) { // Arrange std::ifstream input_file; std::ofstream output_file; @@ -565,8 +650,8 @@ TEST_F( ASSERT_FALSE(input_file.rdstate() == std::ifstream::goodbit); } -TEST_F(ImplementationPlatformTests, - GetDownloadPath_FileNameContainsNoDotsReturnsWithIncrementAtEnd) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPath_FileNameContainsNo\ +DotsReturnsWithIncrementAtEnd) { std::ifstream input_file; std::ofstream output_file; @@ -603,8 +688,8 @@ TEST_F(ImplementationPlatformTests, ASSERT_FALSE(input_file.rdstate() == std::ifstream::goodbit); } -TEST_F(ImplementationPlatformTests, - GetDownloadPath_FileNameExistsWithAHoleBetweenRenamedFiles) { +TEST_F(ImplementationPlatformTests, DISABLED_GetDownloadPath_FileNameExistsWith\ +AHoleBetweenRenamedFiles) { std::ifstream input_file; std::ofstream output_file; @@ -684,4 +769,3 @@ TEST_F(ImplementationPlatformTests, input_file.open(output_file3_path, std::ifstream::binary | std::ifstream::in); ASSERT_FALSE(input_file.rdstate() == std::ifstream::goodbit); } -#endif