Add safer handling of applying binary delta files (1.x)

.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

Sparkle.xcodeproj/xcshareddata/xcschemes/BinaryDelta.xcscheme

@@ -0,0 +1,78 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<Scheme
+   LastUpgradeVersion = "1310"
+   version = "1.3">
+   <BuildAction
+      parallelizeBuildables = "YES"
+      buildImplicitDependencies = "YES">
+      <BuildActionEntries>
+         <BuildActionEntry
+            buildForTesting = "YES"
+            buildForRunning = "YES"
+            buildForProfiling = "YES"
+            buildForArchiving = "YES"
+            buildForAnalyzing = "YES">
+            <BuildableReference
+               BuildableIdentifier = "primary"
+               BlueprintIdentifier = "5D06E8CF0FD68C7C005AE3F6"
+               BuildableName = "BinaryDelta"
+               BlueprintName = "BinaryDelta"
+               ReferencedContainer = "container:Sparkle.xcodeproj">
+            </BuildableReference>
+         </BuildActionEntry>
+      </BuildActionEntries>
+   </BuildAction>
+   <TestAction
+      buildConfiguration = "Debug"
+      selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
+      selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      shouldUseLaunchSchemeArgsEnv = "YES">
+      <Testables>
+      </Testables>
+   </TestAction>
+   <LaunchAction
+      buildConfiguration = "Debug"
+      selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
+      selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
+      launchStyle = "0"
+      useCustomWorkingDirectory = "NO"
+      ignoresPersistentStateOnLaunch = "NO"
+      debugDocumentVersioning = "YES"
+      debugServiceExtension = "internal"
+      allowLocationSimulation = "YES">
+      <BuildableProductRunnable
+         runnableDebuggingMode = "0">
+         <BuildableReference
+            BuildableIdentifier = "primary"
+            BlueprintIdentifier = "5D06E8CF0FD68C7C005AE3F6"
+            BuildableName = "BinaryDelta"
+            BlueprintName = "BinaryDelta"
+            ReferencedContainer = "container:Sparkle.xcodeproj">
+         </BuildableReference>
+      </BuildableProductRunnable>
+   </LaunchAction>
+   <ProfileAction
+      buildConfiguration = "Release"
+      shouldUseLaunchSchemeArgsEnv = "YES"
+      savedToolIdentifier = ""
+      useCustomWorkingDirectory = "NO"
+      debugDocumentVersioning = "YES">
+      <BuildableProductRunnable
+         runnableDebuggingMode = "0">
+         <BuildableReference
+            BuildableIdentifier = "primary"
+            BlueprintIdentifier = "5D06E8CF0FD68C7C005AE3F6"
+            BuildableName = "BinaryDelta"
+            BlueprintName = "BinaryDelta"
+            ReferencedContainer = "container:Sparkle.xcodeproj">
+         </BuildableReference>
+      </BuildableProductRunnable>
+   </ProfileAction>
+   <AnalyzeAction
+      buildConfiguration = "Debug">
+   </AnalyzeAction>
+   <ArchiveAction
+      buildConfiguration = "Release"
+      revealArchiveInOrganizer = "YES">
+   </ArchiveAction>
+</Scheme>

Sparkle/SUBinaryDeltaApply.m

@@ -14,9 +14,22 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <xar/xar.h>
+#include <Availability.h>
 
 #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,25 +99,45 @@ 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] }];
         }
         return NO;
     }
 
     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<NSFileAttributeKey, id> *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);

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);

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.

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];