Bug 168010 – [WebRTC][Mac][WebKit2] Conditionally add sandbox extensions to the Network Process

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

Conditionally add sandbox extensions to the Network Process when the WebRTC/Media Capture features are enabled.

[Jon Lee]

rdar://problem/30245503

[Brent Fulgham]

Created attachment 301326 [details]
Patch

[WebKit Commit Bot]

Attachment 301326 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:120:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:121:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:316:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:324:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 301371 [details]
Patch

[Alex Christensen]

Comment on attachment 301371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301371&action=review

> Source/WebKit2/ChangeLog:12
> +        Note: This does not yet do anything in the UIProcess to confirm we should expand the sandbox.

Not yet...

> Source/WebKit2/ChangeLog:18
> +        2. Each NetworkRTCProvider should remember what sandbox extensions were created on its behalf,
> +           and revoke them when closing down.

Since the NetworkRTCProvider is kind of owned by the NetworkConnectionToWebProcess, we might not want to wait until that WebProcess closes before revoking the sandbox extensions.  For example, if a user navigated to a page that used WebRTC once a few months ago and then navigated away, we don't want to keep the sandbox extension.  Closing it earlier will require a change to something that isn't touched by this patch, though.

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:50
> +#if USE(LIBWEBRTC)
> +#include <wtf/text/StringBuilder.h>

Let's include this unconditionally.

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:309
> +    WTFLogAlways("NetworkProcessProxy::grantSandboxExtensionsToRTCProvider: Expanding sandbox to include: %s", extensionBuilder.toString().utf8().data());

Do we really want to WTFLogAlways this?

[youenn fablet]

Some discussion items we discussed with brent & ap:
- Having the extension be passed as a Ref<>&&
- Adding a failure message to remove the handler if denied
- The UI process may still want to have something to say about TCP client sockets even though these might not need any sandbox extension (to be checked)

[Brent Fulgham]

Created attachment 301405 [details]
Patch

[youenn fablet]

Comment on attachment 301405 [details]
Patch

LGTM.
A few nits below.

View in context: https://bugs.webkit.org/attachment.cgi?id=301405&action=review

> Source/WebKit2/NetworkProcess/NetworkProcess.h:229
> +    HashMap<uint64_t, Function<void(RefPtr<SandboxExtension>)>> m_sandboxExtensionsForRTCProviderCompletionHandlers;

Shouldn't it be Function<void(Ref<SandboxExtension>)&&> ?

> Source/WebKit2/NetworkProcess/NetworkProcess.messages.in:81
> +    DidGrantSandboxExtensionsToRTCProvider(uint64_t identifier, WebKit::SandboxExtension::Handle handle)

s/Handle handle/Handle/

> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:81
> +        [this, identifier, socketAddress, minPort, maxPort](Ref<SandboxExtension>&& extension) {

You need to ensure that "this" is not captured by taking a ref from it, something like:
protectedThis = makeRef(*this).

That is why we also need to ensure the UI process actually denies access instead of not responding, should we have a need for it.
Not sure how to best keep that information.
Maybe a comment in the UI process code?

> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:83
> +            callOnRTCNetworkThread([this, identifier, socketAddress, minPort, maxPort]() {

No need here to protect "this" since NetworkRTCProvider::NetworkRTCProvider does it for you.

> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:113
> +    WTFLogAlways("NetworkRTCProvider::createClientTCPSocket: Remind me to open a client TCP socket from %s to %s once I have an extension.", localAddress.utf8().data(), remoteAddress.utf8().data());

Why having this log here?

> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.h:105
> +    Vector<RefPtr<SandboxExtension>> m_sandboxExtensions;

We support Vector<Ref<>> I think

[youenn fablet]

Some discussion items related to webrtc networking and sandbox and have webrtc networking in its own process.
That would have a few benefits:
- No need to relax the current network process sandbox
- Tighten webrtc networking sandbox (no file access...)
- No risk of compromising the network process with libwebrtc code issues
- Go directly from libwebrtc network thread (in the web process) to webrtc networking process

[Brent Fulgham]

(In reply to comment #9)
> Some discussion items related to webrtc networking and sandbox and have
> webrtc networking in its own process.
> That would have a few benefits:
> - No need to relax the current network process sandbox
> - Tighten webrtc networking sandbox (no file access...)
> - No risk of compromising the network process with libwebrtc code issues
> - Go directly from libwebrtc network thread (in the web process) to webrtc
> networking process

Yeah -- given some of the limitations in what we can dynamically change in the sandbox, moving to a separate process would probably be a better model.

[Brent Fulgham]

Created attachment 301682 [details]
Patch

[youenn fablet]

Comment on attachment 301682 [details]
Patch

Since we are not able to consume the network sandbox extension, I would remove the code related to it from this patch.
The grant/deny callback would then be Function<void()>>.

I would also consider splitting the verification to check for socket opening by the UIProcess in a separate patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=301682&action=review

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:316
> +void NetworkProcess::grantSandboxExtensionsToRTCProvider(const String& protocol, const String& address, uint16_t minPort, uint16_t maxPort, Function<void(Ref<SandboxExtension>&&)>&& completionHandler)

If we keep this mechanism, I would rename it to something like grantRTCConnection/denyRTCConnection.
When being allowed, a sandbox extension might be given if the network process can handle dynamic extension.

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:319
> +    m_sandboxExtensionsForRTCProviderCompletionHandlers.set(requestID, WTFMove(completionHandler));

I would rename it to m_pendingConnections.

> Source/WebKit2/NetworkProcess/NetworkProcess.h:229
> +    HashMap<uint64_t, Function<void(Ref<SandboxExtension>)>> m_sandboxExtensionsForRTCProviderCompletionHandlers;

If we keep it, it should probably be Function<void(Ref<SandboxExtension>&&>

> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:112
> +    WTFLogAlways("NetworkRTCProvider::createClientTCPSocket: Remind me to open a client TCP socket from %s to %s once I have an extension.", localAddress.utf8().data(), remoteAddress.utf8().data());

Is it needed?

> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.h:105
> +    Vector<RefPtr<SandboxExtension>> m_sandboxExtensions;

Should be a Vector of Ref.

> Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:249
> +    bool webRTCEnabled = m_defaultPageGroup->preferences().peerConnectionEnabled();

We should test for mediaStreamEnabled, so s/webRTCEnabled/mediaStreamEnabled.
Also we redefine webRTCEnabled below, this would fix the issue here.

> Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:307
> +#if ENABLE(MEDIA_STREAM)

Should be ENABLE(WEB_RTC)

[Brent Fulgham]

(In reply to comment #12)
> Comment on attachment 301682 [details]
> Patch
> 
> I would also consider splitting the verification to check for socket opening
> by the UIProcess in a separate patch.

I'll land that patch under Bug 168242.

[Brent Fulgham]

Committed r212451: <http://trac.webkit.org/changeset/212451>