Bug 238255 – Update Sandbox profiles for system content path

• Component: Platform
• Status: RESOLVED FIXED

[Michael Saboff]

We need to add sandbox rules when compiling with the system content path.

[Michael Saboff]

<rdar://90343926>

[Michael Saboff]

Created attachment 455508 [details]
Work in Progress Patch

[Michael Saboff]

Created attachment 455512 [details]
Updated Work in Progress Patch

[David Quesada]

Comment on attachment 455512 [details]
Updated Work in Progress Patch

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

> Source/WebKit/DerivedSources.make:368
> +	com.apple.WebKit.WebContent.sb

Nit: You could keep the \ here so this line doesn't need to change now, or if any other profiles are added in the future.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:858
> +#include <WebKitAdditions/SystemContent-ios.sb>

Is this `#include` intentionally placed within the `(with-elevated-precedence` block? It doesn't look like any of the other profiles do it that way.

[Per Arne Vollan]

Comment on attachment 455512 [details]
Updated Work in Progress Patch

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

> Source/WebKit/DerivedSources.make:354
> +ifeq ($(USE_SYSTEM_CONTENT_PATH),YES)
> +	SANDBOX_DEFINES = -DUSE_SYSTEM_CONTENT_PATH=1 -DSYSTEM_CONTENT_PATH=$(SYSTEM_CONTENT_PATH)
> +endif

