[webkit-reviews] review canceled: [Bug 199584] Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod] : [Attachment 373655] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has canceled Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 199584: Validate reply block signature in [WKRemoteObjectRegistry
_invokeMethod]
https://bugs.webkit.org/show_bug.cgi?id=199584

Attachment 373655: Patch

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




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 373655
  --> https://bugs.webkit.org/attachment.cgi?id=373655
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);
    }