Bug 199584 - Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod]
Summary: Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-08 12:15 PDT by Chris Dumez
Modified: 2019-07-09 12:11 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.70 KB, patch)
2019-07-08 12:18 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.54 KB, patch)
2019-07-09 10:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.50 KB, patch)
2019-07-09 10:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.