Improve signing error message to developers (#2471)

If we detect the wrong archive is being served (i.e, expected content length differs from archive length) we log this out to developers. If the app version in the archive (if available) also differs, we report this discrepancy as well. If the update archive looks the same but signing validation fails, we tell the developer the update may have not been signed correctly.

I'm not changing the generic error that is reported to users about the update being improperly signed (that would involve propagating this information there and updating bunch of localizations). The extra info is more for the developer than it is for the user.

Fixes #2468

Autoupdate/AppInstaller.m

@@ -27,6 +27,7 @@
 #import "SPUInstallerAgentProtocol.h"
 #import "SPUInstallationType.h"
 #import "SPULocalCacheDirectory.h"
+#import "SPUVerifierInformation.h"
 
 
 #include "AppKitPrevention.h"
@@ -63,6 +64,7 @@ @implementation AppInstaller
     SUSignatures *_signatures;
     NSString *_relaunchPath;
     NSString *_installationType;
+    SPUVerifierInformation *_verifierInformation;
 
     id<SUInstallerProtocol> _installer;
 
@@ -179,7 +181,15 @@ - (void)extractAndInstallUpdate SPU_OBJC_DIRECT
 
         success = NO;
     } else {
-        _updateValidator = [[SUUpdateValidator alloc] initWithDownloadPath:archivePath signatures:_signatures host:_host];
+        NSError *fileAttributesError = nil;
+        NSDictionary<NSFileAttributeKey, id> *archiveFileAttributes = [NSFileManager.defaultManager attributesOfItemAtPath:archivePath error:&fileAttributesError];
+        if (archiveFileAttributes == nil) {
+            SULog(SULogLevelError, @"Failed to retrieve file attributes from archive: %@.", fileAttributesError);
+        } else {
+            _verifierInformation.actualContentLength = (uint64_t)(archiveFileAttributes.fileSize);
+        }
+
+        _updateValidator = [[SUUpdateValidator alloc] initWithDownloadPath:archivePath signatures:_signatures host:_host verifierInformation:_verifierInformation];
 
         // Delta & package updates will require validation before extraction
         // Normal application updates are a bit more lenient allowing developers to change one of apple dev ID or EdDSA keys
@@ -451,6 +461,7 @@ - (void)handleMessageWithIdentifier:(int32_t)identifier data:(NSData *)data
             self->_signatures = installationData.signatures;
             self->_updateDirectoryPath = cacheInstallationPath;
             self->_host = [[SUHost alloc] initWithBundle:hostBundle];
+            self->_verifierInformation = [[SPUVerifierInformation alloc] initWithExpectedVersion:installationData.expectedVersion expectedContentLength:installationData.expectedContentLength];
 
             [self extractAndInstallUpdate];
         });

Autoupdate/SPUInstallationInputData.h

@@ -21,15 +21,19 @@ SPU_OBJC_DIRECT_MEMBERS @interface SPUInstallationInputData : NSObject <NSSecure
  * downloadName - name of update archive in update directory
  * signatures - signatures for the update that came from the appcast item
  * decryptionPassword - optional decryption password for dmg archives
+ * expectedVersion - optional expected version of the new update
+ * expectedContentLength - optional expected content length of the new download archive
  */
