Although we prevent access to WebGL and WebAudio APIs in the WebContent Process, we should also add MESSAGE_CHECK protections in the GPU Process to prevent it from accepting messages for prohibited API from a Captive Portal process.
<rdar://91478748>
Created attachment 457998 [details] Patch
Comment on attachment 457998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457998&action=review > Source/WebKit/ChangeLog:10 > + sent froma Captive Portal process. This change also adds a flag to the GPUProcessConnectionParameters typo: froma > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:161 > +#define MESSAGE_CHECK(assertion) MESSAGE_CHECK_BASE(assertion, (&connection())) Did you validate that the WebProcess is indeed exiting and relaunching when this happens? I think this should work in theory since MESSAGE_CHECK_BASE() will mark the message as invalid and then GPUConnectionToWebProcess::didReceiveInvalidMessage() will ask for the WebProcess to be terminated. However, it is good to double check manually that it works. > Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:152 > + MESSAGE_CHECK(!m_gpuConnectionToWebProcess.isCaptivePortalModeEnabled()); Here it is a little less obvious that the MESSAGE_CHECK() does the right thing, not sure what the handling is when a message gets marked as invalid in the context of a RemoteAudioDestinationManager. Would be good to double check if you haven't yet. The main worry is that we'll just sever the IPC connection and the WebProcess will just reconnect without getting killed. In the UIProcess, severing the IPC connection causes the child process to exit but that's not the case here since the GPU process is not the parent process.
Comment on attachment 457998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457998&action=review >> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:152 >> + MESSAGE_CHECK(!m_gpuConnectionToWebProcess.isCaptivePortalModeEnabled()); > > Here it is a little less obvious that the MESSAGE_CHECK() does the right thing, not sure what the handling is when a message gets marked as invalid in the context of a RemoteAudioDestinationManager. Would be good to double check if you haven't yet. > > The main worry is that we'll just sever the IPC connection and the WebProcess will just reconnect without getting killed. In the UIProcess, severing the IPC connection causes the child process to exit but that's not the case here since the GPU process is not the parent process. I patterned this off of the MESSAGE_CHECK in RemoteMediaRecorder, but maybe those are wrong, too? Do you think I should duplicate the TERMINATE_WEB_PROCESS_WITH_MESSAGE logic Wenson added to 'RemoteRenderingBackend.cpp'? And if so, should we make the same correction to RemoteMediaRecorder?
(In reply to Brent Fulgham from comment #4) > Comment on attachment 457998 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457998&action=review > > >> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:152 > >> + MESSAGE_CHECK(!m_gpuConnectionToWebProcess.isCaptivePortalModeEnabled()); > > > > Here it is a little less obvious that the MESSAGE_CHECK() does the right thing, not sure what the handling is when a message gets marked as invalid in the context of a RemoteAudioDestinationManager. Would be good to double check if you haven't yet. > > > > The main worry is that we'll just sever the IPC connection and the WebProcess will just reconnect without getting killed. In the UIProcess, severing the IPC connection causes the child process to exit but that's not the case here since the GPU process is not the parent process. > > I patterned this off of the MESSAGE_CHECK in RemoteMediaRecorder, but maybe > those are wrong, too? > > Do you think I should duplicate the TERMINATE_WEB_PROCESS_WITH_MESSAGE logic > Wenson added to 'RemoteRenderingBackend.cpp'? And if so, should we make the > same correction to RemoteMediaRecorder? I think we should validate if your approach is working in practice. I don't know that the person who added those checks to RemoteMediaRecorder did that. TERMINATE_WEB_PROCESS_WITH_MESSAGE() was validated if I remember correctly so that should work fine. That said, I'd be fine with your approach if you validate manually that the WebProcess actually gets killed on bad IPC.
Created attachment 458300 [details] Patch
(In reply to Chris Dumez from comment #5) > (In reply to Brent Fulgham from comment #4) > > Comment on attachment 457998 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=457998&action=review > > > > >> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:152 > > >> + MESSAGE_CHECK(!m_gpuConnectionToWebProcess.isCaptivePortalModeEnabled()); > > > > > > Here it is a little less obvious that the MESSAGE_CHECK() does the right thing, not sure what the handling is when a message gets marked as invalid in the context of a RemoteAudioDestinationManager. Would be good to double check if you haven't yet. > > > > > > The main worry is that we'll just sever the IPC connection and the WebProcess will just reconnect without getting killed. In the UIProcess, severing the IPC connection causes the child process to exit but that's not the case here since the GPU process is not the parent process. > > > > I patterned this off of the MESSAGE_CHECK in RemoteMediaRecorder, but maybe > > those are wrong, too? > > > > Do you think I should duplicate the TERMINATE_WEB_PROCESS_WITH_MESSAGE logic > > Wenson added to 'RemoteRenderingBackend.cpp'? And if so, should we make the > > same correction to RemoteMediaRecorder? > > I think we should validate if your approach is working in practice. I don't > know that the person who added those checks to RemoteMediaRecorder did that. > > TERMINATE_WEB_PROCESS_WITH_MESSAGE() was validated if I remember correctly > so that should work fine. That said, I'd be fine with your approach if you > validate manually that the WebProcess actually gets killed on bad IPC. I marked the GPU process connection as being on behalf of a Captive Portal in all cases, and then exercised WebGL and WebAudio samples. In the WebGL case, the MESSAGE_CHECK calls killed the relevant WebProcess, as expected. The remote audio case killed the GPU process, not the WebContent process. This seems wrong, so I will try Wenson's approach there.
Comment on attachment 458300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458300&action=review > Source/WebKit/ChangeLog:12 > + Add MESSAGE_CHECK protections to a set of message handlers for API prohibited when > + sent from a Captive Portal process. This change also adds a flag to the GPUProcessConnectionParameters > + struct so that GPU Process connections can be marked as being associated with a Captive Portal > + process. What prevents the web process from trying to send these messages in the captive portal case? To state the obvious, we don’t want a MESSAGE_CHECK to ever fire unless the web process is compromised. So presumably we have to have code that prevents trying to send these messages in normal circumstances, along with the MESSAGE_CHECK protection that is where the full security value comes from when the web process has been controlled or attacked.
Comment on attachment 458300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458300&action=review >> Source/WebKit/ChangeLog:12 >> + process. > > What prevents the web process from trying to send these messages in the captive portal case? To state the obvious, we don’t want a MESSAGE_CHECK to ever fire unless the web process is compromised. So presumably we have to have code that prevents trying to send these messages in normal circumstances, along with the MESSAGE_CHECK protection that is where the full security value comes from when the web process has been controlled or attacked. Indeed! We block access to these APIs in the captive portal case, so in theory script should not be capable of sending these messages. However, if an attacker did gain control a captive portal process and attempted to send off-limits messages to the GPU process we want to recognize this and kill the misbehaving process. The first part is already true, this patch attempts to solve the second issue.
Created attachment 458306 [details] Patch for landing
Committed r293380 (249970@main): <https://commits.webkit.org/249970@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458306 [details].