Mach lookup to com.apple.webinspector should not be allowed in WebKit's WebContent process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Currently, mach lookup to com.apple.webinspector is allowed in the WebContent process, but instead of always allowing this, we should issue a mach sandbox extension in the UI process when the user is enabling the Web inspector.

[Per Arne Vollan]

Created attachment 381472 [details]
Patch

[Per Arne Vollan]

Created attachment 381481 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 381472 [details]
Patch

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

> Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:43
> +#if PLATFORM(COCOA)
> +std::atomic<bool> RemoteInspector::needMachSandboxExtension = true;
> +#endif

Hmm. Since the default is false, this looks like like it could break remote debugging of a JSContext:

    (1) Applications/Daemons should already have access to the port in the default sandbox rules
    (2) Disabling RemoteInspector by default means JSContext only applications would not be debuggable

I see that you relax the restriction for +[WebView initialization] (WebView / UIWebView), but it seems we would also need to do the same for JSContext / JSContextRef.

However maybe the default should be that the sandbox requirement is not necessary, and it is only turned on in WebContentProcess? That seems less likely to be problematic for Remote Inspection in the future.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:111
> +#if PLATFORM(COCOA)
> +    static void setNeedMachSandboxExtension(bool needExtension) { needMachSandboxExtension = needExtension; }
> +#endif

This is only ever called with `false`. How about renaming it to describe what it is effectively doing?

    static void relaxSandboxExtensionRequirement() { needMachSandboxExtension = false; }

> Source/WebKit/ChangeLog:12
> +        If the Web inspector is enabled when the WebContent process is started, a sandbox extension is created
> +        for 'com.apple.webinspectord' and a message is sent to the WebContent process, where the extension will
> +        be consumed, and the remote Web inspector will be started. The same happens if Web inspector is enabled
> +        by the user while Safari is running. When RemoteInspector::singleton() is called in the UI process there
> +        is no need for an extension, since access to the Web inspector daemon is already allowed.