-- (instancetype)initWithRelaunchPath:(NSString *)relaunchPath hostBundlePath:(NSString *)hostBundlePath updateURLBookmarkData:(NSData *)updateURLBookmarkData installationType:(NSString *)installationType signatures:(SUSignatures * _Nullable)signatures decryptionPassword:(nullable NSString *)decryptionPassword;
+- (instancetype)initWithRelaunchPath:(NSString *)relaunchPath hostBundlePath:(NSString *)hostBundlePath updateURLBookmarkData:(NSData *)updateURLBookmarkData installationType:(NSString *)installationType signatures:(SUSignatures * _Nullable)signatures decryptionPassword:(nullable NSString *)decryptionPassword expectedVersion:(NSString *)expectedVersion expectedContentLength:(uint64_t)expectedContentLength;
 
 @property (nonatomic, copy, readonly) NSString *relaunchPath;
 @property (nonatomic, copy, readonly) NSString *hostBundlePath;
 @property (nonatomic, copy, readonly) NSData *updateURLBookmarkData;
 @property (nonatomic, copy, readonly) NSString *installationType;
 @property (nonatomic, readonly, nullable) SUSignatures *signatures; // nullable because although not using signatures is deprecated, it's still supported
 @property (nonatomic, copy, readonly, nullable) NSString *decryptionPassword;
+@property (nonatomic, copy, readonly, nullable) NSString *expectedVersion;
+@property (nonatomic, readonly) uint64_t expectedContentLength;
 
 @end
 

Autoupdate/SPUInstallationInputData.m

@@ -18,6 +18,8 @@
 static NSString *SUSignaturesKey = @"SUSignatures";
 static NSString *SUDecryptionPasswordKey = @"SUDecryptionPassword";
 static NSString *SUInstallationTypeKey = @"SUInstallationType";
+static NSString *SUExpectedVersionKey = @"SUExpectedVersion";
+static NSString *SUExpectedContentLength = @"SUExpectedContentLength";
 
 @implementation SPUInstallationInputData
 
@@ -27,8 +29,10 @@ @implementation SPUInstallationInputData
 @synthesize signatures = _signatures;
 @synthesize decryptionPassword = _decryptionPassword;
 @synthesize installationType = _installationType;
+@synthesize expectedVersion = _expectedVersion;
+@synthesize expectedContentLength = _expectedContentLength;
 
-- (instancetype)initWithRelaunchPath:(NSString *)relaunchPath hostBundlePath:(NSString *)hostBundlePath updateURLBookmarkData:(NSData *)updateURLBookmarkData installationType:(NSString *)installationType signatures:(SUSignatures * _Nullable)signatures decryptionPassword:(nullable NSString *)decryptionPassword
+- (instancetype)initWithRelaunchPath:(NSString *)relaunchPath hostBundlePath:(NSString *)hostBundlePath updateURLBookmarkData:(NSData *)updateURLBookmarkData installationType:(NSString *)installationType signatures:(SUSignatures * _Nullable)signatures decryptionPassword:(nullable NSString *)decryptionPassword expectedVersion:(nonnull NSString *)expectedVersion expectedContentLength:(uint64_t)expectedContentLength
 {
     self = [super init];
     if (self != nil) {
@@ -41,6 +45,9 @@ - (instancetype)initWithRelaunchPath:(NSString *)relaunchPath hostBundlePath:(NS
 
         _signatures = signatures;
         _decryptionPassword = [decryptionPassword copy];
+
+        _expectedVersion = [expectedVersion copy];
+        _expectedContentLength = expectedContentLength;
     }
     return self;
 }
@@ -74,7 +81,10 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder
 
     NSString *decryptionPassword = [decoder decodeObjectOfClass:[NSString class] forKey:SUDecryptionPasswordKey];
 
-    return [self initWithRelaunchPath:relaunchPath hostBundlePath:hostBundlePath updateURLBookmarkData:updateURLBookmarkData installationType:installationType signatures:signatures decryptionPassword:decryptionPassword];
+    NSString *expectedVersion = [decoder decodeObjectOfClass:[NSString class] forKey:SUExpectedVersionKey];
+    uint64_t expectedContentLength = (uint64_t)[decoder decodeInt64ForKey:SUExpectedContentLength];
+
+    return [self initWithRelaunchPath:relaunchPath hostBundlePath:hostBundlePath updateURLBookmarkData:updateURLBookmarkData installationType:installationType signatures:signatures decryptionPassword:decryptionPassword expectedVersion:expectedVersion expectedContentLength:expectedContentLength];
 }
 
 - (void)encodeWithCoder:(NSCoder *)coder