Is this strictly needed? The preprocessing step includes "wtf/Platform.h".

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:857
> +(with-elevated-precedence

Is this strictly needed?

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:858
> +#include <WebKitAdditions/SystemContent-ios.sb>

I think we should make sure there are only defines in this sandbox include file.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:860
> +    (allow file-read* (with telemetry)

This could generate a lot of telemetry. I think it may be good to avoid that.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:863
> +    (allow file-map-executable (with telemetry)

Ditto.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:869
> +    (allow file-issue-extension (with telemetry)

Ditto.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb.in:798
> +(with-elevated-precedence

This may not be needed.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebAuthn.sb.in:452
> +(with-elevated-precedence

Ditto.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:1662
> +(with-elevated-precedence

Ditto.

> Source/WebKit/Shared/Sandbox/preferences.sb:54
> +#if USE(SYSTEM_CONTENT_PATH)
> +#include <WebKitAdditions/SystemContent-macos.sb>
> +#endif

This is sandbox include file for preference related things. Perhaps we could move this include to the WebKit sandboxes?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1101
> +#if USE(SYSTEM_CONTENT_PATH)
> +(allow-read-directory-and-issue-read-extensions
> +    (apply read-directory-and-issue-read-extension-secondary-paths)
> +#endif

Should this also be added to the Network and GPU process on Mac?

[Per Arne Vollan]

Comment on attachment 455512 [details]
Updated Work in Progress Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:15757
> -			shellScript = "echo \"Preprocessing sandbox\"\nScripts/generate-derived-sources.sh sandbox-profiles-ios\ncp ${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit/com.apple.WebKit.WebContent.sb ${DSTROOT}/${INSTALL_PATH}\n";
> +			shellScript = "echo \"Preprocessing sandbox\"\nScripts/generate-derived-sources.sh sandbox-profiles-ios\ncp ${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit/com.apple.WebKit.adattributiond.sb ${DSTROOT}/${INSTALL_PATH}\ncp ${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit/com.apple.WebKit.GPU.sb ${DSTROOT}/${INSTALL_PATH}\ncp ${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit/com.apple.WebKit.Networking.sb ${DSTROOT}/${INSTALL_PATH}\ncp ${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit/com.apple.WebKit.WebAuth.sb ${DSTROOT}/${INSTALL_PATH}\ncp ${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit/com.apple.WebKit.WebContent.sb ${DSTROOT}/${INSTALL_PATH}\n";

I think you also can remove the Copy Files step in the Sandbox Profiles target in WebKit now.

[Saam Barati]

Comment on attachment 455512 [details]
Updated Work in Progress Patch

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

> Source/WebKit/WebAuthnProcess/mac/com.apple.WebKit.WebAuthnProcess.sb.in:316
> +#if USE(SYSTEM_CONTENT_PATH)
> +(allow-read-directory-and-issue-read-extensions
> +    (apply read-directory-and-issue-read-extension-secondary-paths)
> +#endif

I think you're missing a closing paren here

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1100
> +(allow-read-directory-and-issue-read-extensions
> +    (apply read-directory-and-issue-read-extension-secondary-paths)

ditto about missing paren.

[Michael Saboff]

Created attachment 455540 [details]
Patch with updates from reviews

[Per Arne Vollan]

Comment on attachment 455540 [details]
Patch with updates from reviews

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

> Source/WebKit/DerivedSources.make:354
> +ifeq ($(USE_SYSTEM_CONTENT_PATH),YES)
> +	SANDBOX_DEFINES = -DUSE_SYSTEM_CONTENT_PATH=1 -DSYSTEM_CONTENT_PATH=$(SYSTEM_CONTENT_PATH)
> +endif

Perhaps this can be omitted since the preprocess step includes wtf/Platform.h?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1105
> +
> +#if USE(SYSTEM_CONTENT_PATH)
> +(allow-read-directory-and-issue-read-extensions
> +    (apply read-directory-and-issue-read-extension-secondary-paths))
> +#endif

Do you also need to add this to the other WebKit sandboxes on Mac?

[Michael Saboff]

(In reply to Per Arne Vollan from comment #9)
> Comment on attachment 455540 [details]
> Patch with updates from reviews
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455540&action=review
> 
> > Source/WebKit/DerivedSources.make:354
> > +ifeq ($(USE_SYSTEM_CONTENT_PATH),YES)
> > +	SANDBOX_DEFINES = -DUSE_SYSTEM_CONTENT_PATH=1 -DSYSTEM_CONTENT_PATH=$(SYSTEM_CONTENT_PATH)
> > +endif
> 
> Perhaps this can be omitted since the preprocess step includes
> wtf/Platform.h?

Platform.h doesn't define the *SYSTEM_CONTENT_PATH values.
 
> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1105
> > +
> > +#if USE(SYSTEM_CONTENT_PATH)
> > +(allow-read-directory-and-issue-read-extensions
> > +    (apply read-directory-and-issue-read-extension-secondary-paths))
> > +#endif
> 
> Do you also need to add this to the other WebKit sandboxes on Mac?

I don't think so.  The value read-directory-and-issue-read-extension-secondary-paths is used for the system content path of WebInspectorUI.framework and it is only needed in WebProcess.sb.in and WebAuthnProcess.sb.in.  Maybe we should choose a different name for the value, say webinspectorui-secondary-path?

[Per Arne Vollan]

Comment on attachment 455540 [details]
Patch with updates from reviews

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

R=me.

>>> Source/WebKit/DerivedSources.make:354
>>> +endif
>> 
>> Perhaps this can be omitted since the preprocess step includes wtf/Platform.h?
> 
> Platform.h doesn't define the *SYSTEM_CONTENT_PATH values.

Ah, I see, thanks!

>>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1105
>>> +#endif
>> 
>> Do you also need to add this to the other WebKit sandboxes on Mac?
> 
> I don't think so.  The value read-directory-and-issue-read-extension-secondary-paths is used for the system content path of WebInspectorUI.framework and it is only needed in WebProcess.sb.in and WebAuthnProcess.sb.in.  Maybe we should choose a different name for the value, say webinspectorui-secondary-path?

I think that would make it clearer, but not strictly necessary.

[Saam Barati]

Comment on attachment 455540 [details]
Patch with updates from reviews

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

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:871
> +            (apply subpath issue-extension-secondary-paths)))

nit: This is over-indented.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:1666
> +       (apply subpath "secondary-framework-and-dylib-paths))

style nit: this is over-indented

> Source/WebKit/WebAuthnProcess/mac/com.apple.WebKit.WebAuthnProcess.sb.in:319
> +(allow-read-directory-and-issue-read-extensions
> +    (apply read-directory-and-issue-read-extension-secondary-paths))

Should this be:

`(apply allow-read-directory-and-issue-read-extensions read-directory-and-issue-read-extension-secondary-paths)`

?

[Michael Saboff]

Comment on attachment 455540 [details]
Patch with updates from reviews

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

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:860
> +(allow file-read* file-read-existence-secondary-paths
> +    (apply subpath file-read-secondary-paths))

This should really be:

(allow file-read* file-read-existence
    (apply subpath file-read-existence-secondary-paths))

There are a total of 5 places where this needs to be done.

[Michael Saboff]

Created attachment 455569 [details]
Patch for Landing

[Per Arne Vollan]

Comment on attachment 455569 [details]
Patch for Landing

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

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.GPU.sb.in:866
> +(allow-read-and-issue-generic-extensions
> +    (apply subpath issue-extension-secondary-paths))

I think this should be:

(allow file-read* file-issue-extension
    (apply subpath issue-extension-secondary-paths))

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb.in:805
> +(allow-read-and-issue-generic-extensions
> +    (apply subpath issue-extension-secondary-paths))

Ditto.

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:1669
> +(allow-read-and-issue-generic-extensions
> +    (apply subpath issue-extension-secondary-paths))

Ditto.

> Source/WebKit/WebAuthnProcess/mac/com.apple.WebKit.WebAuthnProcess.sb.in:319
> +(apply allow-read-directory-and-issue-read-extensions
> +    read-directory-and-issue-read-extension-secondary-paths)

I think this should be:

(allow file-read* file-issue-extension
    (apply subpath read-directory-and-issue-read-extension-secondary-paths))

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:1104
> +(apply allow-read-directory-and-issue-read-extensions
> +    read-directory-and-issue-read-extension-secondary-paths)

Ditto.

[Per Arne Vollan]

Comment on attachment 455569 [details]
Patch for Landing

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

> Source/WebKit/DerivedSources.make:376
> +	grep -o '^[^;]*' $< | $(CC) $(SDK_FLAGS) $(TARGET_TRIPLE_FLAGS) $(SANDBOX_DEFINES) $(TEXT_PREPROCESSOR_FLAGS) $(FRAMEWORK_FLAGS) $(HEADER_FLAGS) -include "wtf/Platform.h" - > $@

I also think you need to make sure the preprocessed sandboxes on iOS are not overwriting the macOS sandboxes in DerivedSources. Perhaps they should go into a separate directory?

[Michael Saboff]

Committed r291814 (248841@trunk): <https://commits.webkit.org/248841@trunk>