Bug 163710

Summary:

Keychain Access in WebKit should be limited to a single process (macOS)

Product:

WebKit

Reporter:

Pranjal Jumde <pranjal.jumde>

Component:

WebKit Misc.

Assignee:

Pranjal Jumde <pranjal.jumde>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aestes, bfulgham, commit-queue, dbates, ddkilzer

Priority:

Keywords:

InRadar

Version:

Safari 10

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=170074

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Description Pranjal Jumde 2016-10-19 17:05:21 PDT

With the upcoming changes in the networking stack, we can remove Keychain access from the Networking and the WebContent process.

Comment 1 Pranjal Jumde 2016-10-19 17:20:04 PDT

Created attachment 292132 [details]
Patch

Comment 2 Brent Fulgham 2016-10-19 17:23:24 PDT

Comment on attachment 292132 [details]
Patch

Looks good, but we have to make sure we don't apply these changes to builds that target older operating systems.

Comment 3 Pranjal Jumde 2016-10-19 18:35:45 PDT

Created attachment 292141 [details]
Patch

Comment 4 Sam Weinig 2016-10-19 23:12:33 PDT

Comment on attachment 292141 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        * NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:
> +        * WebProcess/com.apple.WebProcess.sb.in:

Please add more information in this change log.

> Source/WebKit2/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:141
> +(allow file-read*
> +       (subpath "/private/var/db/mds")
> +       (literal "/private/var/db/DetachedSignatures")
> +       (literal "/Library/Preferences/com.apple.crypto.plist")
> +       (literal "/Library/Preferences/com.apple.security.plist")
> +       (literal "/Library/Preferences/com.apple.security.common.plist")
> +       (literal "/Library/Preferences/com.apple.security.revocation.plist")
> +       (home-literal "/Library/Application Support/SyncServices/Local/ClientsWithChanges/com.apple.Keychain")
> +       (home-literal "/Library/Preferences/com.apple.security.plist")
> +       (home-literal "/Library/Preferences/com.apple.security.revocation.plist"))

Is this defining some of these rules twice if __MAC_OS_X_VERSION_MIN_REQUIRED < 101200 is true?

Comment 5 Pranjal Jumde 2016-10-20 11:32:25 PDT

Created attachment 292229 [details]
Patch

Comment 6 Brent Fulgham 2016-10-21 09:16:14 PDT

Comment on attachment 292229 [details]
Patch

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

> Source/WebKit2/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:142
> +

This seems like a lot of duplication. Can't you just have the #ifdef check just by about the one line related to "/Library/Keychains" ?

> Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:245
> +#endif

Ditto the above comment.

Comment 7 Pranjal Jumde 2016-10-24 15:53:26 PDT

Created attachment 292672 [details]
Patch

Comment 8 WebKit Commit Bot 2016-11-14 13:05:29 PST

Comment on attachment 292672 [details]
Patch

Clearing flags on attachment: 292672

Committed r208702: <http://trac.webkit.org/changeset/208702>

Comment 9 WebKit Commit Bot 2016-11-14 13:05:34 PST

All reviewed patches have been landed.  Closing bug.

Comment 10 Brent Fulgham 2016-11-14 13:41:58 PST

Follow up:

Bumped version to avoid breaking STP and nightly users.

Committed r208707: <http://trac.webkit.org/changeset/208707>