@@ -87,6 +97,10 @@ - (void)encodeWithCoder:(NSCoder *)coder
     if (_decryptionPassword != nil) {
         [coder encodeObject:_decryptionPassword forKey:SUDecryptionPasswordKey];
     }
+    if (_expectedVersion != nil) {
+        [coder encodeObject:_expectedVersion forKey:SUExpectedVersionKey];
+    }
+    [coder encodeInt64:(int64_t)_expectedContentLength forKey:SUExpectedContentLength];
 }
 
 + (BOOL)supportsSecureCoding

Autoupdate/SUSignatureVerifier.h

@@ -18,15 +18,22 @@
 #import <Foundation/Foundation.h>
 @class SUSignatures;
 @class SUPublicKeys;
+@class SPUVerifierInformation;
+
+NS_ASSUME_NONNULL_BEGIN
 
 SPU_OBJC_DIRECT_MEMBERS @interface SUSignatureVerifier : NSObject
 
-+ (BOOL)validatePath:(NSString *)path withSignatures:(SUSignatures *)signatures withPublicKeys:(SUPublicKeys *)pkeys error:(NSError * __autoreleasing *)error;
++ (BOOL)validatePath:(NSString *)path withSignatures:(SUSignatures *)signatures withPublicKeys:(SUPublicKeys *)pkeys verifierInformation:(SPUVerifierInformation * _Nullable)verifierInformation error:(NSError * __autoreleasing *)error;
+
+- (instancetype)init NS_UNAVAILABLE;
 
 - (instancetype)initWithPublicKeys:(SUPublicKeys *)pkeys;
 
-- (BOOL)verifyFileAtPath:(NSString *)path signatures:(SUSignatures *)signatures error:(NSError * __autoreleasing *)error;
+- (BOOL)verifyFileAtPath:(NSString *)path signatures:(SUSignatures *)signatures verifierInformation:(SPUVerifierInformation * _Nullable)verifierInformation error:(NSError * __autoreleasing *)error;
 
 @end
 
+NS_ASSUME_NONNULL_END
+
 #endif

Autoupdate/SUSignatureVerifier.m

@@ -16,6 +16,8 @@
 #import "SULog.h"
 #import "SUSignatures.h"
 #import "SUErrors.h"
+#import "SPUVerifierInformation.h"
+#import "SUConstants.h"
 #if SPARKLE_BUILD_LEGACY_DSA_SUPPORT
 #include <CommonCrypto/CommonDigest.h>
 #endif
@@ -29,7 +31,7 @@ @implementation SUSignatureVerifier
     SUPublicKeys *_pubKeys;
 }
 
-+ (BOOL)validatePath:(NSString *)path withSignatures:(SUSignatures *)signatures withPublicKeys:(SUPublicKeys *)pkeys error:(NSError * __autoreleasing *)error
++ (BOOL)validatePath:(NSString *)path withSignatures:(SUSignatures *)signatures withPublicKeys:(SUPublicKeys *)pkeys verifierInformation:(SPUVerifierInformation * _Nullable)verifierInformation error:(NSError * __autoreleasing *)error
 {
     SUSignatureVerifier *verifier = [(SUSignatureVerifier *)[self alloc] initWithPublicKeys:pkeys];
 
@@ -40,7 +42,7 @@ + (BOOL)validatePath:(NSString *)path withSignatures:(SUSignatures *)signatures
         return NO;
     }
 
-    return [verifier verifyFileAtPath:path signatures:signatures error:error];
+    return [verifier verifyFileAtPath:path signatures:signatures verifierInformation:verifierInformation error:error];
 }
 
 - (instancetype)initWithPublicKeys:(SUPublicKeys *)pubkeys
@@ -90,7 +92,7 @@ - (SecKeyRef)dsaSecKeyRef SPU_OBJC_DIRECT
 }
 #endif
 
