Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable mounting encrypted volumes #18

Merged
merged 8 commits into from
Jan 22, 2018
Merged

Conversation

acecilia
Copy link
Contributor

@acecilia acecilia commented Jan 11, 2018

Fixes #17

@acecilia acecilia changed the title Enable mounting encrypted volumes (Fix https://github.com/kainjow/Semulov/issues/17) Enable mounting encrypted volumes Jan 11, 2018
@acecilia
Copy link
Contributor Author

acecilia commented Jan 12, 2018

Another alternative I consider (in case it is possible) is mount the encrypted disk using the same method that diskUtility uses. After some investigation seems that the popup where you enter the password, is being presented by the process SecurityAgent using the plugin DiskUnlock.bundle (found under /System/Library/CoreServices/SecurityAgentPlugins/DiskUnlock.bundle).

captura de pantalla 2018-01-12 a las 2 35 16

Apple has documentation about this here. I will be looking into it when I have some time.

My guess is that it may be possible to call SecurityAgent->DiskUnlock.bundle from Semulov, and pass it the disk identifier, so it prompts the user for the password or gets the key from the keychain + unlocks and mounts the disk.

@richardmalone
Copy link

acecilia,

That is awesome! Thank you so much, that works for me. BTW, I had to link the Security.framework to get this to build.

Great work.

@acecilia
Copy link
Contributor Author

@richardmalone fixed linking of security.framework

@acecilia
Copy link
Contributor Author

Continuing with the idea of using the DiskUnlock.bundle to unlock the disk.

Using the following code:

AuthorizationRef authRef;
AuthorizationItem authItem;
AuthorizationRights authRights;
AuthorizationFlags authFlags;

OSStatus authStatus;

// Create empty authorization
printf("awaiting: AuthorizationCreate\n");
authStatus = AuthorizationCreate(NULL, kAuthorizationEmptyEnvironment, kAuthorizationFlagDefaults, &authRef);
printf("AuthorizationCreate // authStatus == %d\n", authStatus);

// Create a new authorization proces
authItem.name = "system.disk.unlock";
authItem.valueLength = 0;
authItem.value = NULL;
authItem.flags = 0;

authRights.count = 1;
authRights.items = &authItem;

authFlags = kAuthorizationFlagDefaults | kAuthorizationFlagInteractionAllowed; 

// Get new credentials
printf("awaiting: AuthorizationCopyRights\n");
authStatus = AuthorizationCopyRights(authRef, &authRights, kAuthorizationEmptyEnvironment, authFlags, NULL);
printf("AuthorizationCopyRights // authStatus == %d\n", authStatus);

printf("DONE");

I managed to invoke the window prompt form the DiskUnlock.bundle:

captura de pantalla 2018-01-14 a las 1 49 48

But the authorization finally fails:

captura de pantalla 2018-01-14 a las 1 49 55

My guess is that it fails because it requires the UUID of the disk to mount, but I am not sure how that information should be passed to the DiskUnlock.bundle plugin. I could not find any documentation about this plugin and which data it needs, plus the general documentation about authentication plugins is very limited. Any help is appreciated.

Copy link
Owner

@kainjow kainjow left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

SLDiskManager.m Outdated
disk.encrypted = [volumeType rangeOfString:@"encrypt"].location == NSNotFound;
CFUUIDRef uuidRef = (__bridge CFUUIDRef)[description objectForKey:(NSString *)kDADiskDescriptionVolumeUUIDKey];
if (uuidRef) {
disk.volumeUUID = (__bridge NSString *)CFUUIDCreateString(NULL, uuidRef);
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be __bridge_transfer__.

SLDiskManager.m Outdated
[task launch];
} else {
NSAlert *alert = [[NSAlert alloc] init];
[alert setMessageText: [NSString stringWithFormat:@"Error: %@ \n\nRemember that in order to be able to mount an encrypted volume, previously you have to: \na) Mount it using Disk Utility\nb) Save the password in the keychain", SecCopyErrorMessageString(status, NULL)]];
Copy link
Owner

Choose a reason for hiding this comment

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

Use NSLocalizedString() here.

Also SecCopyErrorMessageString() result is leaked, needs __bridge_transfer__ here.

SLDiskManager.m Outdated
[alert setMessageText: [NSString stringWithFormat:@"Error: %@ \n\nRemember that in order to be able to mount an encrypted volume, previously you have to: \na) Mount it using Disk Utility\nb) Save the password in the keychain", SecCopyErrorMessageString(status, NULL)]];
[alert runModal];

NSLog(@"Failed to fetch the volume encryption password from the keychain: %@", SecCopyErrorMessageString(status, NULL));
Copy link
Owner

@kainjow kainjow Jan 14, 2018

Choose a reason for hiding this comment

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

Same as above with the leak. IMO create a var that's bridge_retained and just use that?

SLDiskManager.m Outdated
if (pwData) SecKeychainItemFreeContent(NULL, pwData); // Free memory
} else {
DADiskRef diskref = DADiskCreateFromBSDName(kCFAllocatorDefault, _session, [disk.diskID UTF8String]);
DADiskMount(diskref, NULL, kDADiskMountOptionDefault, NULL, NULL);
Copy link
Owner

Choose a reason for hiding this comment

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

Only do this and the CFRelease if diskref is not NULL, otherwise could potentially crash. I just realized the original code was doing this but incorrectly checked disk instead of diskref.

@acecilia
Copy link
Contributor Author

@kainjow done. Thanks for the feedback.
Still working on this, do not merge it yet :) I would like to implement it using DiskUnlock.bundle, or if that is not possible, prompt the user for the password in case it is not in the keychain.

…not found in the keychain

Added the hability to check if the mounting was successful, in case the introduced password is wrong
@acecilia
Copy link
Contributor Author

@kainjow You can merge it, I am not going to have much time to work on the DiskUnlock.bundle approach this days.

SLDiskManager.m Outdated
[alert runModal];
NSLog(@"Error when fetching the encryption password from the keychain: %@ Prompting the user for it.", (__bridge_transfer NSString *)SecCopyErrorMessageString(status, NULL));

NSAlert *alert = [NSAlert alertWithMessageText: [NSString stringWithFormat: NSLocalizedString(@"Introduce the encryption password to unlock the volume once:", nil), (__bridge_transfer NSString *)SecCopyErrorMessageString(status, NULL)] defaultButton: NSLocalizedString(@"Ok", nil) alternateButton: NSLocalizedString(@"Cancel", nil) otherButton: nil informativeTextWithFormat: @"If you want to mount an encrypted volume automatically without manually introducing the password: \na) Mount it using Disk Utility\nb) Save the password in the keychain"];
Copy link
Owner

Choose a reason for hiding this comment

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

Use NSLocalizedString for informativeTextWithFormat param.

SLDiskManager.m Outdated

// In case return code is not zero, there was an error
if ([task terminationStatus]) {
NSAlert *alert = [NSAlert alertWithMessageText: @"There was an error unlocking the disk" defaultButton: @"Ok" alternateButton: nil otherButton: nil informativeTextWithFormat: @"Check that you introduced the correct password"];
Copy link
Owner

Choose a reason for hiding this comment

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

NSLocalizedString here for UI text as well.

SLDiskManager.m Outdated

// In case return code is not zero, there was an error
if ([task terminationStatus]) {
NSAlert *alert = [NSAlert alertWithMessageText:@"There was an error unlocking the disk" defaultButton:@"Ok" alternateButton:nil otherButton:nil informativeTextWithFormat:@"Check that you introduced the correct password"];
Copy link
Owner

Choose a reason for hiding this comment

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

Don't use "Ok", that's non-standard. Try nil. If that doesn't work, use standard NSAlert alloc init routine and set message text and info and then don't set the button text, which defaults to "OK" for English. Basically, provide less so to use defaults when possible. Also, I'd rather have more lines that are shorter then a big long line that's harder to read/edit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nil worked :)

@kainjow kainjow merged commit a0b1bf7 into kainjow:master Jan 22, 2018
@richardmalone
Copy link

Thanks to you both for all your efforts getting this feature included into Semulov. Really useful piece of software - even better now. I owe you both a beer!

@@ -152,6 +152,13 @@ - (void)updateDisk:(SLDisk *)disk fromDescription:(NSDictionary *)description
disk.deviceName = [disk.diskImage lastPathComponent];
}
}
disk.volumeKind = [description objectForKey:(NSString *)kDADiskDescriptionVolumeKindKey];
NSString* volumeType = [description objectForKey:(NSString *)kDADiskDescriptionVolumeTypeKey];
disk.encrypted = [volumeType rangeOfString:@"encrypt"].location == NSNotFound;
Copy link
Owner

Choose a reason for hiding this comment

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

@acecilia shouldn't this be the opposite? Currently it's set to encrypted if "encrypted" Is not found..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a bug, yes. Your fix in master looks good though, tested on my machine and now all encrypted disks are identified correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants