RESOLVED FIXED 199584
Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod]
https://bugs.webkit.org/show_bug.cgi?id=199584
Summary Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod]
Chris Dumez
Reported 2019-07-08 12:15:12 PDT
Validate reply block signature in [WKRemoteObjectRegistry _invokeMethod] for robustness.
Attachments
Patch (2.70 KB, patch)
2019-07-08 12:18 PDT, Chris Dumez
no flags
Patch (3.54 KB, patch)
2019-07-09 10:15 PDT, Chris Dumez
no flags
Patch (3.50 KB, patch)
2019-07-09 10:55 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2019-07-08 12:15:26 PDT
Chris Dumez
Comment 2 2019-07-08 12:18:24 PDT
Maciej Stachowiak
Comment 3 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.
Chris Dumez
Comment 4 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); }
Chris Dumez
Comment 5 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.
Chris Dumez
Comment 6 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.
Chris Dumez
Comment 7 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.
Chris Dumez
Comment 8 2019-07-09 10:15:14 PDT
Geoffrey Garen
Comment 9 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.
Chris Dumez
Comment 10 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.
Chris Dumez
Comment 11 2019-07-09 10:55:06 PDT
Geoffrey Garen
Comment 12 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!
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2019-07-09 11:41:54 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.