-- (BOOL)verifyFileAtPath:(NSString *)path signatures:(SUSignatures *)signatures error:(NSError * __autoreleasing *)error
+- (BOOL)verifyFileAtPath:(NSString *)path signatures:(SUSignatures *)signatures verifierInformation:(SPUVerifierInformation * _Nullable)verifierInformation error:(NSError * __autoreleasing *)error
 {
     if (!path || !path.length) {
         if (error != NULL) {
@@ -176,9 +178,31 @@ - (BOOL)verifyFileAtPath:(NSString *)path signatures:(SUSignatures *)signatures
                     return YES;
                 }
             } else {
-                NSString *message = @"EdDSA signature does not match. Data of the update file being checked is different than data that has been signed, or the public key and the private key are not from the same set.";
+                NSMutableString *message = [NSMutableString stringWithString:@"EdDSA signature does not match. Data of the update file being checked is different than data that has been signed, or the public key and the private key are not from the same set."];
 
-                SULog(SULogLevelError, @"%@", message);
+                // Elaborate on the error message if we have more information about the download archive
+                if (verifierInformation != nil) {
+                    BOOL reportedDiscrepancy = NO;
+
+                    if (verifierInformation.expectedContentLength > 0 && verifierInformation.actualContentLength > 0) {
+                        if (verifierInformation.expectedContentLength != verifierInformation.actualContentLength) {
+                            [message appendFormat:@" The downloaded update (%@) is likely different than the signed archive because the expected content length from the appcast item (%llu bytes) differs from the downloaded archive length (%llu bytes).", path.lastPathComponent, verifierInformation.expectedContentLength, verifierInformation.actualContentLength];
+                            reportedDiscrepancy = YES;
+                        }
+                    }
+
+                    NSString *actualVersion = verifierInformation.actualVersion;
+                    if (actualVersion != nil && ![verifierInformation.expectedVersion isEqualToString:actualVersion]) {
+                        [message appendFormat:@" The downloaded update (%@) also has a CFBundleVersion (%@) which differs from the %@ in the appcast item (%@).", path.lastPathComponent, actualVersion, SUAppcastAttributeVersion, verifierInformation.expectedVersion];
+                        reportedDiscrepancy = YES;
+                    }
+
+                    if (!reportedDiscrepancy && verifierInformation.expectedContentLength > 0) {
+                        [message appendFormat:@" The downloaded update (%@) is likely not signed correctly because the archive has the expected content length (%llu bytes)%@ which matches the appcast item.", path.lastPathComponent, verifierInformation.actualContentLength, (actualVersion == nil ? @"" : [NSString stringWithFormat:@" and CFBundleVersion (%@)", actualVersion])];
+                    }
+                }
+
+                SULog(SULogLevelError, @"%@", [message copy]);
 
 #if SPARKLE_BUILD_LEGACY_DSA_SUPPORT
                 if (signatures.dsaSignatureStatus != SUSigningInputStatusAbsent) {

Sparkle.xcodeproj/project.pbxproj

@@ -392,6 +392,8 @@
 		72BC6C3D275027BF0083F14B /* SparkleTestCodeSign_apfs.dmg in Resources */ = {isa = PBXBuildFile; fileRef = 72BC6C3C275027BF0083F14B /* SparkleTestCodeSign_apfs.dmg */; };
 		72BEBFEF1D7287570019146B /* SUSpotlightImporterTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 72BEBFEE1D7287560019146B /* SUSpotlightImporterTest.swift */; };
 		72CCDEBE27421FD500B53718 /* SparkleTestCodeSignApp_bad_header.zip in Resources */ = {isa = PBXBuildFile; fileRef = 72CCDEBD27421FD500B53718 /* SparkleTestCodeSignApp_bad_header.zip */; };
+		72D04F3D2B094C8400A6DEAA /* SPUVerifierInformation.m in Sources */ = {isa = PBXBuildFile; fileRef = 72D04F3C2B094C8400A6DEAA /* SPUVerifierInformation.m */; };
+		72D04F3E2B097D4300A6DEAA /* SPUVerifierInformation.m in Sources */ = {isa = PBXBuildFile; fileRef = 72D04F3C2B094C8400A6DEAA /* SPUVerifierInformation.m */; };
 		72D60CD928C2BAE900189AB8 /* SPUStandardUserDriver+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 72D60CD828C2BA2100189AB8 /* SPUStandardUserDriver+Private.h */; settings = {ATTRIBUTES = (Private, ); }; };
 		72D954811CBACC35006F28BD /* InstallerProgressAppController.m in Sources */ = {isa = PBXBuildFile; fileRef = 72D954801CBACC35006F28BD /* InstallerProgressAppController.m */; };
 		72D954831CBAD34F006F28BD /* main.m in Sources */ = {isa = PBXBuildFile; fileRef = 72D954821CBAD34F006F28BD /* main.m */; };
@@ -1384,6 +1386,8 @@
 		72BC6C3C275027BF0083F14B /* SparkleTestCodeSign_apfs.dmg */ = {isa = PBXFileReference; lastKnownFileType = file; path = SparkleTestCodeSign_apfs.dmg; sourceTree = "<group>"; };
 		72BEBFEE1D7287560019146B /* SUSpotlightImporterTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SUSpotlightImporterTest.swift; sourceTree = "<group>"; };
 		72CCDEBD27421FD500B53718 /* SparkleTestCodeSignApp_bad_header.zip */ = {isa = PBXFileReference; lastKnownFileType = archive.zip; path = SparkleTestCodeSignApp_bad_header.zip; sourceTree = "<group>"; };
+		72D04F3B2B094C8400A6DEAA /* SPUVerifierInformation.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = SPUVerifierInformation.h; sourceTree = "<group>"; };
+		72D04F3C2B094C8400A6DEAA /* SPUVerifierInformation.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SPUVerifierInformation.m; sourceTree = "<group>"; };
 		72D60CD828C2BA2100189AB8 /* SPUStandardUserDriver+Private.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "SPUStandardUserDriver+Private.h"; sourceTree = "<group>"; };
 		72D9547F1CBACC35006F28BD /* InstallerProgressAppController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = InstallerProgressAppController.h; path = Sparkle/InstallerProgress/InstallerProgressAppController.h; sourceTree = SOURCE_ROOT; };
 		72D954801CBACC35006F28BD /* InstallerProgressAppController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = InstallerProgressAppController.m; path = Sparkle/InstallerProgress/InstallerProgressAppController.m; sourceTree = SOURCE_ROOT; };
@@ -2096,6 +2100,8 @@
 				7267E5991D3D8A5A00D1BF90 /* SUCodeSigningVerifier.m */,
 				7267E59A1D3D8A5A00D1BF90 /* SUSignatureVerifier.h */,
 				7267E59B1D3D8A5A00D1BF90 /* SUSignatureVerifier.m */,
+				72D04F3B2B094C8400A6DEAA /* SPUVerifierInformation.h */,
+				72D04F3C2B094C8400A6DEAA /* SPUVerifierInformation.m */,
 			);
 			name = Verifiers;
 			path = ..;
