WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2019-07-08 12:15:26 PDT
<
rdar://problem/46268249
>
Chris Dumez
Comment 2
2019-07-08 12:18:24 PDT
Created
attachment 373655
[details]
Patch
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
Created
attachment 373728
[details]
Patch
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
Created
attachment 373732
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug