Bug 199584

Summary: Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod]
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, beidson, commit-queue, ggaren, mitz, mjs, sam, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=199629
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2019-07-08 12:15:12 PDT
Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod] for robustness.
Comment 1 Chris Dumez 2019-07-08 12:15:26 PDT
<rdar://problem/46268249>
Comment 2 Chris Dumez 2019-07-08 12:18:24 PDT
Created attachment 373655 [details]
Patch
Comment 3 Maciej Stachowiak 2019-07-08 20:13:22 PDT
Comment on attachment 373655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373655&action=review

> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:195
> +    // The block should return a void.
> +    if (strcmp(wireBlockSignature.methodReturnType, "v"))
> +        return false;
> +
> +    // First implicit argument of a block is always the block itself.
> +    if (wireBlockSignature.numberOfArguments < 1 || strcmp([wireBlockSignature getArgumentTypeAtIndex:0], "@?"))
> +        return false;

Can anything else be wrong? Is there a need to compare the remote signature to the local one for instance?

> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:238
> +            if (!validateReplyBlockSignature(wireBlockSignature)) {
> +                NSLog(@"_invokeMethod: Failed to validate reply block signature: %@", wireBlockSignature._typeString);

Perhaps we should kill the WebContent process when this happens instead of just logging? Seems equivalent to an ill-formed CoreIPC message.
Comment 4 Chris Dumez 2019-07-09 08:35:59 PDT
Comment on attachment 373655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373655&action=review

>> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:195
>> +        return false;
> 
> Can anything else be wrong? Is there a need to compare the remote signature to the local one for instance?

Ideally, we would want to do more validation but I have not found a suitable way to do it yet.

As far as I can tell, the process is as follows:
1. Process A calls method M on Object O that takes a completionHandler
2. We send an IPC from Process A to Process B asking to invoke method M and we pass with this IPC the signature of the completion handler (serialized as a String).
3. Process B receives the IPC, decodes the completion handler's signature and create a reply block for it, and passes with when calling the local method M. What this block does is send an IPC back to process A with the reply.

There must be a way to validate that the block we create at step 3 has the same signature as the one expected by local method M. I will look more into it.

>> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:238
>> +                NSLog(@"_invokeMethod: Failed to validate reply block signature: %@", wireBlockSignature._typeString);
> 
> Perhaps we should kill the WebContent process when this happens instead of just logging? Seems equivalent to an ill-formed CoreIPC message.

Like for IPC::Connection, it appears _WKRemoteObjectRegistry can be used for communication from the WebProcess to the UIProcess AND from the UIProcess to the WebContent process. The IPC::Connection code normally does not terminate the process, it merely marks messages as invalid and lets the client decide what to do about it.
The reason is that the policy is different depending on who the sender is (because we trust the UIProcess but not the WebProcess). We always want to kill the WebContent process, never the UIProcess. Anyway, calling CRASH() here would likely not be the right thing to do as we may end up crashing the UIProcess for a bad message from a compromised WebProcess.

I followed the pattern in the rest of _WKRemoteObjectRegistry.mm, the pattern is always to NSLog and return (ignore the message in case of an error), e.g.:
    @try {
        replyInvocation = [decoder decodeObjectOfClass:[NSInvocation class] forKey:invocationKey];
    } @catch (NSException *exception) {
        NSLog(@"Exception caught during decoding of reply: %@", exception);
        return;
    }

or

    @try {
        [invocation invoke];
    } @catch (NSException *exception) {
        NSLog(@"%@: Warning: Exception caught during invocation of received message, dropping incoming message .\nException: %@", self, exception);
    }
Comment 5 Chris Dumez 2019-07-09 08:51:49 PDT
Comment on attachment 373655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373655&action=review

> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:234
>              NSMethodSignature *wireBlockSignature = [NSMethodSignature signatureWithObjCTypes:replyInfo->blockSignature.utf8().data()];

Since it seems that methodSignature is the signature of the local method, I assumed I could compare wireBlockSignature to [methodSignature _signatureForBlockAtArgumentIndex:i] and make sure they are equal.
However, for some reason, [methodSignature _signatureForBlockAtArgumentIndex:i] returns nil.
Comment 6 Chris Dumez 2019-07-09 09:46:20 PDT
Comment on attachment 373655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373655&action=review

>> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:234
>>              NSMethodSignature *wireBlockSignature = [NSMethodSignature signatureWithObjCTypes:replyInfo->blockSignature.utf8().data()];
> 
> Since it seems that methodSignature is the signature of the local method, I assumed I could compare wireBlockSignature to [methodSignature _signatureForBlockAtArgumentIndex:i] and make sure they are equal.
> However, for some reason, [methodSignature _signatureForBlockAtArgumentIndex:i] returns nil.

I also tried to get the target's method signature like so:
NSMethodSignature *targetMethodSignature = [interfaceAndObject.first methodSignatureForSelector:invocation.selector];
I get a valid looking method signature. However, I still cannot get the type of the block parameter because the following returns null:
[targetMethodSignature _signatureForBlockAtArgumentIndex:i];

All I can tell is that the type of parameter at index i is @?, so a block. However, I can unable to introspect the parameters of said block for some reason.
Comment 7 Chris Dumez 2019-07-09 09:58:11 PDT
(In reply to Chris Dumez from comment #6)
> Comment on attachment 373655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373655&action=review
> 
> >> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:234
> >>              NSMethodSignature *wireBlockSignature = [NSMethodSignature signatureWithObjCTypes:replyInfo->blockSignature.utf8().data()];
> > 
> > Since it seems that methodSignature is the signature of the local method, I assumed I could compare wireBlockSignature to [methodSignature _signatureForBlockAtArgumentIndex:i] and make sure they are equal.
> > However, for some reason, [methodSignature _signatureForBlockAtArgumentIndex:i] returns nil.
> 
> I also tried to get the target's method signature like so:
> NSMethodSignature *targetMethodSignature = [interfaceAndObject.first
> methodSignatureForSelector:invocation.selector];
> I get a valid looking method signature. However, I still cannot get the type
> of the block parameter because the following returns null:
> [targetMethodSignature _signatureForBlockAtArgumentIndex:i];
> 
> All I can tell is that the type of parameter at index i is @?, so a block.
> However, I can unable to introspect the parameters of said block for some
> reason.

Ok, it looks like I found a way to get the block's signature finally. Working on a patch.
Comment 8 Chris Dumez 2019-07-09 10:15:14 PDT
Created attachment 373728 [details]
Patch
Comment 9 Geoffrey Garen 2019-07-09 10:49:28 PDT
Comment on attachment 373728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373728&action=review

r=me

> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:205
> +    ASSERT(expectedBlockSignature);

Should we null check and return false here too (as above)?

> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:246
> +            // Validate the signature.

Not sure this comment adds anything. The function name is very clear.
Comment 10 Chris Dumez 2019-07-09 10:54:39 PDT
Comment on attachment 373728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373728&action=review

>> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:205
>> +    ASSERT(expectedBlockSignature);
> 
> Should we null check and return false here too (as above)?

I would think the the [wireBlockSignature isEqual:expectedBlockSignature] call below would already return false if expectedBlockSignature were nil.
Comment 11 Chris Dumez 2019-07-09 10:55:06 PDT
Created attachment 373732 [details]
Patch
Comment 12 Geoffrey Garen 2019-07-09 11:00:00 PDT
> >> Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:205
> >> +    ASSERT(expectedBlockSignature);
> > 
> > Should we null check and return false here too (as above)?
> 
> I would think the the [wireBlockSignature isEqual:expectedBlockSignature]
> call below would already return false if expectedBlockSignature were nil.

Good point!
Comment 13 WebKit Commit Bot 2019-07-09 11:41:52 PDT
Comment on attachment 373732 [details]
Patch

Clearing flags on attachment: 373732

Committed r247264: <https://trac.webkit.org/changeset/247264>
Comment 14 WebKit Commit Bot 2019-07-09 11:41:54 PDT
All reviewed patches have been landed.  Closing bug.