@@ -3344,6 +3350,7 @@
 			isa = PBXSourcesBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
+				72D04F3E2B097D4300A6DEAA /* SPUVerifierInformation.m in Sources */,
 				72266A8C29464D0200645376 /* SUSignatures.m in Sources */,
 				72266A8B29464CEA00645376 /* SUHost.m in Sources */,
 				72266A8A2946493C00645376 /* SUCodeSigningVerifier.m in Sources */,
@@ -3557,6 +3564,7 @@
 				7267E57F1D3D896700D1BF90 /* SUBinaryDeltaUnarchiver.m in Sources */,
 				7267E59C1D3D8A5A00D1BF90 /* SUCodeSigningVerifier.m in Sources */,
 				7267E5CC1D3D8C6B00D1BF90 /* SUConstants.m in Sources */,
+				72D04F3D2B094C8400A6DEAA /* SPUVerifierInformation.m in Sources */,
 				7267E5861D3D89B300D1BF90 /* SUDiskImageUnarchiver.m in Sources */,
 				7267E59D1D3D8A5A00D1BF90 /* SUSignatureVerifier.m in Sources */,
 				7267E5E71D3D90AA00D1BF90 /* SUFileManager.m in Sources */,

Sparkle/SPUInstallerDriver.m

@@ -249,7 +249,7 @@ - (void)sendInstallationData SPU_OBJC_DIRECT
 
     id<SPUInstallerDriverDelegate> delegate = _delegate;
 
