Date
1 - 9 of 9
Verbose warning splurged to the log with Secure Coding (dearchiving) -- can they be serious?
Graham Cox
Hi all,
I was working on some demo code investigating the use of secure archiving, since I guess we all have to adopt that if we can. I have a simple container class (a linked list) that I decided to make secure archivable. It works, but when dearchiving, I need to dearchive the ‘payload’ of the list nodes, which can be anything at all - it’s defined as an id<NSObject>. So when I ask for this item securely, I use
-decodeObjectOfClass:[NSObject class] forKey:<blah blah>
But the dearchiver spits this to the log:
2022-02-24 17:59:55.701579+1100 GCSimpleContainers[25591:31009796] [general] *** -[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' (0x7ff85c7c2b88) [/usr/lib]"
)}
I get what it’s saying — that asking for NSObject is too open-ended, and it renders the secure coding moot. But what else can I do? I don’t know the payload class, I want it to be as flexible as possible. Forcing this to be a subclass of NSObject of my own means I lose the ability to store all standard Cocoa objects ‘as is’ in my container. Adding a wrapper class doesn’t help because it would still need to archive what it wrapped. It strikes me that Apple themselves must run into this when archiving NSDictionary, since keys can be anything, but that works, so perhaps they use an internal bit of trickery.
It concerns me that they say “this will become an error in the future”. I can go back to traditional archiving with no security, no errors there, but then what’s the point of secure coding?
A way to simply suppress the log of the message would do for now. It’s extra bad because it logs this every time it happens, not just once. Is this a case of Apple being too zealous in overlooking a common use-case, or have I missed something?
—G
Glenn L. Austin
On Feb 23, 2022, at 11:30 PM, Graham Cox <graham@...> wrote:Hi all,I was working on some demo code investigating the use of secure archiving, since I guess we all have to adopt that if we can. I have a simple container class (a linked list) that I decided to make secure archivable. It works, but when dearchiving, I need to dearchive the ‘payload’ of the list nodes, which can be anything at all - it’s defined as an id<NSObject>. So when I ask for this item securely, I use-decodeObjectOfClass:[NSObject class] forKey:<blah blah>
A suggestion? Archive the class name as a part of your archiving process, then get the class instance by NSClassFromString. You can then use that as the parameter for decodeObjectOfClass:forKey:.
It's not entirely secure, it's not the best style to avoid the warning, but it should prevent the spew into the system log. Also, if you can't get the class from the class name, you know you'll not be able to decode the value anyway.
Graham Cox
Hi Glenn,
toggle quoted message
Show quoted text
This seemed like a perfectly fine idea in this case, so I tried it.
Now I get a new message spewed to the log:
2022-02-24 20:50:13.158117+1100 GCSimpleContainers[26354:31119213] [general] *** -[NSKeyedUnarchiver validateAllowedClass:forKey:] allowed unarchiving safe plist type ''NSString' (0x7ff85c670920) [/System/Library/Frameworks/Foundation.framework]' for key 'data', even though it was not explicitly included in the client allowed classes set: '{(
"'__NSCFConstantString' (0x7ff85c645908) [/System/Library/Frameworks/CoreFoundation.framework]"
)}'. This will be disallowed in the future.
This happens for the line that decodes the payload object, which happens to be a string. My code is:
Class dataClass = NSClassFromString([coder decodeObjectOfClass:[NSString class] forKey:@"data_class"]); //<—— this is OK
self.data = [coder decodeObjectOfClass:dataClass forKey:@"data”]; //<— this is not
The actual class is __NSCFConstantString, rather than just a generic NSString, that seems to be what it is complaining about — if I change it to use [NSString class] it’s fine.
Since the archive originally wrote the result of NSStringFromClass(), it will of course write the explicit class rather than the more general umbrella class of a cluster. Can you think of a simple solution for this, in general?
—G.
On 24 Feb 2022, at 6:44 pm, Glenn L. Austin <glenn@...> wrote:On Feb 23, 2022, at 11:30 PM, Graham Cox <graham@...> wrote:Hi all,I was working on some demo code investigating the use of secure archiving, since I guess we all have to adopt that if we can. I have a simple container class (a linked list) that I decided to make secure archivable. It works, but when dearchiving, I need to dearchive the ‘payload’ of the list nodes, which can be anything at all - it’s defined as an id<NSObject>. So when I ask for this item securely, I use-decodeObjectOfClass:[NSObject class] forKey:<blah blah>A suggestion? Archive the class name as a part of your archiving process, then get the class instance by NSClassFromString. You can then use that as the parameter for decodeObjectOfClass:forKey:.It's not entirely secure, it's not the best style to avoid the warning, but it should prevent the spew into the system log. Also, if you can't get the class from the class name, you know you'll not be able to decode the value anyway.
Graham Cox
Well, I figured out a solution, though I’m not sure — it seems slightly smelly, even though it looks neat from the outside and is easy to use. Would appreciate any feedback.
toggle quoted message
Show quoted text
Basically, I walk up the class hierarchy until I hit NSObject, then return the class once removed from that. Then I added a category on NSCoder that uses this to encode that class name in the archive. Using these methods in NSCoding is easy and hides the minor pain involved.
Class GCLowestSpecificAncestorClass( Class aClass )
{
// walks back up the class hierarchy from <aClass> until it reaches NSObject (or nil), and returns the
// class that subclasses NSObject.
Class sc = [aClass superclass];
while( sc && sc != [NSObject class])
{
aClass = sc;
sc = [aClass superclass];
}
return aClass;
}
@implementation NSCoder (GCSecureAnonymousClassSupport)
- (void) encodeAnonymousObject:(id) theObject forKey:(NSString*) key
{
Class umbrella = GCLowestSpecificAncestorClass([theObject class]);
NSAssert( umbrella != [NSObject class], @"cannot archive NSObject as anonymous class");
NSString* className = NSStringFromClass( umbrella );
[self encodeObject:className forKey:[NSString stringWithFormat:@"class_for_%@", key]];
[self encodeObject:theObject forKey:key];
}
- (id) decodeAnonymousObjectForKey:(NSString*) key
{
NSString* className = [self decodeObjectOfClass:[NSString class] forKey:[NSString stringWithFormat:@"class_for_%@", key]];
Class umbrella = NSClassFromString( className );
return [self decodeObjectOfClass:umbrella forKey:key];
}
@end
On 24 Feb 2022, at 9:07 pm, Graham Cox <graham@...> wrote:Hi Glenn,This seemed like a perfectly fine idea in this case, so I tried it.Now I get a new message spewed to the log:2022-02-24 20:50:13.158117+1100 GCSimpleContainers[26354:31119213] [general] *** -[NSKeyedUnarchiver validateAllowedClass:forKey:] allowed unarchiving safe plist type ''NSString' (0x7ff85c670920) [/System/Library/Frameworks/Foundation.framework]' for key 'data', even though it was not explicitly included in the client allowed classes set: '{("'__NSCFConstantString' (0x7ff85c645908) [/System/Library/Frameworks/CoreFoundation.framework]")}'. This will be disallowed in the future.This happens for the line that decodes the payload object, which happens to be a string. My code is:Class dataClass = NSClassFromString([coder decodeObjectOfClass:[NSString class] forKey:@"data_class"]); //<—— this is OKself.data = [coder decodeObjectOfClass:dataClass forKey:@"data”]; //<— this is notThe actual class is __NSCFConstantString, rather than just a generic NSString, that seems to be what it is complaining about — if I change it to use [NSString class] it’s fine.Since the archive originally wrote the result of NSStringFromClass(), it will of course write the explicit class rather than the more general umbrella class of a cluster. Can you think of a simple solution for this, in general?—G.On 24 Feb 2022, at 6:44 pm, Glenn L. Austin <glenn@...> wrote:On Feb 23, 2022, at 11:30 PM, Graham Cox <graham@...> wrote:Hi all,I was working on some demo code investigating the use of secure archiving, since I guess we all have to adopt that if we can. I have a simple container class (a linked list) that I decided to make secure archivable. It works, but when dearchiving, I need to dearchive the ‘payload’ of the list nodes, which can be anything at all - it’s defined as an id<NSObject>. So when I ask for this item securely, I use-decodeObjectOfClass:[NSObject class] forKey:<blah blah>A suggestion? Archive the class name as a part of your archiving process, then get the class instance by NSClassFromString. You can then use that as the parameter for decodeObjectOfClass:forKey:.It's not entirely secure, it's not the best style to avoid the warning, but it should prevent the spew into the system log. Also, if you can't get the class from the class name, you know you'll not be able to decode the value anyway.
On 24 Feb 2022, at 2:07 am, Graham Cox <graham@...> wrote:In respect of this particular issue, I think if you call `classForCoder` rather than `class` on the object of interest, you'll get the portable class name ("NSString") you're looking for.
The actual class is __NSCFConstantString, rather than just a generic NSString, that seems to be what it is complaining about — if I change it to use [NSString class] it’s fine.
Since the archive originally wrote the result of NSStringFromClass(), it will of course write the explicit class rather than the more general umbrella class of a cluster. Can you think of a simple solution for this, in general?
-ben
Quincey Morris
I think you’re Doing It Wrong™. Your solution is effectively disabling secure coding, which solves your run-time error, but leaves you with the exact security vulnerability that secure coding is supposed to protect against.
Your goal of “I want it to be as flexible as possible” without specifying classes in advance isn’t compatible with secure decoding.
Let’s stick with your NSDictionary example. There are 3 cases:
1. You (try to) decode the dictionary and specify class NSDictionary. This is insecure, because decoding the dictionary also decodes arbitrary classes contained in the dictionary.
2. The dictionary contains only objects of “plist” classes such as NSNumber, NSDate and standard NS collection classes. There are a finite number of these, so you can always list them all in `decodeObjectOfClasses:forKey:`, and this kind of dictionary is useful for a lot of things without getting into custom classes.
3. The dictionary contains your custom classes and/or client code's custom classes as well. Again, you must use `decodeObjectOfClasses:forKey:` to specify *all* of the classes that might be in the dictionary, but you don’t know them all at compile time.
You can’t do #1, so you must do #2 or #3. Either way, use `decodeObjectOfClasses:forKey:` for a heterogenous collection or hierarchy.
Note that your set of classes for `decodeObjectOfClasses:forKey:` can be built at run-time, but you should avoid the temptation to encode that set into the archive, since that also compromises security. (An attacker can simply ask your code to unarchive data containing malicious classes, and naming their classes!)
If you want to make things flexible for client code, I guess the way to do it would be to have the client tell you, on unarchiving, the list of its custom classes that it allows in the archive. You’d augment that set with the additional classes your own wrapper code needs, and specify that for `decodeObjectOfClasses:forKey:`.
On Feb 24, 2022, at 03:35, Graham Cox <graham@...> wrote:Well, I figured out a solution, though I’m not sure — it seems slightly smelly, even though it looks neat from the outside and is easy to use. Would appreciate any feedback.Basically, I walk up the class hierarchy until I hit NSObject, then return the class once removed from that. Then I added a category on NSCoder that uses this to encode that class name in the archive. Using these methods in NSCoding is easy and hides the minor pain involved.
Graham Cox
This sounds so right. But unfortunately doesn’t work for __NSCFConstantString, which returns that, and not NSString. I haven’t yet tried on other classes, but most of the time it’ll be a string...
toggle quoted message
Show quoted text
In respect of this particular issue, I think if you call `classForCoder` rather than `class` on the object of interest, you'll get the portable class name ("NSString") you're looking for.
-ben
Graham Cox
On 25 Feb 2022, at 12:55 pm, Quincey Morris <quinceymorris@...> wrote:I think you’re Doing It Wrong™. Your solution is effectively disabling secure coding, which solves your run-time error, but leaves you with the exact security vulnerability that secure coding is supposed to protect against.
Yes, I understand that writing the class name into the archive bypasses what secure coding is supposed to accomplish, though it adds a small extra hoop to jump through for a would-be malfeasant.
However, I’ve never quite understood how such an attack could succeed anyway — sure, you could theoretically hack an archive to substitute a different class, but that still isn't loading any code, which must be already linked to your app. Given other safeguards such as code signing, etc, I can’t see how that’s possible. If the class doesn’t exist already in your app, dearchiving fails. Does secure coding actually solve any known type of attack?
Nevertheless, I’d prefer to avoid this “solution” for the reason you state — that’s why I said it seems slightly smelly.
Obfuscating or encrypting the classname in the archive might be a possibility.
Your goal of “I want it to be as flexible as possible” without specifying classes in advance isn’t compatible with secure decoding.Let’s stick with your NSDictionary example. There are 3 cases:1. You (try to) decode the dictionary and specify class NSDictionary. This is insecure, because decoding the dictionary also decodes arbitrary classes contained in the dictionary.
But dictionaries could be embedded deep inside some other objects that gets archived, even if you don’t know that. NSDictionary allows any kind of object (conforming to NSCopying) to be a key, so my thinking is that Apple must run into this exact same issue themselves for the secure archiving of NSDictionary. And yet they appear not to (I’ve not exhaustively tested this, but I can experiment further), suggesting that for their own classes they’re doing something privileged that we aren’t allowed to.
2. The dictionary contains only objects of “plist” classes such as NSNumber, NSDate and standard NS collection classes. There are a finite number of these, so you can always list them all in `decodeObjectOfClasses:forKey:`, and this kind of dictionary is useful for a lot of things without getting into custom classes.
Limiting keys to be plist types may be OK, without making it too inflexible. Most of the time strings are used. But limiting payload classes to plist types is a bigger limitation, since these are much more likely to be custom classes defined by client code.
3. The dictionary contains your custom classes and/or client code's custom classes as well. Again, you must use `decodeObjectOfClasses:forKey:` to specify *all* of the classes that might be in the dictionary, but you don’t know them all at compile time.You can’t do #1, so you must do #2 or #3. Either way, use `decodeObjectOfClasses:forKey:` for a heterogenous collection or hierarchy.Note that your set of classes for `decodeObjectOfClasses:forKey:` can be built at run-time, but you should avoid the temptation to encode that set into the archive, since that also compromises security. (An attacker can simply ask your code to unarchive data containing malicious classes, and naming their classes!)
OK, I might look into building such a set, but that would require that there is some use of the class before any dearchiving, so that the actual classes used for keys, etc can be seen and added to the set. (Other than common types which I can establish in advance). However, dearchiving could well be the first use of the class, if your app launches and opens a document. The set could be saved in preferences I guess, so it’s persistent over time… I dunno, seems pretty smelly again.
If you want to make things flexible for client code, I guess the way to do it would be to have the client tell you, on unarchiving, the list of its custom classes that it allows in the archive. You’d augment that set with the additional classes your own wrapper code needs, and specify that for `decodeObjectOfClasses:forKey:`.
I can’t see this being practical, for a basic container class that could live deep within another class. If the client code wants to dearchive an object graph containing this private stuff deep inside, they are just not going to know what classes are in use internally. They likely don’t even have a public interface. Certainly this is OK for where I build one class into another, and I’m already doing that in a couple of places.
Anyway, I’m still thinking about this, and I greatly appreciate the input Quincey.
—G
Quincey Morris
On Feb 24, 2022, at 19:14, Graham Cox <graham@...> wrote:
On 25 Feb 2022, at 12:55 pm, Quincey Morris <quinceymorris@...> wrote:I think you’re Doing It Wrong™. Your solution is effectively disabling secure coding, which solves your run-time error, but leaves you with the exact security vulnerability that secure coding is supposed to protect against.Yes, I understand that writing the class name into the archive bypasses what secure coding is supposed to accomplish, though it adds a small extra hoop to jump through for a would-be malfeasant.However, I’ve never quite understood how such an attack could succeed anyway — sure, you could theoretically hack an archive to substitute a different class, but that still isn't loading any code, which must be already linked to your app. Given other safeguards such as code signing, etc, I can’t see how that’s possible. If the class doesn’t exist already in your app, dearchiving fails. Does secure coding actually solve any known type of attack?
I don’t know an exact scenario, but hacking a class name in an archive is a trivial way of substituting a malicious class type for any other type. It will negate whatever protections secure coding gives you.
Nevertheless, I’d prefer to avoid this “solution” for the reason you state — that’s why I said it seems slightly smelly.Obfuscating or encrypting the classname in the archive might be a possibility.Your goal of “I want it to be as flexible as possible” without specifying classes in advance isn’t compatible with secure decoding.Let’s stick with your NSDictionary example. There are 3 cases:1. You (try to) decode the dictionary and specify class NSDictionary. This is insecure, because decoding the dictionary also decodes arbitrary classes contained in the dictionary.But dictionaries could be embedded deep inside some other objects that gets archived, even if you don’t know that. NSDictionary allows any kind of object (conforming to NSCopying) to be a key, so my thinking is that Apple must run into this exact same issue themselves for the secure archiving of NSDictionary. And yet they appear not to (I’ve not exhaustively tested this, but I can experiment further), suggesting that for their own classes they’re doing something privileged that we aren’t allowed to.
I’m not sure I understand your concern exactly. When you say `decodeObjectOfClasses`, you’re starting a hierarchy/graph of decodes from a root object, because each object has to decode its own properties (or whatever sub-objects it’s chosen to represent itself). The entire hierarchy/graph of decodes is constrained by the classes you specified at the top level, so nothing escapes the validation.
2. The dictionary contains only objects of “plist” classes such as NSNumber, NSDate and standard NS collection classes. There are a finite number of these, so you can always list them all in `decodeObjectOfClasses:forKey:`, and this kind of dictionary is useful for a lot of things without getting into custom classes.Limiting keys to be plist types may be OK, without making it too inflexible. Most of the time strings are used. But limiting payload classes to plist types is a bigger limitation, since these are much more likely to be custom classes defined by client code.3. The dictionary contains your custom classes and/or client code's custom classes as well. Again, you must use `decodeObjectOfClasses:forKey:` to specify *all* of the classes that might be in the dictionary, but you don’t know them all at compile time.You can’t do #1, so you must do #2 or #3. Either way, use `decodeObjectOfClasses:forKey:` for a heterogenous collection or hierarchy.Note that your set of classes for `decodeObjectOfClasses:forKey:` can be built at run-time, but you should avoid the temptation to encode that set into the archive, since that also compromises security. (An attacker can simply ask your code to unarchive data containing malicious classes, and naming their classes!)OK, I might look into building such a set, but that would require that there is some use of the class before any dearchiving, so that the actual classes used for keys, etc can be seen and added to the set. (Other than common types which I can establish in advance). However, dearchiving could well be the first use of the class, if your app launches and opens a document. The set could be saved in preferences I guess, so it’s persistent over time… I dunno, seems pretty smelly again.If you want to make things flexible for client code, I guess the way to do it would be to have the client tell you, on unarchiving, the list of its custom classes that it allows in the archive. You’d augment that set with the additional classes your own wrapper code needs, and specify that for `decodeObjectOfClasses:forKey:`.I can’t see this being practical, for a basic container class that could live deep within another class. If the client code wants to dearchive an object graph containing this private stuff deep inside, they are just not going to know what classes are in use internally. They likely don’t even have a public interface. Certainly this is OK for where I build one class into another, and I’m already doing that in a couple of places.
What I had in mind, assuming your code is doing the unarchiving for client code that has its own classes in the object graph, is that the client code would just supply you with a list of its classes once (during startup, or via some method or delegate that’s invoked just before decoding starts). You’d add those into the set of classes that your code uses, and that’s what you would pass to `decodeObjectOfClasses:forKey:`.
Or, it could work the other way round if the client code does the unarchiving of an object graph that includes your private objects. It would ask you, or you would volunteer, a set of classes before decoding starts, and that would allow your classes to decode securely. The client doesn’t need to know *where* your objects are in the object graph.
As I said before, this set is applied once at the top level of decoding. It doesn’t matter where in the object graph the classes are actually used.
Anyway, I’m still thinking about this, and I greatly appreciate the input Quincey.—G