Skip to content

Commit

Permalink
Fix authorized file moving code:
Browse files Browse the repository at this point in the history 
- We now correctly handle the case where the containing directory doesn't allow us adding files without authorization (e.g. /Applications).
- We do the quarantine release *before* the copy.
  • Loading branch information
@uliwitness
uliwitness committed Dec 14, 2009
1 parent 27f2b9c commit e49a698
Showing 1 changed file with 37 additions and 21 deletions.
58 changes: 37 additions & 21 deletions SUPlainInstallerInternals.m
View file
Expand Up @@ -182,6 +182,24 @@ + (BOOL)_copyPathWithForcedAuthentication:(NSString *)src toPath:(NSString *)dst
snprintf(uidgid, sizeof(uidgid), "%d:%d",
dstSB.st_uid, dstSB.st_gid);

// If the currently-running application is trusted, the new
// version should be trusted as well. Remove it from the
// quarantine to avoid a delay at launch, and to avoid
// presenting the user with a confusing trust dialog.
//
// This needs to be done after the application is moved to its
// new home with "mv" in case it's moved across filesystems: if
// that happens, "mv" actually performs a copy and may result
// in the application being quarantined. It also needs to be
// done before "chown" changes ownership, because the ownership
// change will almost certainly make it impossible to change
// attributes to release the files from the quarantine.
if (res)
{
SULog(@"releaseFromQuarantine");
[self performSelectorOnMainThread:@selector(releaseFromQuarantine:) withObject:dst waitUntilDone:YES];
}

if( res ) // Set permissions while it's still in source, so we have it with working and correct perms when it arrives at destination.
{
const char* coParams[] = { "-R", uidgid, srcPath, NULL };
Expand Down Expand Up @@ -213,22 +231,6 @@ + (BOOL)_copyPathWithForcedAuthentication:(NSString *)src toPath:(NSString *)dst
// res = AuthorizationExecuteWithPrivilegesAndWait( auth, "/bin/rm", kAuthorizationFlagDefaults, rmParams2 );
// }

// If the currently-running application is trusted, the new
// version should be trusted as well. Remove it from the
// quarantine to avoid a delay at launch, and to avoid
// presenting the user with a confusing trust dialog.
//
// This needs to be done after the application is moved to its
// new home with "mv" in case it's moved across filesystems: if
// that happens, "mv" actually performs a copy and may result
// in the application being quarantined. It also needs to be
// done before "chown" changes ownership, because the ownership
// change will almost certainly make it impossible to change
// attributes to release the files from the quarantine.
if (res) {
[self performSelectorOnMainThread:@selector(releaseFromQuarantine:) withObject:dst waitUntilDone:YES];
}

AuthorizationFree(auth, 0);

if (!res)
Expand Down Expand Up @@ -342,17 +344,28 @@ + (void)_movePathToTrash:(NSString *)path

+ (BOOL)copyPathWithAuthentication:(NSString *)src overPath:(NSString *)dst temporaryName:(NSString *)tmp error:(NSError **)error
{
FSRef srcRef, dstRef, dstDirRev, movedRef, tmpDirRef;
FSRef srcRef, dstRef, dstDirRef, movedRef, tmpDirRef;
OSErr err;
BOOL hadFileAtDest = NO, didFindTrash = NO;
NSString *tmpPath = [self _temporaryCopyNameForPath: dst didFindTrash: &didFindTrash];

// Make FSRef for destination:
err = FSPathMakeRefWithOptions((UInt8 *)[dst fileSystemRepresentation], kFSPathMakeRefDoNotFollowLeafSymlink, &dstRef, NULL);
hadFileAtDest = (err == noErr); // There is a file at the destination, move it aside. If we normalized the name, we might not get here, so don't error.
if( hadFileAtDest )
{
if (0 != access([dst fileSystemRepresentation], W_OK) || 0 != access([[dst stringByDeletingLastPathComponent] fileSystemRepresentation], W_OK))
{
return [self _copyPathWithForcedAuthentication:src toPath:dst temporaryPath:tmpPath error:error];
}
}
else
{
if (0 != access([[dst stringByDeletingLastPathComponent] fileSystemRepresentation], W_OK)
|| 0 != access([[[dst stringByDeletingLastPathComponent] stringByDeletingLastPathComponent] fileSystemRepresentation], W_OK))
{
return [self _copyPathWithForcedAuthentication:src toPath:dst temporaryPath:tmpPath error:error];
}
}

if( hadFileAtDest )
Expand All @@ -362,9 +375,12 @@ + (BOOL)copyPathWithAuthentication:(NSString *)src overPath:(NSString *)dst temp
err = FSPathMakeRef((UInt8 *)[[dst stringByDeletingLastPathComponent] fileSystemRepresentation], &tmpDirRef, NULL);
}

err = FSPathMakeRef((UInt8 *)[[dst stringByDeletingLastPathComponent] fileSystemRepresentation], &dstDirRev, NULL);
if (err == noErr)
err = FSPathMakeRef((UInt8 *)[[dst stringByDeletingLastPathComponent] fileSystemRepresentation], &dstDirRef, NULL);

if (err == noErr && hadFileAtDest)
{
err = FSMoveObjectSync(&dstRef, &tmpDirRef, (CFStringRef)[tmpPath lastPathComponent], &movedRef, 0);
}
if (err != noErr && hadFileAtDest)
{
if (error != NULL)
Expand All @@ -373,12 +389,12 @@ + (BOOL)copyPathWithAuthentication:(NSString *)src overPath:(NSString *)dst temp
}
err = FSPathMakeRef((UInt8 *)[src fileSystemRepresentation], &srcRef, NULL);
if (err == noErr)
err = FSCopyObjectSync(&srcRef, &dstDirRev, (CFStringRef)[dst lastPathComponent], NULL, 0);
err = FSCopyObjectSync(&srcRef, &dstDirRef, (CFStringRef)[dst lastPathComponent], NULL, 0);
if (err != noErr)
{
// We better move the old version back to its old location
if( hadFileAtDest )
FSMoveObjectSync(&movedRef, &dstDirRev, (CFStringRef)[dst lastPathComponent], &movedRef, 0);
FSMoveObjectSync(&movedRef, &dstDirRef, (CFStringRef)[dst lastPathComponent], &movedRef, 0);
if (error != NULL)
*error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUFileCopyFailure userInfo:[NSDictionary dictionaryWithObject:[NSString stringWithFormat:@"Couldn't copy %@ to %@.", src, dst] forKey:NSLocalizedDescriptionKey]];
return NO;
Expand Down

1 comment on commit e49a698

@danielpunkass
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uli, can you take a look at line 364 and 365 above and let me know if my reasoning for why this should be changed rings true for you? It looks like the case this is handling is where the target file does not yet exist, so for example if we are trying to copy into /Applications/MyApp.app.

The test then for write access is confirming write access for both:

"/Applications"

AND

"/"

It strikes me that in the case where there is no file already there, it should ONLY need to test for write access to the container folder for the file to be created. I suspect the redundant check on the second-level parent is a vestige of copying/modeling on the code that handles the case where the file exists and thus permission for both the file and the containing folder needs to be tested.

The downside to the code as it stands in this commit is if the user DOES have access to the container folder, but DOES NOT have access to the second-level container, Sparkle will erroneously prompt for admin privileges to complete the installation.

AMIRITE? :)

Please sign in to comment.