fix(resourceinfo): reject manifest paths escaping the destination

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 )

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

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