Permalink
Browse files

More security tidbits!

This patch prevents malicious downgrades, which are still possible with DSA validation: suppose there's some (signed) version with a security hole. A malicious attacker could serve an appcast with that version's URL and DSA signature, but a higher version number, forcing the user to "upgrade" to the version with the security hole.

While I was at it, I fixed a bug that should have completely stopped .pkg installation from working since 1.5b1. Why didn't I hear anything about that? Does anyone actually use .pkgs? It still needs testing to be sure it works.
  • Loading branch information...
1 parent 95f9658 commit 9a0acca84976fa4af2939dec19954ef3ba7b2898 @andymatuschak andymatuschak committed Sep 10, 2008
Showing with 31 additions and 20 deletions.
  1. +1 −1 SUBasicUpdateDriver.m
  2. +1 −0 SUConstants.h
  3. +1 −0 SUConstants.m
  4. +2 −1 SUInstaller.h
  5. +2 −6 SUInstaller.m
  6. +0 −1 SUPackageInstaller.h
  7. +2 −2 SUPackageInstaller.m
  8. +2 −1 SUPlainInstaller.h
  9. +20 −8 SUPlainInstaller.m
View
@@ -227,7 +227,7 @@ - (void)installUpdate
if ([[NSFileManager defaultManager] copyPath:relaunchPathToCopy toPath:targetPath handler:nil])
relaunchPath = [targetPath retain];
- [SUInstaller installFromUpdateFolder:[downloadPath stringByDeletingLastPathComponent] overHost:host delegate:self synchronously:[self shouldInstallSynchronously]];
+ [SUInstaller installFromUpdateFolder:[downloadPath stringByDeletingLastPathComponent] overHost:host delegate:self synchronously:[self shouldInstallSynchronously] versionComparator:[self _versionComparator]];
}
- (void)installerFinishedForHost:(SUHost *)aHost
View
@@ -51,6 +51,7 @@ extern OSStatus SUMissingUpdateError;
extern OSStatus SUMissingInstallerToolError;
extern OSStatus SURelaunchError;
extern OSStatus SUInstallationError;
+extern OSStatus SUDowngradeError;
// NSInteger is a type that was added to Leopard.
// Here is some glue to ensure that NSInteger will work with pre-10.5 SDKs:
View
@@ -46,3 +46,4 @@
OSStatus SUMissingInstallerToolError = 4003;
OSStatus SURelaunchError = 4004;
OSStatus SUInstallationError = 4005;
+OSStatus SUDowngradeError = 4006;
View
@@ -10,10 +10,11 @@
#define SUINSTALLER_H
#import <Cocoa/Cocoa.h>
+#import "SUVersionComparisonProtocol.h"
@class SUHost;
@interface SUInstaller : NSObject { }
-+ (void)installFromUpdateFolder:(NSString *)updateFolder overHost:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously;
++ (void)installFromUpdateFolder:(NSString *)updateFolder overHost:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously versionComparator:(id <SUVersionComparison>)comparator;
+ (void)_finishInstallationWithResult:(BOOL)result host:(SUHost *)host error:(NSError *)error delegate:delegate;
@end
View
@@ -10,10 +10,6 @@
#import "SUPlainInstaller.h"
#import "SUPackageInstaller.h"
-NSString *SUInstallerPathKey = @"SUInstallerPath";
-NSString *SUInstallerHostKey = @"SUInstallerHost";
-NSString *SUInstallerDelegateKey = @"SUInstallerDelegate";
-
@implementation SUInstaller
+ (BOOL)_isAliasFolderAtPath:(NSString *)path
@@ -36,7 +32,7 @@ + (BOOL)_isAliasFolderAtPath:(NSString *)path
}
-+ (void)installFromUpdateFolder:(NSString *)updateFolder overHost:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously
++ (void)installFromUpdateFolder:(NSString *)updateFolder overHost:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously versionComparator:(id <SUVersionComparison>)comparator
{
// Search subdirectories for the application
NSString *currentFile, *newAppDownloadPath = nil, *bundleFileName = [[host bundlePath] lastPathComponent], *alternateBundleFileName = [[host name] stringByAppendingPathExtension:[[host bundlePath] pathExtension]];
@@ -76,7 +72,7 @@ + (void)installFromUpdateFolder:(NSString *)updateFolder overHost:(SUHost *)host
}
else
{
- [(isPackage ? [SUPackageInstaller class] : [SUPlainInstaller class]) performInstallationWithPath:newAppDownloadPath host:host delegate:delegate synchronously:synchronously];
+ [(isPackage ? [SUPackageInstaller class] : [SUPlainInstaller class]) performInstallationWithPath:newAppDownloadPath host:host delegate:delegate synchronously:synchronously versionComparator:comparator];
}
}
View
@@ -13,7 +13,6 @@
#import "SUPlainInstaller.h"
@interface SUPackageInstaller : SUPlainInstaller { }
-+ (void)installPath:(NSString *)path overHost:(SUHost *)bundle delegate:delegate;
@end
#endif
View
@@ -11,7 +11,7 @@
@implementation SUPackageInstaller
-+ (void)installPath:(NSString *)path overHost:(SUHost *)bundle delegate:delegate
++ (void)performInstallationWithPath:(NSString *)path host:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously versionComparator:(id <SUVersionComparison>)comparator;
{
NSError *error = nil;
BOOL result = YES;
@@ -26,7 +26,7 @@ + (void)installPath:(NSString *)path overHost:(SUHost *)bundle delegate:delegate
NSTask *installer = [NSTask launchedTaskWithLaunchPath:installerPath arguments:[NSArray arrayWithObjects:path, nil]];
[installer waitUntilExit];
// Known bug: if the installation fails or is canceled, Sparkle goes ahead and restarts, thinking everything is fine.
- [self _finishInstallationWithResult:result host:bundle error:error delegate:delegate];
+ [self _finishInstallationWithResult:result host:host error:error delegate:delegate];
}
@end
View
@@ -11,9 +11,10 @@
#import "Sparkle.h"
#import "SUInstaller.h"
+#import "SUVersionComparisonProtocol.h"
@interface SUPlainInstaller : SUInstaller { }
-+ (void)performInstallationWithPath:(NSString *)path host:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously;
++ (void)performInstallationWithPath:(NSString *)path host:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously versionComparator:(id <SUVersionComparison>)comparator;
@end
#endif
View
@@ -9,31 +9,43 @@
#import "SUPlainInstaller.h"
#import "SUPlainInstallerInternals.h"
-extern NSString *SUInstallerPathKey;
-extern NSString *SUInstallerHostKey;
-extern NSString *SUInstallerDelegateKey;
+NSString *SUInstallerPathKey = @"SUInstallerPath";
+NSString *SUInstallerHostKey = @"SUInstallerHost";
+NSString *SUInstallerDelegateKey = @"SUInstallerDelegate";
+NSString *SUInstallerVersionComparatorKey = @"SUInstallerVersionComparator";
@implementation SUPlainInstaller
-+ (void)installPath:(NSString *)path overHost:(SUHost *)bundle delegate:delegate
++ (void)installPath:(NSString *)path overHost:(SUHost *)bundle delegate:delegate versionComparator:(id <SUVersionComparison>)comparator
{
NSError *error;
- BOOL result = [self copyPathWithAuthentication:path overPath:[bundle bundlePath] error:&error];
+ BOOL result = YES;
+
+ // Prevent malicious downgrades:
+ if ([comparator compareVersion:[bundle version] toVersion:[[NSBundle bundleWithPath:path] objectForInfoDictionaryKey:@"CFBundleVersion"]] == NSOrderedDescending)
+ {
+ NSString * errorMessage = [NSString stringWithFormat:@"Sparkle Updater: Possible attack in progress! Attempting to \"upgrade\" from %@ to %@. Aborting update.", [bundle version], [[NSBundle bundleWithPath:path] objectForInfoDictionaryKey:@"CFBundleVersion"]];
+ result = NO;
+ error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUDowngradeError userInfo:[NSDictionary dictionaryWithObject:errorMessage forKey:NSLocalizedDescriptionKey]];
+ }
+
+ if (result)
+ result = [self copyPathWithAuthentication:path overPath:[bundle bundlePath] error:&error];
[self _finishInstallationWithResult:result host:bundle error:error delegate:delegate];
}
+ (void)_performInstallationWithInfo:(NSDictionary *)info
{
NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
- [self installPath:[info objectForKey:SUInstallerPathKey] overHost:[info objectForKey:SUInstallerHostKey] delegate:[info objectForKey:SUInstallerDelegateKey]];
+ [self installPath:[info objectForKey:SUInstallerPathKey] overHost:[info objectForKey:SUInstallerHostKey] delegate:[info objectForKey:SUInstallerDelegateKey] versionComparator:[info objectForKey:SUInstallerVersionComparatorKey]];
[pool drain];
}
-+ (void)performInstallationWithPath:(NSString *)path host:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously;
++ (void)performInstallationWithPath:(NSString *)path host:(SUHost *)host delegate:delegate synchronously:(BOOL)synchronously versionComparator:(id <SUVersionComparison>)comparator
{
- NSDictionary *info = [NSDictionary dictionaryWithObjectsAndKeys:path, SUInstallerPathKey, host, SUInstallerHostKey, delegate, SUInstallerDelegateKey, nil];
+ NSDictionary *info = [NSDictionary dictionaryWithObjectsAndKeys:path, SUInstallerPathKey, host, SUInstallerHostKey, delegate, SUInstallerDelegateKey, comparator, SUInstallerVersionComparatorKey, nil];
if (synchronously)
[self _performInstallationWithInfo:info];
else

0 comments on commit 9a0acca

Please sign in to comment.