From 7b109be51d36b662fc86492ccdfba20eb62b373d Mon Sep 17 00:00:00 2001 From: Zorg Date: Fri, 29 Oct 2021 12:15:45 -0700 Subject: [PATCH] Add safer handling for applying binary delta files (1.x) (#1990) --- .github/workflows/ci.yml | 4 +- .../xcschemes/BinaryDelta.xcscheme | 78 +++++++++++++++ Sparkle/SUBinaryDeltaApply.m | 99 ++++++++++++++++++- Sparkle/SUBinaryDeltaCommon.h | 8 +- Sparkle/SUBinaryDeltaCommon.m | 10 +- Sparkle/SUBinaryDeltaCreate.m | 12 +-- Tests/SUBinaryDeltaTest.m | 2 +- generate_appcast/ArchiveItem.swift | 2 +- 8 files changed, 193 insertions(+), 22 deletions(-) create mode 100644 Sparkle.xcodeproj/xcshareddata/xcschemes/BinaryDelta.xcscheme diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0c1a21a557..c8d39af714 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,10 +14,10 @@ jobs: include: - xcode: 'xcode12.5.1' xcode-path: '/Applications/Xcode_12.5.1.app/Contents/Developer' - upload-dist: true + upload-dist: false - xcode: 'xcode13.1' xcode-path: '/Applications/Xcode_13.1.app/Contents/Developer' - upload-dist: false # In Beta, so no upload + upload-dist: true name: Build and Test Sparkle runs-on: macos-11 diff --git a/Sparkle.xcodeproj/xcshareddata/xcschemes/BinaryDelta.xcscheme b/Sparkle.xcodeproj/xcshareddata/xcschemes/BinaryDelta.xcscheme new file mode 100644 index 0000000000..6b4385b695 --- /dev/null +++ b/Sparkle.xcodeproj/xcshareddata/xcschemes/BinaryDelta.xcscheme @@ -0,0 +1,78 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Sparkle/SUBinaryDeltaApply.m b/Sparkle/SUBinaryDeltaApply.m index 3eeee8d487..787fdcf3a2 100644 --- a/Sparkle/SUBinaryDeltaApply.m +++ b/Sparkle/SUBinaryDeltaApply.m @@ -14,9 +14,22 @@ #include #include #include +#include #include "AppKitPrevention.h" +#if __MAC_OS_X_VERSION_MAX_ALLOWED >= 120000 + #define HAS_XAR_GET_SAFE_PATH 1 +#else + #define HAS_XAR_GET_SAFE_PATH 0 +#endif + +#if HAS_XAR_GET_SAFE_PATH +// This is preferred over xar_get_path (which is deprecated) when it's available +// Don't use OS availability for this API +extern char *xar_get_safe_path(xar_file_t f) __attribute__((weak_import)); +#endif + static BOOL applyBinaryDeltaToFile(xar_t x, xar_file_t file, NSString *sourceFilePath, NSString *destinationFilePath) { NSString *patchFile = temporaryFilename(@"apply-binary-delta"); @@ -86,6 +99,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi } if (majorDiffVersion < FIRST_DELTA_DIFF_MAJOR_VERSION) { + xar_close(x); + if (error != NULL) { *error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileReadCorruptFileError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Unable to identify diff-version %u in delta. Giving up.", majorDiffVersion] }]; } @@ -93,18 +108,36 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi } if (majorDiffVersion > LATEST_DELTA_DIFF_MAJOR_VERSION) { + xar_close(x); + if (error != NULL) { *error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileReadUnknownError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"A later version is needed to apply this patch (on major version %u, but patch requests version %u).", LATEST_DELTA_DIFF_MAJOR_VERSION, majorDiffVersion] }]; } return NO; } + +#if HAS_XAR_GET_SAFE_PATH + // Reject patches that did not generate valid hierarchical xar container paths + // These will not succeed to patch using recent versions of BinaryDelta + if ((majorDiffVersion == SUBinaryDeltaMajorVersion2 && minorDiffVersion < 3) || (majorDiffVersion == SUBinaryDeltaMajorVersion1 && minorDiffVersion < 2)) { + xar_close(x); + + if (error != NULL) { + *error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileReadCorruptFileError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"This patch version (%u.%u) is too old and potentially unsafe to apply. Please re-generate the patch using the latest version of BinaryDelta or generate_appcast. New version %u.%u patches will still be compatible with older versions of Sparkle.", majorDiffVersion, minorDiffVersion, majorDiffVersion, latestMinorVersionForMajorVersion(majorDiffVersion)] }]; + } + + return NO; + } +#endif - BOOL usesNewTreeHash = MAJOR_VERSION_IS_AT_LEAST(majorDiffVersion, SUBeigeMajorVersion); + BOOL usesNewTreeHash = MAJOR_VERSION_IS_AT_LEAST(majorDiffVersion, SUBinaryDeltaMajorVersion2); NSString *expectedBeforeHash = usesNewTreeHash ? expectedNewBeforeHash : expectedBeforeHashv1; NSString *expectedAfterHash = usesNewTreeHash ? expectedNewAfterHash : expectedAfterHashv1; if (!expectedBeforeHash || !expectedAfterHash) { + xar_close(x); + if (error != NULL) { *error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileReadCorruptFileError userInfo:@{ NSLocalizedDescriptionKey: @"Unable to find before-sha1 or after-sha1 metadata in delta. Giving up." }]; } @@ -120,6 +153,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi NSString *beforeHash = hashOfTreeWithVersion(source, majorDiffVersion); if (!beforeHash) { + xar_close(x); + if (verbose) { fprintf(stderr, "\n"); } @@ -130,6 +165,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi } if (![beforeHash isEqualToString:expectedBeforeHash]) { + xar_close(x); + if (verbose) { fprintf(stderr, "\n"); } @@ -146,6 +183,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi progressCallback(2/6.0); if (!removeTree(destination)) { + xar_close(x); + if (verbose) { fprintf(stderr, "\n"); } @@ -158,6 +197,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi progressCallback(3/6.0); if (!copyTree(source, destination)) { + xar_close(x); + if (verbose) { fprintf(stderr, "\n"); } @@ -169,7 +210,7 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi progressCallback(4/6.0); - BOOL hasExtractKeyAvailable = MAJOR_VERSION_IS_AT_LEAST(majorDiffVersion, SUBeigeMajorVersion); + BOOL hasExtractKeyAvailable = MAJOR_VERSION_IS_AT_LEAST(majorDiffVersion, SUBinaryDeltaMajorVersion2); if (verbose) { fprintf(stderr, "\nPatching..."); @@ -178,9 +219,52 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi xar_file_t file; xar_iter_t iter = xar_iter_new(); for (file = xar_file_first(x, iter); file; file = xar_file_next(iter)) { - NSString *path = @(xar_get_path(file)); + char *pathCString; +#if HAS_XAR_GET_SAFE_PATH + if (xar_get_safe_path != NULL) { + pathCString = xar_get_safe_path(file); + } + else +#endif + { + pathCString = xar_get_path(file); + } + + if (pathCString == NULL) { + continue; + } + + NSString *path = @(pathCString); + if ([path.pathComponents containsObject:@".."]) { + xar_close(x); + + if (error != NULL) { + *error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileWriteUnknownError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Relative path '%@' contains '..' path component", path] }]; + } + return NO; + } + NSString *sourceFilePath = [source stringByAppendingPathComponent:path]; NSString *destinationFilePath = [destination stringByAppendingPathComponent:path]; + { + NSString *destinationParentDirectory = destinationFilePath.stringByDeletingLastPathComponent; + NSDictionary *destinationParentDirectoryAttributes = [fileManager attributesOfItemAtPath:destinationParentDirectory error:NULL]; + + // It is OK for the directory parent to not exist if it has already been removed + if (destinationParentDirectoryAttributes != nil) { + // But if it does exist, make sure the entry in the parent directory we're looking at is good + // If it's inside a symlink, this is not good in any circumstance + NSString *fileType = destinationParentDirectoryAttributes[NSFileType]; + if ([fileType isEqualToString:NSFileTypeSymbolicLink]) { + xar_close(x); + + if (error != NULL) { + *error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileWriteUnknownError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Failed to create patch because '%@' cannot be a symbolic link.", destinationParentDirectory] }]; + } + return NO; + } + } + } // Don't use -[NSFileManager fileExistsAtPath:] because it will follow symbolic links BOOL fileExisted = verbose && [fileManager attributesOfItemAtPath:destinationFilePath error:nil]; @@ -192,6 +276,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi const char *value; if (!xar_prop_get(file, DELETE_KEY, &value) || (!hasExtractKeyAvailable && !xar_prop_get(file, DELETE_THEN_EXTRACT_OLD_KEY, &value))) { if (!removeTree(destinationFilePath)) { + xar_close(x); + if (verbose) { fprintf(stderr, "\n"); } @@ -212,6 +298,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi if (!xar_prop_get(file, BINARY_DELTA_KEY, &value)) { if (!applyBinaryDeltaToFile(x, file, sourceFilePath, destinationFilePath)) { + xar_close(x); + if (verbose) { fprintf(stderr, "\n"); } @@ -227,6 +315,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi } else if ((hasExtractKeyAvailable && !xar_prop_get(file, EXTRACT_KEY, &value)) || (!hasExtractKeyAvailable && xar_prop_get(file, MODIFY_PERMISSIONS_KEY, &value))) { // extract and permission modifications don't coexist if (xar_extract_tofile(x, file, [destinationFilePath fileSystemRepresentation]) != 0) { + xar_close(x); + if (verbose) { fprintf(stderr, "\n"); } @@ -250,6 +340,8 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi if (!xar_prop_get(file, MODIFY_PERMISSIONS_KEY, &value)) { mode_t mode = (mode_t)[[NSString stringWithUTF8String:value] intValue]; if (!modifyPermissions(destinationFilePath, mode)) { + xar_close(x); + if (verbose) { fprintf(stderr, "\n"); } @@ -264,6 +356,7 @@ BOOL applyBinaryDelta(NSString *source, NSString *destination, NSString *patchFi } } } + xar_close(x); progressCallback(5/6.0); diff --git a/Sparkle/SUBinaryDeltaCommon.h b/Sparkle/SUBinaryDeltaCommon.h index 6bc572b2fa..26388eed78 100644 --- a/Sparkle/SUBinaryDeltaCommon.h +++ b/Sparkle/SUBinaryDeltaCommon.h @@ -53,12 +53,12 @@ typedef NS_ENUM(uint16_t, SUBinaryDeltaMajorVersion) { - SUAzureMajorVersion = 1, - SUBeigeMajorVersion = 2 + SUBinaryDeltaMajorVersion1 = 1, + SUBinaryDeltaMajorVersion2 = 2 }; -#define FIRST_DELTA_DIFF_MAJOR_VERSION SUAzureMajorVersion -#define LATEST_DELTA_DIFF_MAJOR_VERSION SUBeigeMajorVersion +#define FIRST_DELTA_DIFF_MAJOR_VERSION SUBinaryDeltaMajorVersion1 +#define LATEST_DELTA_DIFF_MAJOR_VERSION SUBinaryDeltaMajorVersion2 extern int compareFiles(const FTSENT **a, const FTSENT **b); extern NSString *hashOfTreeWithVersion(NSString *path, uint16_t majorVersion); diff --git a/Sparkle/SUBinaryDeltaCommon.m b/Sparkle/SUBinaryDeltaCommon.m index babfb1328d..8f4b941809 100644 --- a/Sparkle/SUBinaryDeltaCommon.m +++ b/Sparkle/SUBinaryDeltaCommon.m @@ -42,9 +42,9 @@ int compareFiles(const FTSENT **a, const FTSENT **b) int latestMinorVersionForMajorVersion(SUBinaryDeltaMajorVersion majorVersion) { switch (majorVersion) { - case SUAzureMajorVersion: - return 1; - case SUBeigeMajorVersion: + case SUBinaryDeltaMajorVersion1: + return 2; + case SUBinaryDeltaMajorVersion2: return 3; } return 0; @@ -162,7 +162,7 @@ static BOOL _hashOfFileContents(unsigned char *hash, FTSENT *ent) if (ent->fts_info != FTS_F && ent->fts_info != FTS_SL && ent->fts_info != FTS_D) continue; - if (ent->fts_info == FTS_D && !MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBeigeMajorVersion)) { + if (ent->fts_info == FTS_D && !MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBinaryDeltaMajorVersion2)) { continue; } @@ -179,7 +179,7 @@ static BOOL _hashOfFileContents(unsigned char *hash, FTSENT *ent) const char *relativePathBytes = [relativePath fileSystemRepresentation]; CC_SHA1_Update(&hashContext, relativePathBytes, (CC_LONG)strlen(relativePathBytes)); - if (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBeigeMajorVersion)) { + if (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBinaryDeltaMajorVersion2)) { uint16_t mode = ent->fts_statp->st_mode; // permission of symlinks is irrelevant and can't be changed. // hardcoding a value helps avoid differences between filesystems. diff --git a/Sparkle/SUBinaryDeltaCreate.m b/Sparkle/SUBinaryDeltaCreate.m index c3355b9fab..5074194da1 100644 --- a/Sparkle/SUBinaryDeltaCreate.m +++ b/Sparkle/SUBinaryDeltaCreate.m @@ -527,8 +527,8 @@ BOOL createBinaryDelta(NSString *source, NSString *destination, NSString *patchF xar_subdoc_prop_set(attributes, MINOR_DIFF_VERSION_KEY, [[NSString stringWithFormat:@"%u", minorVersion] UTF8String]); // Version 1 patches don't have a major or minor version field, so we need to differentiate between the hash keys - const char *beforeHashKey = MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBeigeMajorVersion) ? BEFORE_TREE_SHA1_KEY : BEFORE_TREE_SHA1_OLD_KEY; - const char *afterHashKey = MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBeigeMajorVersion) ? AFTER_TREE_SHA1_KEY : AFTER_TREE_SHA1_OLD_KEY; + const char *beforeHashKey = MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBinaryDeltaMajorVersion2) ? BEFORE_TREE_SHA1_KEY : BEFORE_TREE_SHA1_OLD_KEY; + const char *afterHashKey = MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBinaryDeltaMajorVersion2) ? AFTER_TREE_SHA1_KEY : AFTER_TREE_SHA1_OLD_KEY; xar_subdoc_prop_set(attributes, beforeHashKey, [beforeHash UTF8String]); xar_subdoc_prop_set(attributes, afterHashKey, [afterHash UTF8String]); @@ -566,7 +566,7 @@ BOOL createBinaryDelta(NSString *source, NSString *destination, NSString *patchF NSDictionary *originalInfo = originalTreeState[key]; NSDictionary *newInfo = newTreeState[key]; if (shouldSkipDeltaCompression(originalInfo, newInfo)) { - if (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBeigeMajorVersion) && shouldSkipExtracting(originalInfo, newInfo)) { + if (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBinaryDeltaMajorVersion2) && shouldSkipExtracting(originalInfo, newInfo)) { if (shouldChangePermissions(originalInfo, newInfo)) { xar_file_t newFile = _xarAddFile(fileTable, x, key, NULL); assert(newFile); @@ -582,14 +582,14 @@ BOOL createBinaryDelta(NSString *source, NSString *destination, NSString *patchF assert(newFile); if (shouldDeleteThenExtract(originalInfo, newInfo)) { - if (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBeigeMajorVersion)) { + if (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBinaryDeltaMajorVersion2)) { xar_prop_set(newFile, DELETE_KEY, "true"); } else { xar_prop_set(newFile, DELETE_THEN_EXTRACT_OLD_KEY, "true"); } } - if (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBeigeMajorVersion)) { + if (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBinaryDeltaMajorVersion2)) { xar_prop_set(newFile, EXTRACT_KEY, "true"); } @@ -603,7 +603,7 @@ BOOL createBinaryDelta(NSString *source, NSString *destination, NSString *patchF } } else { NSNumber *permissions = - (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBeigeMajorVersion) && shouldChangePermissions(originalInfo, newInfo)) ? + (MAJOR_VERSION_IS_AT_LEAST(majorVersion, SUBinaryDeltaMajorVersion2) && shouldChangePermissions(originalInfo, newInfo)) ? newInfo[INFO_PERMISSIONS_KEY] : nil; CreateBinaryDeltaOperation *operation = [[CreateBinaryDeltaOperation alloc] initWithRelativePath:key oldTree:source newTree:destination oldPermissions:originalInfo[INFO_PERMISSIONS_KEY] newPermissions:permissions]; diff --git a/Tests/SUBinaryDeltaTest.m b/Tests/SUBinaryDeltaTest.m index 1a906adef9..e18b1efdd1 100644 --- a/Tests/SUBinaryDeltaTest.m +++ b/Tests/SUBinaryDeltaTest.m @@ -352,7 +352,7 @@ - (void)testRegularFileAdded // Make sure old version patches still work for simple cases - (void)testRegularFileAddedWithAzureVersion { - XCTAssertTrue([self createAndApplyPatchUsingVersion:SUAzureMajorVersion beforeDiffHandler:^(NSFileManager *__unused fileManager, NSString *sourceDirectory, NSString *destinationDirectory) { + XCTAssertTrue([self createAndApplyPatchUsingVersion:SUBinaryDeltaMajorVersion1 beforeDiffHandler:^(NSFileManager *__unused fileManager, NSString *sourceDirectory, NSString *destinationDirectory) { NSString *sourceFile = [sourceDirectory stringByAppendingPathComponent:@"A"]; NSString *destinationFile = [destinationDirectory stringByAppendingPathComponent:@"A"]; NSString *destinationFile2 = [destinationDirectory stringByAppendingPathComponent:@"B"]; diff --git a/generate_appcast/ArchiveItem.swift b/generate_appcast/ArchiveItem.swift index 145de107ec..c9facd9553 100644 --- a/generate_appcast/ArchiveItem.swift +++ b/generate_appcast/ArchiveItem.swift @@ -24,7 +24,7 @@ class DeltaUpdate { class func create(from: ArchiveItem, to: ArchiveItem, archivePath: URL) throws -> DeltaUpdate { var applyDiffError: NSError? - if !createBinaryDelta(from.appPath.path, to.appPath.path, archivePath.path, .beigeMajorVersion, false, &applyDiffError) { + if !createBinaryDelta(from.appPath.path, to.appPath.path, archivePath.path, .version2, false, &applyDiffError) { throw applyDiffError! }