-    SPUInstallationInputData *installationData = [[SPUInstallationInputData alloc] initWithRelaunchPath:pathToRelaunch hostBundlePath:_host.bundlePath updateURLBookmarkData:_updateURLBookmarkData installationType:_updateItem.installationType signatures:_updateItem.signatures decryptionPassword:decryptionPassword];
+    SPUInstallationInputData *installationData = [[SPUInstallationInputData alloc] initWithRelaunchPath:pathToRelaunch hostBundlePath:_host.bundlePath updateURLBookmarkData:_updateURLBookmarkData installationType:_updateItem.installationType signatures:_updateItem.signatures decryptionPassword:decryptionPassword expectedVersion:_updateItem.versionString expectedContentLength:_updateItem.contentLength];
 
     NSData *archivedData = SPUArchiveRootObjectSecurely(installationData);
     if (archivedData == nil) {

Sparkle/SPUVerifierInformation.h

@@ -0,0 +1,24 @@
+//
+//  SPUVerifierInformation.h
+//  Autoupdate
+//
+//  Copyright © 2023 Sparkle Project. All rights reserved.
+//
+
+#import <Foundation/Foundation.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+SPU_OBJC_DIRECT_MEMBERS @interface SPUVerifierInformation : NSObject
+
+- (instancetype)initWithExpectedVersion:(NSString * _Nullable)expectedVersion expectedContentLength:(uint64_t)expectedContentLength;
+
+@property (nonatomic, readonly, copy, nullable) NSString *expectedVersion;
+@property (nonatomic, readonly) uint64_t expectedContentLength;
+
+@property (nonatomic, copy, nullable) NSString *actualVersion;
+@property (nonatomic) uint64_t actualContentLength;
+
+@end
+
+NS_ASSUME_NONNULL_END

Sparkle/SPUVerifierInformation.m

@@ -0,0 +1,27 @@
+//
+//  SPUVerifierInformation.m
+//  Autoupdate
+//
+//  Copyright © 2023 Sparkle Project. All rights reserved.
+//
+
+#import "SPUVerifierInformation.h"
+
+@implementation SPUVerifierInformation
+
+@synthesize expectedVersion = _expectedVersion;
+@synthesize expectedContentLength = _expectedContentLength;
+@synthesize actualVersion = _actualVersion;
+@synthesize actualContentLength = _actualContentLength;
+
+- (instancetype)initWithExpectedVersion:(NSString *)expectedVersion expectedContentLength:(uint64_t)expectedContentLength
+{
+    self = [super init];
+    if (self != nil) {
+        _expectedVersion = [expectedVersion copy];
+        _expectedContentLength = expectedContentLength;
+    }
+    return self;
+}
+
+@end

Sparkle/SUAppcastItem+Private.h

@@ -10,6 +10,7 @@
 #define SUAppcastItem_Private_h
 
 #import <Foundation/Foundation.h>
+#import <Sparkle/SUAppcastItem.h>
 
 NS_ASSUME_NONNULL_BEGIN