Skip to content

Commit

Permalink
Cherry-pick 80c45a1. rdar://113527046
Browse files Browse the repository at this point in the history
    Fix crashes from hardening _WKRemoteObjectRegistry decoding
    https://bugs.webkit.org/show_bug.cgi?id=262983
    rdar://113527046

    Reviewed by David Kilzer.

    In April 2023, I made _WKRemoteObjectRegistry deserialization stricter by not allowing subclasses
    of the specified class to be deserialized.  To make this transition as smooth as possible, I added
    a set of common and safe always-allowed subclasses including NSMutableString and several others.
    Telemetry indicates this increased the crash rate and this is to bring that crash rate back down.

    After analyzing 100% of the recent crash reports with useful data, I found 4 things that will help:
    1. Allowing a class named "NSDecimalNumberPlaceholder" which is a subclass of NSDecimalNumber that
       some internal frameworks apparently use and give to Safari in a path that serializes it.  Allow this.
    2. Some crash logs indicate that NSDate objects are failing to decode because they are not in the
       set of allowed classes.  There are some JS to ObjC object converters (such as the one used in
       WKWebView.callAsyncJavaScript) that can produce an NSDate, and this is ok and safe.  Allow this too.
    3. There are logs that indicate that sometimes a class is being sent from one process to another,
       and the receiving process has not loaded the dylib containing the ObjC class so the ObjC runtime
       can't find it.  Add telemetry to get this class name for future diagnosis.
    4. There are logs that indicate that sometimes a class is being sent that does not conform to
       NSSecureCoding according to NSCoder.  Also add telemetry to get this class name for future diagnosis,
       but in this case I needed to add a @try/@catch because validateClassSupportsSecureCoding either
       returns YES or throws an ObjC exception, so to get the class name to CRASH_WITH_INFO I need to catch.

    * Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:
    (alwaysAllowedClasses):
    (validateClass):
    (decodeObject):

    Canonical link: https://commits.webkit.org/269185@main
Identifier: 267815.308@safari-7617.1.11.11-branch
  • Loading branch information
achristensen07 authored and MyahCobbs committed Oct 16, 2023
1 parent 6e09d12 commit a95254e
Showing 1 changed file with 23 additions and 7 deletions.
30 changes: 23 additions & 7 deletions Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm
Original file line number Diff line number Diff line change
Expand Up @@ -799,10 +799,25 @@ static id decodeObjectFromObjectStream(WKRemoteObjectDecoder *decoder, const Has
(__bridge CFTypeRef)NSUUID.class,
(__bridge CFTypeRef)NSError.class,
(__bridge CFTypeRef)NSURLError.class,
(__bridge CFTypeRef)NSDate.class,
(__bridge CFTypeRef)NSDecimalNumber.class,
(__bridge CFTypeRef)NSClassFromString(@"NSDecimalNumberPlaceholder"),
} };
return classes.get();
}

NO_RETURN static void crashWithClassName(const char* className)
{
std::array<uint64_t, 6> values { 0, 0, 0, 0, 0, 0 };
strncpy(reinterpret_cast<char*>(values.data()), className, sizeof(values));
CRASH_WITH_INFO(values[0], values[1], values[2], values[3], values[4], values[5]);
}

NO_RETURN static void crashWithClassName(Class objectClass)
{
crashWithClassName(NSStringFromClass(objectClass).UTF8String);
}

static void checkIfClassIsAllowed(WKRemoteObjectDecoder *decoder, Class objectClass)
{
auto* allowedClasses = decoder->_allowedClasses;
Expand All @@ -815,10 +830,7 @@ static void checkIfClassIsAllowed(WKRemoteObjectDecoder *decoder, Class objectCl
if (alwaysAllowedClasses().contains((__bridge CFTypeRef)objectClass))
return;

NSString *message = [NSString stringWithFormat:@"%@,%@", NSStringFromClass(objectClass), decoder.allowedClasses];
std::array<uint64_t, 6> values { 0, 0, 0, 0, 0, 0 };
strncpy(reinterpret_cast<char*>(values.data()), message.UTF8String, sizeof(values));
CRASH_WITH_INFO(values[0], values[1], values[2], values[3], values[4], values[5]);
crashWithClassName(objectClass);
}

static void validateClass(WKRemoteObjectDecoder *decoder, Class objectClass)
Expand All @@ -831,8 +843,12 @@ static void validateClass(WKRemoteObjectDecoder *decoder, Class objectClass)
if (objectClass == [NSInvocation class] || objectClass == [NSBlockInvocation class])
return;

if (![decoder validateClassSupportsSecureCoding:objectClass])
[NSException raise:NSInvalidUnarchiveOperationException format:@"Object of class \"%@\" does not support NSSecureCoding.", objectClass];
@try {
if (![decoder validateClassSupportsSecureCoding:objectClass])
[NSException raise:NSInvalidUnarchiveOperationException format:@"Object of class \"%@\" does not support NSSecureCoding.", objectClass];
} @catch(NSException *exception) {
crashWithClassName(objectClass);
}
}

static void decodeInvocationArguments(WKRemoteObjectDecoder *decoder, NSInvocation *invocation, const Vector<HashSet<CFTypeRef>>& allowedArgumentClasses, NSUInteger firstArgument)
Expand Down Expand Up @@ -1034,7 +1050,7 @@ static id decodeObject(WKRemoteObjectDecoder *decoder)

Class objectClass = objc_lookUpClass(className.data());
if (!objectClass)
[NSException raise:NSInvalidUnarchiveOperationException format:@"Class \"%s\" does not exist", className.data()];
crashWithClassName(className.data());

validateClass(decoder, objectClass);

Expand Down

0 comments on commit a95254e

Please sign in to comment.