diff --git a/src/ResourceInfo/ResourceInfo.cpp b/src/ResourceInfo/ResourceInfo.cpp index 1a2394b..ed05368 100644 --- a/src/ResourceInfo/ResourceInfo.cpp +++ b/src/ResourceInfo/ResourceInfo.cpp @@ -21,6 +21,47 @@ namespace CarbonResources { +namespace +{ + +bool IsPathWithinBase( const std::filesystem::path& base, const std::filesystem::path& candidate ) +{ + std::error_code ec; + + std::filesystem::path normalizedBase = std::filesystem::weakly_canonical( base, ec ); + if( ec ) + { + normalizedBase = base.lexically_normal(); + } + + ec.clear(); + + std::filesystem::path normalizedCandidate = std::filesystem::weakly_canonical( candidate, ec ); + if( ec ) + { + normalizedCandidate = candidate.lexically_normal(); + } + + std::filesystem::path relative = normalizedCandidate.lexically_relative( normalizedBase ); + + if( relative.empty() ) + { + return false; + } + + for( const auto& part : relative ) + { + if( part == ".." ) + { + return false; + } + } + + return true; +} + +} + std::string Location::CalculateLocationFromChecksums( const std::string& relativePathChecksum, const std::string& dataChecksum ) const { std::stringstream ss; @@ -330,6 +371,11 @@ Result ResourceInfo::PutDataLocalRelative( ResourcePutDataParams& params ) const std::filesystem::path path = params.resourceDestinationSettings.basePath / m_relativePath.GetValue(); + if( !IsPathWithinBase( params.resourceDestinationSettings.basePath, path ) ) + { + return Result{ ResultType::MALFORMED_RESOURCE_INPUT }; + } + bool res = ResourceTools::SaveFile( path, data ); if( res ) @@ -349,6 +395,11 @@ Result ResourceInfo::PutDataRemoteCdn( ResourcePutDataParams& params ) const // Construct path std::filesystem::path dataPath = params.resourceDestinationSettings.basePath / m_location.GetValue().ToString(); + if( !IsPathWithinBase( params.resourceDestinationSettings.basePath, dataPath ) ) + { + return Result{ ResultType::MALFORMED_RESOURCE_INPUT }; + } + std::string compressedData; if( !ResourceTools::GZipCompressData( data, compressedData ) ) @@ -375,6 +426,11 @@ Result ResourceInfo::PutDataLocalCdn( ResourcePutDataParams& params ) const // Construct path std::filesystem::path dataPath = params.resourceDestinationSettings.basePath / m_location.GetValue().ToString(); + if( !IsPathWithinBase( params.resourceDestinationSettings.basePath, dataPath ) ) + { + return Result{ ResultType::MALFORMED_RESOURCE_INPUT }; + } + bool res = ResourceTools::SaveFile( dataPath, data ); if( res ) @@ -391,6 +447,11 @@ Result ResourceInfo::PutDataStreamLocalRelative( ResourcePutDataStreamParams& pa { std::filesystem::path path = params.resourceDestinationSettings.basePath / m_relativePath.GetValue(); + if( !IsPathWithinBase( params.resourceDestinationSettings.basePath, path ) ) + { + return Result{ ResultType::MALFORMED_RESOURCE_INPUT }; + } + bool res = params.dataStream->StartWrite( path ); if( res ) @@ -408,6 +469,11 @@ Result ResourceInfo::PutDataStreamLocalCdn( ResourcePutDataStreamParams& params // Construct path std::filesystem::path dataPath = params.resourceDestinationSettings.basePath / m_location.GetValue().ToString(); + if( !IsPathWithinBase( params.resourceDestinationSettings.basePath, dataPath ) ) + { + return Result{ ResultType::MALFORMED_RESOURCE_INPUT }; + } + bool res = params.dataStream->StartWrite( dataPath ); if( res ) diff --git a/tests/src/ResourcesLibraryTest.cpp b/tests/src/ResourcesLibraryTest.cpp index fb9e05c..46f55e8 100644 --- a/tests/src/ResourcesLibraryTest.cpp +++ b/tests/src/ResourcesLibraryTest.cpp @@ -644,6 +644,89 @@ TEST_F( ResourcesLibraryTest, CreateAndUnpackBundle ) EXPECT_TRUE( std::filesystem::exists( unpackedGroupPath ) ); } +// Zip-slip: a manifest relative path escaping the destination (here "../") must be +// rejected on unpack instead of writing the resource outside the destination base path. +TEST_F( ResourcesLibraryTest, UnpackBundleRejectsZipSlipRelativePath ) +{ + // Import a ResourceGroup whose single resource escapes the destination via "../". + CarbonResources::ResourceGroup resourceGroup; + + CarbonResources::ResourceGroupImportFromFileParams importParams; + + importParams.filename = GetTestFileAbsolutePath( "ZipSlip/ZipSlipResourceGroup.yaml" ); + + importParams.callbackSettings.statusCallback = StatusUpdate; + + EXPECT_EQ( resourceGroup.ImportFromFile( importParams ).type, CarbonResources::ResultType::SUCCESS ); + + EXPECT_TRUE( StatusIsValid() ); + + // Bundle it, reading the resource data from the local CDN by location so the + // malicious relative path only takes effect when the bundle is unpacked. + CarbonResources::BundleCreateParams bundleCreateParams; + + bundleCreateParams.resourceGroupRelativePath = "ResourceGroup.yaml"; + + bundleCreateParams.resourceGroupBundleRelativePath = "BundleResourceGroup.yaml"; + + bundleCreateParams.resourceSourceSettings.sourceType = CarbonResources::ResourceSourceType::LOCAL_CDN; + + bundleCreateParams.resourceSourceSettings.basePaths = { GetTestFileAbsolutePath( "resourcesLocal/" ) }; + + bundleCreateParams.chunkDestinationSettings.destinationType = CarbonResources::ResourceDestinationType::LOCAL_CDN; + + bundleCreateParams.chunkDestinationSettings.basePath = "ZipSlipBundleChunks"; + + bundleCreateParams.resourceBundleResourceGroupDestinationSettings.destinationType = CarbonResources::ResourceDestinationType::LOCAL_RELATIVE; + + bundleCreateParams.resourceBundleResourceGroupDestinationSettings.basePath = "ZipSlipBundleOut"; + + bundleCreateParams.chunkSize = 1000; + + bundleCreateParams.callbackSettings.statusCallback = StatusUpdate; + + EXPECT_EQ( resourceGroup.CreateBundle( bundleCreateParams ).type, CarbonResources::ResultType::SUCCESS ); + + EXPECT_TRUE( StatusIsValid() ); + + // Load the generated bundle. + CarbonResources::BundleResourceGroup bundleResourceGroup; + + CarbonResources::ResourceGroupImportFromFileParams bundleImportParams; + + bundleImportParams.filename = bundleCreateParams.resourceBundleResourceGroupDestinationSettings.basePath / bundleCreateParams.resourceGroupBundleRelativePath; + + bundleImportParams.callbackSettings.statusCallback = StatusUpdate; + + EXPECT_EQ( bundleResourceGroup.ImportFromFile( bundleImportParams ).type, CarbonResources::ResultType::SUCCESS ); + + EXPECT_TRUE( StatusIsValid() ); + + // Clear any escaped file a previous (vulnerable) run may have written. + std::filesystem::path destinationBasePath = "ZipSlipUnpackOut"; + + std::filesystem::path escapedPath = ( destinationBasePath / "../zipSlipEscaped.txt" ).lexically_normal(); + + std::filesystem::remove( escapedPath ); + + // Unpack must refuse the escaping path rather than write outside the destination. + CarbonResources::BundleUnpackParams bundleUnpackParams; + + bundleUnpackParams.chunkSourceSettings.sourceType = CarbonResources::ResourceSourceType::LOCAL_CDN; + + bundleUnpackParams.chunkSourceSettings.basePaths = { bundleCreateParams.chunkDestinationSettings.basePath }; + + bundleUnpackParams.resourceDestinationSettings.destinationType = CarbonResources::ResourceDestinationType::LOCAL_RELATIVE; + + bundleUnpackParams.resourceDestinationSettings.basePath = destinationBasePath; + + bundleUnpackParams.callbackSettings.statusCallback = StatusUpdate; + + EXPECT_EQ( bundleResourceGroup.Unpack( bundleUnpackParams ).type, CarbonResources::ResultType::MALFORMED_RESOURCE_INPUT ); + + EXPECT_FALSE( std::filesystem::exists( escapedPath ) ); +} + TEST_F( ResourcesLibraryTest, ApplyPatch ) { // Load the patch file diff --git a/tests/testData/ZipSlip/ZipSlipResourceGroup.yaml b/tests/testData/ZipSlip/ZipSlipResourceGroup.yaml new file mode 100644 index 0000000..7d3acd6 --- /dev/null +++ b/tests/testData/ZipSlip/ZipSlipResourceGroup.yaml @@ -0,0 +1,13 @@ +Version: 0.1.0 +Type: ResourceGroup +NumberOfResources: 1 +TotalResourcesSizeCompressed: 3312 +TotalResourcesSizeUnCompressed: 9719 +Resources: + - RelativePath: ../zipSlipEscaped.txt + Type: Resource + Location: a9/a9d1721dd5cc6d54_e6bbb2df307e5a9527159a4c971034b5 + Checksum: e6bbb2df307e5a9527159a4c971034b5 + UncompressedSize: 9719 + CompressedSize: 3312 + Prefix: res