Given we now go through the UIProcess... does the WebContent process ever need access to this mach port now? Hmm, it probably needs it for direct Service-Worker debugging.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:869
> +    if (CFPreferencesGetAppIntegerValue(CFSTR("RemoteInspectorEnabled"), CFSTR("com.apple.webinspectord"), &keyExistsAndHasValidFormat) && keyExistsAndHasValidFormat) {

Is it normal to have access to this? I would not expect this.

[Alex Christensen]

Comment on attachment 381481 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:1041
> +    Inspector::RemoteInspector::setNeedMachSandboxExtension(false);

This doesn't look right.  Shouldn't we be consuming a sandbox extension before calling this?

> Source/WebKit/WebProcess/WebProcess.messages.in:135
> +    RemoteWebInspectorEnabled(WebKit::SandboxExtension::Handle handle);

This sounds like it should return a bool.  EnableRemoteWebInspector?

[Per Arne Vollan]

(In reply to Joseph Pecoraro from comment #3)
> Comment on attachment 381472 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381472&action=review
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:43
> > +#if PLATFORM(COCOA)
> > +std::atomic<bool> RemoteInspector::needMachSandboxExtension = true;
> > +#endif
> 
> Hmm. Since the default is false, this looks like like it could break remote
> debugging of a JSContext:
> 
>     (1) Applications/Daemons should already have access to the port in the
> default sandbox rules
>     (2) Disabling RemoteInspector by default means JSContext only
> applications would not be debuggable
> 
> I see that you relax the restriction for +[WebView initialization] (WebView
> / UIWebView), but it seems we would also need to do the same for JSContext /
> JSContextRef.
> 
> However maybe the default should be that the sandbox requirement is not
> necessary, and it is only turned on in WebContentProcess? That seems less
> likely to be problematic for Remote Inspection in the future.
> 

Good point, I'll change the default to not require a sandbox extension.

> > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:111
> > +#if PLATFORM(COCOA)
> > +    static void setNeedMachSandboxExtension(bool needExtension) { needMachSandboxExtension = needExtension; }
> > +#endif
> 
> This is only ever called with `false`. How about renaming it to describe
> what it is effectively doing?
> 
>     static void relaxSandboxExtensionRequirement() {
> needMachSandboxExtension = false; }
> 

By changing the default of the flag to not needing a sandbox extension, I think the WebContent process will need to pass both true and false to this method.

> > Source/WebKit/ChangeLog:12
> > +        If the Web inspector is enabled when the WebContent process is started, a sandbox extension is created
> > +        for 'com.apple.webinspectord' and a message is sent to the WebContent process, where the extension will
> > +        be consumed, and the remote Web inspector will be started. The same happens if Web inspector is enabled
> > +        by the user while Safari is running. When RemoteInspector::singleton() is called in the UI process there
> > +        is no need for an extension, since access to the Web inspector daemon is already allowed.
> 
> Given we now go through the UIProcess... does the WebContent process ever
> need access to this mach port now? Hmm, it probably needs it for direct
> Service-Worker debugging.
> 

Oh, that is interesting! How can I test if it's needed for Service-Worker debugging?

> > Source/WebKit/UIProcess/WebProcessProxy.cpp:869
> > +    if (CFPreferencesGetAppIntegerValue(CFSTR("RemoteInspectorEnabled"), CFSTR("com.apple.webinspectord"), &keyExistsAndHasValidFormat) && keyExistsAndHasValidFormat) {
> 
> Is it normal to have access to this? I would not expect this.

You're right. This requires a new sandbox rule for the UI process (Safari), which will be added in separate patch.

Thanks for reviewing!

[Joseph Pecoraro]

> > > Source/WebKit/ChangeLog:12
> > > +        If the Web inspector is enabled when the WebContent process is started, a sandbox extension is created
> > > +        for 'com.apple.webinspectord' and a message is sent to the WebContent process, where the extension will
> > > +        be consumed, and the remote Web inspector will be started. The same happens if Web inspector is enabled
> > > +        by the user while Safari is running. When RemoteInspector::singleton() is called in the UI process there
> > > +        is no need for an extension, since access to the Web inspector daemon is already allowed.
> > 
> > Given we now go through the UIProcess... does the WebContent process ever
> > need access to this mach port now? Hmm, it probably needs it for direct
> > Service-Worker debugging.
> > 
> 
> Oh, that is interesting! How can I test if it's needed for Service-Worker
> debugging?

You can visit a page like `https://airhorner.com` and you should continue to see the airhorner.com Service Worker in the Develop menu:

    macOS: Develop > Service Worker > airhorner
    iOS:   Develop > (Device Name) > airhorner

> > > Source/WebKit/UIProcess/WebProcessProxy.cpp:869
> > > +    if (CFPreferencesGetAppIntegerValue(CFSTR("RemoteInspectorEnabled"), CFSTR("com.apple.webinspectord"), &keyExistsAndHasValidFormat) && keyExistsAndHasValidFormat) {
> > 
> > Is it normal to have access to this? I would not expect this.
> 
> You're right. This requires a new sandbox rule for the UI process (Safari),
> which will be added in separate patch.

I'm not exactly a fan of this approach. If this `RemoteInspectorEnabled` setting changes, then someone will have to remember to change WebKit. Currently that setting (on COCOA) is only set in one place and checked in one place.

[Per Arne Vollan]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 381481 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381481&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:1041
> > +    Inspector::RemoteInspector::setNeedMachSandboxExtension(false);
> 
> This doesn't look right.  Shouldn't we be consuming a sandbox extension
> before calling this?
> 

Not in this case, since the UI process does not need to consume a web inspector extension. However, this line will not be needed when the default flag for needing a sandbox extension is being changed to false.

> > Source/WebKit/WebProcess/WebProcess.messages.in:135
> > +    RemoteWebInspectorEnabled(WebKit::SandboxExtension::Handle handle);
> 
> This sounds like it should return a bool.  EnableRemoteWebInspector?

Yes, that is much better. Thanks for reviewing!

[Per Arne Vollan]

Created attachment 381609 [details]
Patch

[Per Arne Vollan]

(In reply to Joseph Pecoraro from comment #6)
> > > > Source/WebKit/ChangeLog:12
> > > > +        If the Web inspector is enabled when the WebContent process is started, a sandbox extension is created
> > > > +        for 'com.apple.webinspectord' and a message is sent to the WebContent process, where the extension will
> > > > +        be consumed, and the remote Web inspector will be started. The same happens if Web inspector is enabled
> > > > +        by the user while Safari is running. When RemoteInspector::singleton() is called in the UI process there
> > > > +        is no need for an extension, since access to the Web inspector daemon is already allowed.
> > > 
> > > Given we now go through the UIProcess... does the WebContent process ever
> > > need access to this mach port now? Hmm, it probably needs it for direct
> > > Service-Worker debugging.
> > > 
> > 
> > Oh, that is interesting! How can I test if it's needed for Service-Worker
> > debugging?
> 
> You can visit a page like `https://airhorner.com` and you should continue to
> see the airhorner.com Service Worker in the Develop menu:
> 
>     macOS: Develop > Service Worker > airhorner
>     iOS:   Develop > (Device Name) > airhorner
> 
> > > > Source/WebKit/UIProcess/WebProcessProxy.cpp:869
> > > > +    if (CFPreferencesGetAppIntegerValue(CFSTR("RemoteInspectorEnabled"), CFSTR("com.apple.webinspectord"), &keyExistsAndHasValidFormat) && keyExistsAndHasValidFormat) {
> > > 
> > > Is it normal to have access to this? I would not expect this.
> > 
> > You're right. This requires a new sandbox rule for the UI process (Safari),
> > which will be added in separate patch.
> 
> I'm not exactly a fan of this approach. If this `RemoteInspectorEnabled`
> setting changes, then someone will have to remember to change WebKit.
> Currently that setting (on COCOA) is only set in one place and checked in
> one place.

Yes, that is a good point. Perhaps these strings could be added to a header file?

[Brent Fulgham]

Comment on attachment 381609 [details]
Patch

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

Looks good. I think we should get an okay from a WebInsoector person like Joe before landing.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:502
> +    CFNotificationCenterRemoveObserver(CFNotificationCenterGetDarwinNotifyCenter(), this, static_cast<CFStringRef>(CFSTR(WIRServiceEnabledNotification)) , nullptr);

Extra white space around the commas.

[Per Arne Vollan]

Created attachment 381693 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #10)
> Comment on attachment 381609 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381609&action=review
> 
> Looks good. I think we should get an okay from a WebInsoector person like
> Joe before landing.
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:502
> > +    CFNotificationCenterRemoveObserver(CFNotificationCenterGetDarwinNotifyCenter(), this, static_cast<CFStringRef>(CFSTR(WIRServiceEnabledNotification)) , nullptr);
> 
> Extra white space around the commas.

Good catch, I have uploaded a new patch.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 381699 [details]
Patch

[Per Arne Vollan]

Created attachment 381703 [details]
Patch

[Per Arne Vollan]

Created attachment 381756 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 381756 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:501
> +    CFNotificationCenterRemoveObserver(CFNotificationCenterGetDarwinNotifyCenter(), this, static_cast<CFStringRef>(CFSTR(WIRServiceEnabledNotification)), nullptr);

Would you have to static_cast a string that is already a CFSTR? Seems like you should be able to drop the `static_cast<CFStringRef>` cast.

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:219
> +    if (!CFPreferencesGetAppIntegerValue(CFSTR(WIRRemoteInspectorEnabledKey), CFSTR(WIRRemoteInspectorDomainName), nullptr))
> +        return;

How will we guarantee that all UIProcesses have access to this setting? Do we want to expose all of their sandboxes to this?

[Per Arne Vollan]

Created attachment 381821 [details]
Patch

[Brent Fulgham]

Comment on attachment 381756 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:219
>> +        return;
> 
> How will we guarantee that all UIProcesses have access to this setting? Do we want to expose all of their sandboxes to this?

I'm not sure I understand the question here. It seems like this setting would be available to any WebKit client that links to a new build of WebKit.

[Brent Fulgham]

Comment on attachment 381821 [details]
Patch

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

> Source/JavaScriptCore/inspector/remote/RemoteInspectorConstants.h:123
> +#define WIRRemoteInspectorDomainName            "com.apple.webinspectord"

You always cast this as CFSTR; should these be defined as @"Remote.." and @"com.apple.webinspectord"?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:420
> +        process->enableRemoteInspectorIfNeeded();

Is the remote inspector needed for every web process? Should it be tied to a specific active tab when the user chose to enable (or show) the WebInspector?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #18)
> Comment on attachment 381756 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381756&action=review
> 
> >> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:219
> >> +        return;
> > 
> > How will we guarantee that all UIProcesses have access to this setting? Do we want to expose all of their sandboxes to this?
> 
> I'm not sure I understand the question here. It seems like this setting
> would be available to any WebKit client that links to a new build of WebKit.

I believe we need to extend the sandbox in the UI process with the right to read this preference. Additionally, the UI process' sandbox needs a new rule to allow creating the mach sandbox extension to 'com.apple.webinspector'.

All apps that wants to be inspectable will have to add these two rules to their sandbox. I will make sure these two rules are added to the sandbox profile which most apps are using.

Thanks for reviewing, all!

[Per Arne Vollan]

Created attachment 381822 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #19)
> Comment on attachment 381821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381821&action=review
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspectorConstants.h:123
> > +#define WIRRemoteInspectorDomainName            "com.apple.webinspectord"
> 
> You always cast this as CFSTR; should these be defined as @"Remote.." and
> @"com.apple.webinspectord"?
> 

Done. Declared as CFSTR in the latest patch.

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:420
> > +        process->enableRemoteInspectorIfNeeded();
> 
> Is the remote inspector needed for every web process? Should it be tied to a
> specific active tab when the user chose to enable (or show) the WebInspector?

I believe it needs to be enabled for every web process, since I believe it is possible to inspect many Service workers at the same time.

[Per Arne Vollan]

Created attachment 382869 [details]
Patch

[Per Arne Vollan]

rdar://problem/56372204

[Per Arne Vollan]

Created attachment 389973 [details]
Patch

[Per Arne Vollan]

Created attachment 390010 [details]
Patch

[Brent Fulgham]

Comment on attachment 390010 [details]
Patch

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

> Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:41
> +#if PLATFORM(IOS)

What about macOS? What about macCatalyst?

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:118
> +    static void setNeedMachSandboxExtension(bool needExtension) { needMachSandboxExtension = needExtension; }

Do we need to do a std::swap() here to benefit from the atomic type? I can't remember.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:242
> +#if PLATFORM(IOS)

Why can't we do this on macOS, too?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #27)
> Comment on attachment 390010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390010&action=review
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspector.cpp:41
> > +#if PLATFORM(IOS)
> 
> What about macOS? What about macCatalyst?
> 

Good point! I will change the code to include maOS as well. Some parts are iOS specific, like the setting to enable remote inspection and the notification name. Are you OK with fixing the remaining macOS specific things in a separate patch?

> > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:118
> > +    static void setNeedMachSandboxExtension(bool needExtension) { needMachSandboxExtension = needExtension; }
> 
> Do we need to do a std::swap() here to benefit from the atomic type? I can't
> remember.
> 

I am not 100% sure, but I think it should be safe to to an assignment here.

> > Source/JavaScriptCore/inspector/remote/RemoteInspector.h:242
> > +#if PLATFORM(IOS)
> 
> Why can't we do this on macOS, too?

I will enable this for macOS as well.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 390596 [details]
Patch

[Per Arne Vollan]

Created attachment 390653 [details]
Patch

[Per Arne Vollan]

Created attachment 390657 [details]
Patch

[Brent Fulgham]

Comment on attachment 390657 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:513
> +    for (size_t i = 0; i < pool->m_processes.size(); ++i) {

Can we use a modern C++ loop here?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:574
> +    CFNotificationCenterAddObserver(CFNotificationCenterGetDarwinNotifyCenter(), this, remoteWebInspectorEnabledCallback, static_cast<CFStringRef>(CFSTR(WIRServiceEnabledNotification)), nullptr, CFNotificationSuspensionBehaviorCoalesce);

Do we not need this for macOS?

> Source/WebKit/WebProcess/WebProcess.cpp:436
> +    Inspector::RemoteInspector::setNeedMachSandboxExtension(true);

Why isn't this needed on macOS, too? And what about MacCatalyst?

[Brent Fulgham]

Comment on attachment 390657 [details]
Patch

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

Thank you for reviewing it again. r=me

>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:574
>> +    CFNotificationCenterAddObserver(CFNotificationCenterGetDarwinNotifyCenter(), this, remoteWebInspectorEnabledCallback, static_cast<CFStringRef>(CFSTR(WIRServiceEnabledNotification)), nullptr, CFNotificationSuspensionBehaviorCoalesce);
> 
> Do we not need this for macOS?

Oh -- I see, you already answered this. Never mind.

>> Source/WebKit/WebProcess/WebProcess.cpp:436
>> +    Inspector::RemoteInspector::setNeedMachSandboxExtension(true);
> 
> Why isn't this needed on macOS, too? And what about MacCatalyst?

Never mind!

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #33)
> Comment on attachment 390657 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390657&action=review
> 
> Thank you for reviewing it again. r=me
> 
> >> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:574
> >> +    CFNotificationCenterAddObserver(CFNotificationCenterGetDarwinNotifyCenter(), this, remoteWebInspectorEnabledCallback, static_cast<CFStringRef>(CFSTR(WIRServiceEnabledNotification)), nullptr, CFNotificationSuspensionBehaviorCoalesce);
> > 
> > Do we not need this for macOS?
> 
> Oh -- I see, you already answered this. Never mind.
> 
> >> Source/WebKit/WebProcess/WebProcess.cpp:436
> >> +    Inspector::RemoteInspector::setNeedMachSandboxExtension(true);
> > 
> > Why isn't this needed on macOS, too? And what about MacCatalyst?
> 
> Never mind!

Thanks for reviewing!

[Per Arne Vollan]

Committed r256742: <https://trac.webkit.org/changeset/256742/webkit>