Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Lockbox.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
-(BOOL)archiveObject:(id<NSSecureCoding>)object forKey:(NSString *)key;
-(BOOL)archiveObject:(id<NSSecureCoding>)object forKey:(NSString *)key accessibility:(CFTypeRef)accessibility;

-(id)unarchiveObjectForKey:(NSString *)key;
-(id)unarchiveObjectForKey:(NSString *)key DEPRECATED_MSG_ATTRIBUTE("Migrate to -unarchiveObjectOfClass:forKey:");;
-(id)unarchiveObjectOfClass:(Class)cls forKey:(NSString *)key;
-(id)unarchiveObjectOfClasses:(NSSet*)clsSet forKey:(NSString *)key;

-(BOOL)setString:(NSString *)value forKey:(NSString *)key DEPRECATED_MSG_ATTRIBUTE("Migrate to -archiveObject:forKey:");
-(BOOL)setString:(NSString *)value forKey:(NSString *)key accessibility:(CFTypeRef)accessibility DEPRECATED_MSG_ATTRIBUTE("Migrate to -archiveObject:forKey:accesibility");
Expand Down Expand Up @@ -57,7 +59,9 @@
+(BOOL)archiveObject:(id<NSSecureCoding>)object forKey:(NSString *)key;
+(BOOL)archiveObject:(id<NSSecureCoding>)object forKey:(NSString *)key accessibility:(CFTypeRef)accessibility;

+(id)unarchiveObjectForKey:(NSString *)key;
+(id)unarchiveObjectForKey:(NSString *)key DEPRECATED_MSG_ATTRIBUTE("Migrate to +unarchiveObjectofClass:forKey:");
+(id)unarchiveObjectOfClass:(Class)cls forKey:(NSString *)key;
+(id)unarchiveObjectOfClasses:(NSSet*)clsSet forKey:(NSString *)key;

+(BOOL)setString:(NSString *)value forKey:(NSString *)key DEPRECATED_MSG_ATTRIBUTE("Migrate to +archiveObject:forKey:");
+(BOOL)setString:(NSString *)value forKey:(NSString *)key accessibility:(CFTypeRef)accessibility DEPRECATED_MSG_ATTRIBUTE("Migrate to +archiveObject:forKey:accesibility");
Expand Down
49 changes: 40 additions & 9 deletions Lockbox.m
Original file line number Diff line number Diff line change
Expand Up @@ -195,33 +195,54 @@ -(BOOL)archiveObject:(id<NSSecureCoding>)object forKey:(NSString *)key

-(BOOL)archiveObject:(id<NSSecureCoding>)object forKey:(NSString *)key accessibility:(CFTypeRef)accessibility
{
NSMutableData *data = [NSMutableData new];
NSKeyedArchiver *archiver = [[NSKeyedArchiver alloc] initForWritingWithMutableData:data];
[archiver encodeObject:object forKey:key];
[archiver finishEncoding];

NSError *error = nil;
NSData *data = [NSKeyedArchiver archivedDataWithRootObject:object
requiringSecureCoding:YES
error:&error];
if (!data) {
DLog(@"[archiveObject] Failed to archive object for key %@: %@", key, error);
return NO;
}
return [self setData:data forKey:key accessibility:accessibility];
}

-(id)unarchiveObjectForKey:(NSString *)key
{
return [self unarchiveObjectOfClass:[NSObject class] forKey:key];
Copy link
Owner

Choose a reason for hiding this comment

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

So it turns out that using [NSObject class] is not a great idea. Xcode surfaces the following runtime issue while running the tests in the sample project:

*** -[NSKeyedUnarchiver validateAllowedClass:forKey:]: NSSecureCoding allowed classes list contains [NSObject class], which bypasses security by allowing any Objective-C class to be implicitly decoded. Consider reducing the scope of allowed classes during decoding by listing only the classes you expect to decode, or a more specific base class than NSObject. This will become an error in the future. Allowed class list: {(
    "'NSObject' (0x7ff84324e1d0) [/Library/Developer/CoreSimulator/Volumes/iOS_22A3351/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 18.0.simruntime/Contents/Resources/RuntimeRoot/usr/lib]"
)}

So basically, this circumvents secure coding best practices.

I need to think about whether accepting this pull request is a good idea with respect to the intended best practices of the newer APIs. You might consider a newer alternative library, Strongbox. It's a Swift implementation of Lockbox that relies on NSSecureCoding protocol conformance to ensure the new APIs can be used.

Copy link
Author

Choose a reason for hiding this comment

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

So using NSObject here is the equivalent of using the old [unarchiver decodeObjectForKey:key]; on old line 215 new line 223 that master branch is using at least form a security perspective. I suspect Apple, when they do disable their functionality, will do so at the same time. Personally I think this is better, because now the developer will get both a compile time and runtime warning that they need to upgrade, where as before it was only a compile time warning. If you want, we can make our compile time deprecation warning a little stronger.
But ultimately the decision is up to you.

}

-(id)unarchiveObjectOfClass:(Class)cls forKey:(NSString *)key
{
NSSet *allowedClass = [NSSet setWithObject:cls];
return [self unarchiveObjectOfClasses:allowedClass
forKey:key];
}

-(id)unarchiveObjectOfClasses:(NSSet*)clsSet forKey:(NSString *)key
{
NSData *data = [self dataForKey:key];
NSError *error = nil;
if (!data)
return nil;

id object = nil;
@try {
NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingWithData:data];
object = [unarchiver decodeObjectForKey:key];
object = [NSKeyedUnarchiver unarchivedObjectOfClasses:clsSet
fromData:data
error:&error];
if (!object && error) {
DLog(@"Failed to unarchive object for key %@: %@", key, error);
}
}
@catch (NSException *exception) {
DLog(@"failed for key %@: %@", key, exception.description);
}

return object;
}



-(BOOL)setString:(NSString *)value forKey:(NSString *)key
{
return [self setString:value forKey:key accessibility:DEFAULT_ACCESSIBILITY];
Expand Down Expand Up @@ -349,7 +370,17 @@ +(BOOL)archiveObject:(id<NSSecureCoding>)object forKey:(NSString *)key accessibi

+(id)unarchiveObjectForKey:(NSString *)key
{
return [_lockBox unarchiveObjectForKey:key];
return [_lockBox unarchiveObjectOfClass:[NSObject class] forKey:key];
}

+(id)unarchiveObjectOfClass:(Class)cls forKey:(NSString *)key
{
return [_lockBox unarchiveObjectOfClass:(Class)cls forKey:key];
}

+(id)unarchiveObjectOfClasses:(NSSet*)clsSet forKey:(NSString *)key
{
return [_lockBox unarchiveObjectOfClasses:clsSet forKey:key];
}

+(BOOL)setString:(NSString *)value forKey:(NSString *)key
Expand Down