Bug 238255

Summary: Update Sandbox profiles for system content path
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: PlatformAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: david_quesada, pvollan, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Work in Progress Patch
msaboff: commit-queue-
Updated Work in Progress Patch
msaboff: commit-queue-
Patch with updates from reviews
pvollan: review+
Patch for Landing none

Description Michael Saboff 2022-03-23 06:16:41 PDT 
We need to add sandbox rules when compiling with the system content path.
Comment 1 Michael Saboff 2022-03-23 06:16:55 PDT 
<rdar://90343926>
Comment 2 Michael Saboff 2022-03-23 09:44:49 PDT 
Created attachment 455508 [details]
Work in Progress Patch
Comment 3 Michael Saboff 2022-03-23 10:11:26 PDT 
Created attachment 455512 [details]
Updated Work in Progress Patch
Comment 4 David Quesada 2022-03-23 11:04:42 PDT 
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.
Comment 5 Per Arne Vollan 2022-03-23 11:21:42 PDT 
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?
Comment 6 Per Arne Vollan 2022-03-23 11:23:19 PDT 
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.
Comment 7 Saam Barati 2022-03-23 11:43:00 PDT 
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.
Comment 8 Michael Saboff 2022-03-23 13:12:22 PDT 
Created attachment 455540 [details]
Patch with updates from reviews
Comment 9 Per Arne Vollan 2022-03-23 13:28:04 PDT 
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?
Comment 10 Michael Saboff 2022-03-23 14:20:46 PDT 
(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?
Comment 11 Per Arne Vollan 2022-03-23 14:42:22 PDT 
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.
Comment 12 Saam Barati 2022-03-23 14:53:35 PDT 
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)`

?
Comment 13 Michael Saboff 2022-03-23 15:11:27 PDT 
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.
Comment 14 Michael Saboff 2022-03-23 15:35:48 PDT 
Created attachment 455569 [details]
Patch for Landing
Comment 15 Per Arne Vollan 2022-03-23 15:49:21 PDT 
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.
Comment 16 Per Arne Vollan 2022-03-24 07:22:12 PDT 
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?
Comment 17 Michael Saboff 2022-03-24 14:39:15 PDT 
Committed r291814 (248841@trunk): <https://commits.webkit.org/248841@trunk>