We should check that a user gesture is being handled when the 'performActionOnElement' message handler is attempting to save images to the user's media Library.
<rdar://72058321>
Created attachment 454637 [details] Patch
Comment on attachment 454637 [details] Patch Presumably, if the WebContent process is compromised enough that it can send bogus IPC, it is compromised enough that it can bypass this user gesture check no? My understanding was that any hardening should be done on IPC recipient side for this reason.
One idea (may need refining) is that WebPageProxy::performActionOnElement() could generate some kind of permission token (e.g. a UUID) and send it with the WebPage::PerformActionOnElement IPC. Then, when WebPage::performActionOnElement() sends IPC back, it would have to pass this same token that the UIProcess could validate. This way, we'd be sure that the WebPageProxy::SaveImageToLibrary IPC was the legitimate result of a legit WebPage::PerformActionOnElement IPC from the UIProcess. Or something in that spirit.
(In reply to Chris Dumez from comment #3) > Comment on attachment 454637 [details] > Patch > > Presumably, if the WebContent process is compromised enough that it can send > bogus IPC, it is compromised enough that it can bypass this user gesture > check no? My understanding was that any hardening should be done on IPC > recipient side for this reason. Yes -- I misunderstood how the UserGestureIndicator worked -- this checks for the indicator, but it won't exist yet. We would need to pass the token with the message. I don't see examples of message passing UI tokens, Instead, we just send a boolean -- but I don't think that improves things at all.
(In reply to Brent Fulgham from comment #5) > (In reply to Chris Dumez from comment #3) > > Comment on attachment 454637 [details] > > Patch > > > > Presumably, if the WebContent process is compromised enough that it can send > > bogus IPC, it is compromised enough that it can bypass this user gesture > > check no? My understanding was that any hardening should be done on IPC > > recipient side for this reason. > > Yes -- I misunderstood how the UserGestureIndicator worked -- this checks > for the indicator, but it won't exist yet. We would need to pass the token > with the message. > > I don't see examples of message passing UI tokens, Instead, we just send a > boolean -- but I don't think that improves things at all. As suggested above, we could send a UI token (like a UUID). If I remember correctly, I did just that for Geolocation permission.
(In reply to Chris Dumez from comment #6) > (In reply to Brent Fulgham from comment #5) > > (In reply to Chris Dumez from comment #3) > > > Comment on attachment 454637 [details] > > > Patch > > > > > > Presumably, if the WebContent process is compromised enough that it can send > > > bogus IPC, it is compromised enough that it can bypass this user gesture > > > check no? My understanding was that any hardening should be done on IPC > > > recipient side for this reason. > > > > Yes -- I misunderstood how the UserGestureIndicator worked -- this checks > > for the indicator, but it won't exist yet. We would need to pass the token > > with the message. > > > > I don't see examples of message passing UI tokens, Instead, we just send a > > boolean -- but I don't think that improves things at all. > > As suggested above, we could send a UI token (like a UUID). If I remember > correctly, I did just that for Geolocation permission. See this example: DidReceiveGeolocationPermissionDecision(WebKit::GeolocationIdentifier geolocationID, String authorizationToken)
Created attachment 454734 [details] Patch
Comment on attachment 454734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454734&action=review It looks like I need to rebase my build before I can put this patch up. > Source/WebKit/UIProcess/WebPageProxy.h:2718 > + StdSet<uint64_t> m_userGestureTokens; @Chris: This duplicates work you did for the GeoLocation manager. Should we modify GeoLocation to make use of this authentication token storage?
Created attachment 454738 [details] Patch
Comment on attachment 454738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454738&action=review > Source/WebKit/UIProcess/WebPageProxy.h:2397 > + bool isValidAuthorizationToken(const String& authorizationToken) const; Same comment as below about the name being so generic while this is not truly reusable. > Source/WebKit/UIProcess/WebPageProxy.h:2398 > + void revokeAuthorizationToken(const String& authorizationToken); ditto. > Source/WebKit/UIProcess/WebPageProxy.h:2716 > + StdSet<uint64_t> m_userGestureTokens; This is unused I think. > Source/WebKit/UIProcess/WebPageProxy.h:3074 > + HashSet<String> m_validAuthorizationTokens; I wonder a bit about the naming being so generic. It makes it look like this is directly reusable for other IPCs. However, I don't think that's true with the current implementation. I don't think you would want the WebProcess to use an auth token from PerformActionOnElement to do some other operation in the UIProcess that is not the result of the PerformActionOnElement IPC. E.g. It might need to be a HashMap from the IPC message name to the auth token. Alternatively, we could keep using a HashSet but the auth-token would be specific to the IPC message name (e.g. "PerformActionOnElement-[UUID]"). If we start using this more (which sounds plausible), then we'll like need something a bit better to better restrict this to certain IPCs. As a result, I would suggest using a more specific name for now (e.g. m_performActionOnElementAuthTokens). Alternatively, we can try and solve the "how to make it generic" problem now but I'd be OK just crossing that bridge when we get there and use a specific name for now. > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:605 > + if (!isValidAuthorizationToken(authorizationToken)) This does a double HashSet lookup which is unfortunate: once for the contains, and then again for the remove. This is not great practice. > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:617 > + auto authorizationToken = needsAuthorizationToken(action) ? createVersion4UUIDString() : String(); Do we really gain much by only passing an authorizationToken for SaveImage? Why can't we just always send one, seems like it would make the code simpler. With the sendWithAsyncReply() I propose below, the auth token will get nicely revoked no matter what. > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:620 > + m_process->send(Messages::WebPage::PerformActionOnElement(action, authorizationToken), m_webPageID); I think it would be nice to use sendWithAsyncReply() and revoke the auth token in the reply lambda. > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3285 > + send(Messages::WebPageProxy::SaveImageToLibrary(SharedMemory::IPCHandle { WTFMove(handle), buffer->size() }, authorizationToken)); It is probably a good idea to do the same for the `send(Messages::WebPageProxy::WritePromisedAttachmentToPasteboard(WTFMove(attachmentInfo)));` call above in this function no?
Comment on attachment 454738 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454738&action=review >> Source/WebKit/UIProcess/WebPageProxy.h:2397 >> + bool isValidAuthorizationToken(const String& authorizationToken) const; > > Same comment as below about the name being so generic while this is not truly reusable. Will do. >> Source/WebKit/UIProcess/WebPageProxy.h:2398 >> + void revokeAuthorizationToken(const String& authorizationToken); > > ditto. Will do. >> Source/WebKit/UIProcess/WebPageProxy.h:2716 >> + StdSet<uint64_t> m_userGestureTokens; > > This is unused I think. Whoops! >> Source/WebKit/UIProcess/WebPageProxy.h:3074 >> + HashSet<String> m_validAuthorizationTokens; > > I wonder a bit about the naming being so generic. It makes it look like this is directly reusable for other IPCs. However, I don't think that's true with the current implementation. > I don't think you would want the WebProcess to use an auth token from PerformActionOnElement to do some other operation in the UIProcess that is not the result of the PerformActionOnElement IPC. > > E.g. It might need to be a HashMap from the IPC message name to the auth token. Alternatively, we could keep using a HashSet but the auth-token would be specific to the IPC message name (e.g. "PerformActionOnElement-[UUID]"). > > If we start using this more (which sounds plausible), then we'll like need something a bit better to better restrict this to certain IPCs. As a result, I would suggest using a more specific name for now (e.g. m_performActionOnElementAuthTokens). > Alternatively, we can try and solve the "how to make it generic" problem now but I'd be OK just crossing that bridge when we get there and use a specific name for now. I'll use m_performActionOnElementAuthTokens for now. >> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:605 >> + if (!isValidAuthorizationToken(authorizationToken)) > > This does a double HashSet lookup which is unfortunate: once for the contains, and then again for the remove. This is not great practice. I'm not even sure this method is needed. I only use it in the 'saveImageToLibrary', which does a message check using the token. I could probably do a find and remove in that method without needing this specialized method. >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3285 >> + send(Messages::WebPageProxy::SaveImageToLibrary(SharedMemory::IPCHandle { WTFMove(handle), buffer->size() }, authorizationToken)); > > It is probably a good idea to do the same for the `send(Messages::WebPageProxy::WritePromisedAttachmentToPasteboard(WTFMove(attachmentInfo)));` call above in this function no? I wasn't sure if some sites might write to pasteboard based on script execution as a convenience for the user. I think we are less worried about pasteboard writes than pasteboard reads. But I'm happy to make that change if you think it's warranted.
(In reply to Brent Fulgham from comment #12) > Comment on attachment 454738 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454738&action=review > > >> Source/WebKit/UIProcess/WebPageProxy.h:2397 > >> + bool isValidAuthorizationToken(const String& authorizationToken) const; > > > > Same comment as below about the name being so generic while this is not truly reusable. > > Will do. > > >> Source/WebKit/UIProcess/WebPageProxy.h:2398 > >> + void revokeAuthorizationToken(const String& authorizationToken); > > > > ditto. > > Will do. > > >> Source/WebKit/UIProcess/WebPageProxy.h:2716 > >> + StdSet<uint64_t> m_userGestureTokens; > > > > This is unused I think. > > Whoops! > > >> Source/WebKit/UIProcess/WebPageProxy.h:3074 > >> + HashSet<String> m_validAuthorizationTokens; > > > > I wonder a bit about the naming being so generic. It makes it look like this is directly reusable for other IPCs. However, I don't think that's true with the current implementation. > > I don't think you would want the WebProcess to use an auth token from PerformActionOnElement to do some other operation in the UIProcess that is not the result of the PerformActionOnElement IPC. > > > > E.g. It might need to be a HashMap from the IPC message name to the auth token. Alternatively, we could keep using a HashSet but the auth-token would be specific to the IPC message name (e.g. "PerformActionOnElement-[UUID]"). > > > > If we start using this more (which sounds plausible), then we'll like need something a bit better to better restrict this to certain IPCs. As a result, I would suggest using a more specific name for now (e.g. m_performActionOnElementAuthTokens). > > Alternatively, we can try and solve the "how to make it generic" problem now but I'd be OK just crossing that bridge when we get there and use a specific name for now. > > I'll use m_performActionOnElementAuthTokens for now. > > >> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:605 > >> + if (!isValidAuthorizationToken(authorizationToken)) > > > > This does a double HashSet lookup which is unfortunate: once for the contains, and then again for the remove. This is not great practice. > > I'm not even sure this method is needed. I only use it in the > 'saveImageToLibrary', which does a message check using the token. I could > probably do a find and remove in that method without needing this > specialized method. > > >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3285 > >> + send(Messages::WebPageProxy::SaveImageToLibrary(SharedMemory::IPCHandle { WTFMove(handle), buffer->size() }, authorizationToken)); > > > > It is probably a good idea to do the same for the `send(Messages::WebPageProxy::WritePromisedAttachmentToPasteboard(WTFMove(attachmentInfo)));` call above in this function no? > > I wasn't sure if some sites might write to pasteboard based on script > execution as a convenience for the user. I think we are less worried about > pasteboard writes than pasteboard reads. But I'm happy to make that change > if you think it's warranted. I think we should add the restriction for WritePromisedAttachmentToPasteboard. I checked and the only call site for the WritePromisedAttachmentToPasteboard IPC is in WebPage::performActionOnElement(). WebPage::performActionOnElement() is only called as a result of the WebPage::PerformActionOnElement IPC from the UIProcess. Therefore, I think the check is safe and very similar to the image saving case. It is also not much added complexity I think.
(In reply to Chris Dumez from comment #13) > > I think we should add the restriction for > WritePromisedAttachmentToPasteboard. I checked and the only call site for > the WritePromisedAttachmentToPasteboard IPC is in > WebPage::performActionOnElement(). WebPage::performActionOnElement() is only > called as a result of the WebPage::PerformActionOnElement IPC from the > UIProcess. > > Therefore, I think the check is safe and very similar to the image saving > case. It is also not much added complexity I think. Will do!
Created attachment 454856 [details] Patch
Created attachment 454858 [details] Patch
Comment on attachment 454858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454858&action=review r=me with changes. > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:608 > + sendWithAsyncReply(Messages::WebPage::PerformActionOnElement(action, authorizationToken), [weakThis = WeakPtr { *this }, authorizationToken] () mutable { authorizationToken = WTFMove(authorizationToken) > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3243 > { I think you should just add the following at the beginning of the function: CompletionHandlerCallingScope callCompletionHandler(WTFMove(completionHandler)); Then you wouldn't need to call the completionHandler on every early return (which is a foot-gun).
Comment on attachment 454858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454858&action=review >> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:608 >> + sendWithAsyncReply(Messages::WebPage::PerformActionOnElement(action, authorizationToken), [weakThis = WeakPtr { *this }, authorizationToken] () mutable { > > authorizationToken = WTFMove(authorizationToken) I tried this initially, and it resulted in the message send getting an empty string (I guess because the PerformActionOnElement constructor runs after the 'authorizationToken' is moved into the lambda). >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3243 >> { > > I think you should just add the following at the beginning of the function: > CompletionHandlerCallingScope callCompletionHandler(WTFMove(completionHandler)); > > Then you wouldn't need to call the completionHandler on every early return (which is a foot-gun). Oh! I was thinking such a thing would be good to have -- I didn't know it existed already. That's great!
Comment on attachment 454858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454858&action=review >>> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:608 >>> + sendWithAsyncReply(Messages::WebPage::PerformActionOnElement(action, authorizationToken), [weakThis = WeakPtr { *this }, authorizationToken] () mutable { >> >> authorizationToken = WTFMove(authorizationToken) > > I tried this initially, and it resulted in the message send getting an empty string (I guess because the PerformActionOnElement constructor runs after the 'authorizationToken' is moved into the lambda). Oh, you're right. My bad. I didn't realize it was being used on the same line.
Created attachment 454865 [details] Patch for landing
Created attachment 454873 [details] Patch for landing
Patch 454865 does not build
Comment on attachment 454873 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=454873&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:10195 > +#if PLATFORM(IOS_FAMILY) This looks a bit weird. Seems like the whole writePromisedAttachmentToPasteboard function and (its IPC message) should be protected via `#if PLATFORM(IOS_FAMILY)`, not just the hardening. It makes it look like we're not doing hardening on macOS.
Created attachment 454890 [details] Patch for landing
Committed r291371 (248503@main): <https://commits.webkit.org/248503@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454890 [details].