Bug 199584 – Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod] for robustness.

[Chris Dumez]

<rdar://problem/46268249>

[Chris Dumez]

Created attachment 373655 [details]
Patch

[Maciej Stachowiak]

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.

[Chris Dumez]

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);
    }

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 373728 [details]
Patch

[Geoffrey Garen]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 373732 [details]
Patch

[Geoffrey Garen]

> >> 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!

[WebKit Commit Bot]

Comment on attachment 373732 [details]
Patch

Clearing flags on attachment: 373732

Committed r247264: <https://trac.webkit.org/changeset/247264>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.