Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to sandbox version 3 on iOS and macOS #6864

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented Nov 28, 2022

129d762

Upgrade to sandbox version 3 on iOS and macOS
https://bugs.webkit.org/show_bug.cgi?id=248399
rdar://102719029

Reviewed by Brent Fulgham.

Upgrade to sandbox version 3 on iOS and macOS. This patch should not introduce any behavior change, since
the resources with implicit access in version 1 has been explicitly allowed. Also, this patch adds more
sandbox include files to be able to share common rules.

* Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in:
* Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:
* Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:
* Source/WebKit/Shared/Sandbox/iOS/common.sb
* Source/WebKit/Shared/Sandbox/macOS/common.sb:
* Source/WebKit/Shared/Sandbox/util.sb
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:
* Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:

Canonical link: https://commits.webkit.org/257272@main

509c22f

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe
βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ api-mac   πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim ❌ πŸ§ͺ mac-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@pvollan pvollan requested a review from cdumez as a code owner November 28, 2022 16:28
@pvollan pvollan self-assigned this Nov 28, 2022
@pvollan pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Nov 28, 2022
@pvollan pvollan marked this pull request as draft November 28, 2022 16:28
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 28, 2022
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 28, 2022
@pvollan pvollan force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from a76df67 to 0210043 Compare November 28, 2022 19:34
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 28, 2022
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 29, 2022
@pvollan pvollan force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from 0210043 to 4cca556 Compare November 29, 2022 09:17
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 29, 2022
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 29, 2022
@pvollan pvollan force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from 4cca556 to e4481f1 Compare November 29, 2022 10:23
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 29, 2022
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 29, 2022
@pvollan pvollan force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from e4481f1 to 5dabc34 Compare November 29, 2022 11:36
@pvollan pvollan force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from 5dabc34 to f5ab65a Compare November 29, 2022 18:11
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 30, 2022
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 30, 2022
@pvollan pvollan force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from f5ab65a to f3ab20e Compare November 30, 2022 12:42
@pvollan pvollan requested a review from szewai November 30, 2022 12:42
@pvollan pvollan marked this pull request as ready for review November 30, 2022 12:43
@pvollan pvollan force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from f3ab20e to f8c5031 Compare November 30, 2022 13:55
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 30, 2022
Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very nice cleanup! r=me

@@ -375,3 +375,8 @@
#if PLATFORM(COCOA) && (HAVE(CGSTYLE_CREATE_SHADOW2) || HAVE(CGSTYLE_COLORMATRIX_BLUR))
#define USE_GRAPHICS_CONTEXT_FILTERS 1
#endif

#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 140000) \
|| ((PLATFORM(IOS) || PLATFORM(MACCATALYST)) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 170000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am always confused by when to use PLATFORM(IOS_FAMILY) versus PLATFORM(IOS). I believe that this rule means that USE_SANDBOX_VERSION_3 will be undefined for watchOS and tvOS. Is this your intention?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am always confused by when to use PLATFORM(IOS_FAMILY) versus PLATFORM(IOS). I believe that this rule means that USE_SANDBOX_VERSION_3 will be undefined for watchOS and tvOS. Is this your intention?

That is a good point. The intention is for also watchOS and tvOS to have version 3. I have removed this from the latest patch, and will enable in a separate patch.

Thanks for reviewing!


(with-filter (mac-policy-name "Sandbox")
(allow system-mac-syscall (mac-syscall-number 5)))
#endif

;;;
;;; The following rules were originally contained in 'common.sb'. We are duplicating them here so we can
;;; remove unneeded sandbox extensions.
;;;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this comment should move to Shared/Sandbox/iOS/common.sb, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still quite a few rules that were not moved to Shared/Sandbox/iOS/common.sb in this patch, so I think we may leave the comment for now. However, we should move all the common rules to the new file including the comment. I will address that in a separate patch.

Thanks for reviewing!


(allow system-privilege (with grant)
(require-all
(privilege-id PRIV_NET_PRIVILEGED_SOCKET_DELEGATE)
(require-entitlement "com.apple.private.network.socket-delegate")))

;; Silence spurious logging due to rdar://20117923 and rdar://72366475
(deny system-privilege (privilege-id PRIV_GLOBAL_PROC_INFO) (with no-report))

;;;
;;; The following rules were originally contained in 'common.sb'. We are duplicating them here so we can
;;; remove unneeded sandbox extensions.
;;;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: Perhaps move the comment to Shared/Sandbox/iOS/common.sb as well?

(deny system-privilege (privilege-id PRIV_GLOBAL_PROC_INFO) (with no-report))
#if USE(SANDBOX_VERSION_3)
(allow dynamic-code-generation)
#endif

;;;
;;; The following rules were originally contained in 'common.sb'. We are duplicating them here so we can
;;; remove unneeded sandbox extensions.
;;;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: move comment to Shared/Sandbox/iOS/common.sb.

@pvollan pvollan added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged merge-queue Applied to send a pull request to merge-queue labels Dec 1, 2022
@pvollan pvollan force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from f8c5031 to 509c22f Compare December 1, 2022 09:24
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 2, 2022
@pvollan pvollan added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Dec 2, 2022
https://bugs.webkit.org/show_bug.cgi?id=248399
rdar://102719029

Reviewed by Brent Fulgham.

Upgrade to sandbox version 3 on iOS and macOS. This patch should not introduce any behavior change, since
the resources with implicit access in version 1 has been explicitly allowed. Also, this patch adds more
sandbox include files to be able to share common rules.

* Source/WebKit/GPUProcess/mac/com.apple.WebKit.GPUProcess.sb.in:
* Source/WebKit/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:
* Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb.in:
* Source/WebKit/Shared/Sandbox/iOS/common.sb
* Source/WebKit/Shared/Sandbox/macOS/common.sb:
* Source/WebKit/Shared/Sandbox/util.sb
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:
* Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:

Canonical link: https://commits.webkit.org/257272@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/Upgrade-to-sandbox-version-3-on-iOS-and-macOS branch from 509c22f to 129d762 Compare December 2, 2022 07:00
@webkit-commit-queue
Copy link
Collaborator

Committed 257272@main (129d762): https://commits.webkit.org/257272@main

Reviewed commits have been landed. Closing PR #6864 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 129d762 into WebKit:main Dec 2, 2022
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
5 participants