Bug 165818 - [Mac][WK2] Tighten Keychain directory access
Summary: [Mac][WK2] Tighten Keychain directory access
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-13 12:48 PST by Brent Fulgham
Modified: 2016-12-14 10:29 PST (History)
3 users (show)

See Also:


Attachments
Patch (10.92 KB, patch)
2016-12-13 12:51 PST, Brent Fulgham
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-12-13 12:48:41 PST
The existing Sandbox rules for the various WebKit processes are overly permissive. We should tighten them down to just the handful of operations we really need:

We should limit our access to: file-read-data, file-read-metadata, and file-write-data.

We should also deny access to newer keychains (with UUID-based names) since those are not meant to be used by user processes.
Comment 1 Brent Fulgham 2016-12-13 12:49:21 PST
<rdar://problem/16863857>
Comment 2 Brent Fulgham 2016-12-13 12:51:53 PST
Created attachment 297036 [details]
Patch
Comment 3 BJ Burg 2016-12-13 13:58:45 PST
Comment on attachment 297036 [details]
Patch

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

> Source/WebKit2/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:41
> +;;; UUID of the form: XXXXXXXX-XXXX-XXXX--XXXX-XXXXXXXXXXXX

All of this profile text is going to get embedded in the binary. Should it?

> Source/WebKit2/PluginProcess/mac/com.apple.WebKit.plugin-common.sb.in:78
> +;;; That's 8X-4X-4X-4X-12X; where X = "[0-9A-F]", length(X) = 8

Ditto.

> Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:48
> +;;; That's 8X-4X-4X-4X-12X; where X = "[0-9A-F]", length(X) = 8

Ditto.
Comment 4 Brent Fulgham 2016-12-13 14:51:08 PST
Comment on attachment 297036 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:41
>> +;;; UUID of the form: XXXXXXXX-XXXX-XXXX--XXXX-XXXXXXXXXXXX
> 
> All of this profile text is going to get embedded in the binary. Should it?

That's okay -- I'll pull it out.

>> Source/WebKit2/PluginProcess/mac/com.apple.WebKit.plugin-common.sb.in:78
>> +;;; That's 8X-4X-4X-4X-12X; where X = "[0-9A-F]", length(X) = 8
> 
> Ditto.

Ditto.

>> Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:48
>> +;;; That's 8X-4X-4X-4X-12X; where X = "[0-9A-F]", length(X) = 8
> 
> Ditto.

Ditto.
Comment 5 Brent Fulgham 2016-12-13 14:57:02 PST
Committed r209779: <http://trac.webkit.org/changeset/209779>
Comment 6 Brent Fulgham 2016-12-14 09:01:58 PST
This change broke the Keygen element (e.g., http/tests/misc/submit-post-keygen.html).

WebProcess needs the file-write-create permission for the Keychains directory.
Comment 7 Brent Fulgham 2016-12-14 09:18:49 PST
Committed r209806: <http://trac.webkit.org/changeset/209806>
Comment 8 Brent Fulgham 2016-12-14 10:29:07 PST
Please note, three changes are needed to integrate this sandbox change:

Committed r209779: <http://trac.webkit.org/changeset/209779>
Committed r209806: <http://trac.webkit.org/changeset/209806>
Committed r209814: <http://trac.webkit.org/changeset/209814>