184428 – Remove access to keychain from the WebContent process

RESOLVED FIXED 184428

Remove access to keychain from the WebContent process

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

Summary Remove access to keychain from the WebContent process

Jiewen Tan

Reported 2018-04-09 14:40:46 PDT

Remove access to keychain from the WebContent process.

Attachments
Patch (1.49 KB, patch) 2018-04-09 14:48 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (4.30 KB, patch) 2018-04-17 21:17 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (4.69 KB, patch) 2018-04-18 13:55 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Patch (4.71 KB, patch) 2018-04-18 14:02 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Part 1 (4.79 KB, patch) 2018-04-18 14:48 PDT, Jiewen Tan	bfulgham: review+	Details Formatted Diff Diff
Part 1 for landing (4.75 KB, patch) 2018-04-19 15:08 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Part 2 (51.47 KB, patch) 2018-04-25 01:40 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Part 2 (46.93 KB, patch) 2018-04-25 11:44 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Part 3 WIP (3.75 KB, patch) 2018-04-25 13:09 PDT, Jiewen Tan	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (749.72 KB, application/zip) 2018-04-25 13:57 PDT, EWS Watchlist	no flags	Details
Part 3 (3.75 KB, patch) 2018-04-25 15:11 PDT, Jiewen Tan	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews202 for win-future (12.69 MB, application/zip) 2018-04-25 16:54 PDT, EWS Watchlist	no flags	Details
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Jiewen Tan

Comment 1 2018-04-09 14:48:12 PDT

Created attachment 337544 [details] Patch

Jiewen Tan

Comment 2 2018-04-09 14:50:15 PDT

<rdar://problem/13150903>

Jiewen Tan

Comment 3 2018-04-17 21:17:58 PDT

Created attachment 338193 [details] Patch

Conrad Shultz

Comment 4 2018-04-17 22:11:17 PDT

Comment on attachment 338193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338193&action=review > Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:342 > + SecIdentityCopyCertificate((SecIdentityRef)object, &certificate); Can this return an error? If so, `certificate` will be nil and we'll crash trying to add it to the `clientCertificates` array.

Jiewen Tan

Comment 5 2018-04-18 13:27:26 PDT

Comment on attachment 338193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338193&action=review Thanks Conrad for reviewing the patch. >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:342 >> + SecIdentityCopyCertificate((SecIdentityRef)object, &certificate); > > Can this return an error? If so, `certificate` will be nil and we'll crash trying to add it to the `clientCertificates` array. Fixed.

Jiewen Tan

Comment 6 2018-04-18 13:55:07 PDT

Created attachment 338258 [details] Patch

Conrad Shultz

Comment 7 2018-04-18 13:59:00 PDT

Comment on attachment 338258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338258&action=review LGTM, but you'll need to round up a proper reviewer. > Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:342 > + OSStatus status = SecIdentityCopyCertificate((SecIdentityRef)object, &certificate); I think this should explicitly use `if (status != errSecSuccess)`

Jiewen Tan

Comment 8 2018-04-18 14:01:39 PDT

Comment on attachment 338258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338258&action=review >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:342 >> + OSStatus status = SecIdentityCopyCertificate((SecIdentityRef)object, &certificate); > > I think this should explicitly use `if (status != errSecSuccess)` Fixed.

Jiewen Tan

Comment 9 2018-04-18 14:02:17 PDT

Created attachment 338261 [details] Patch

Conrad Shultz

Comment 10 2018-04-18 14:04:22 PDT

Comment on attachment 338261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338261&action=review > Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:342 > + OSStatus status = SecIdentityCopyCertificate((SecIdentityRef)object, &certificate); One more thing: do we need to `CFRelease(certificate)` when finished?

Jiewen Tan

Comment 11 2018-04-18 14:23:03 PDT

Comment on attachment 338261 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338261&action=review >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:342 >> + OSStatus status = SecIdentityCopyCertificate((SecIdentityRef)object, &certificate); > > One more thing: do we need to `CFRelease(certificate)` when finished? Honestly, I don't know. Does [NSMutableArray - addObject] copy something? If so, I could use a local variable within the block with adoptCF to maintain the lifetime. Otherwise, I might need to escalate that variable to the function block. Sorry, I am still not good at managing objc objects lifetime in WebKit..

Jiewen Tan

Comment 12 2018-04-18 14:48:33 PDT

Created attachment 338265 [details] Part 1

Conrad Shultz

Comment 13 2018-04-18 15:37:25 PDT

(In reply to Jiewen Tan from comment #11) > Comment on attachment 338261 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338261&action=review > > >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:342 > >> + OSStatus status = SecIdentityCopyCertificate((SecIdentityRef)object, &certificate); > > > > One more thing: do we need to `CFRelease(certificate)` when finished? > > Honestly, I don't know. Does [NSMutableArray - addObject] copy something? If > so, I could use a local variable within the block with adoptCF to maintain > the lifetime. Otherwise, I might need to escalate that variable to the > function block. > Sorry, I am still not good at managing objc objects lifetime in WebKit.. Your use of RetainPtr looks correct!

Brent Fulgham

Comment 14 2018-04-19 13:59:06 PDT

Comment on attachment 338265 [details] Part 1 View in context: https://bugs.webkit.org/attachment.cgi?id=338265&action=review Looks good! > Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:340 > + if (CFGetTypeID(object) == SecIdentityGetTypeID()) { This might be clearer as an early return: if (CFGetTypeID(object) != SecIdentityGetTypeID()) { [clientCertificates addObject:object]; continue; } > Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:354 > + [clientCertificates addObject:object]; .. Then this line would go away (move above line 341).

Jiewen Tan

Comment 15 2018-04-19 14:56:30 PDT

Comment on attachment 338265 [details] Part 1 View in context: https://bugs.webkit.org/attachment.cgi?id=338265&action=review Thanks Brent for r+ this patch. >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:340 >> + if (CFGetTypeID(object) == SecIdentityGetTypeID()) { > > This might be clearer as an early return: > > if (CFGetTypeID(object) != SecIdentityGetTypeID()) { > [clientCertificates addObject:object]; > continue; > } Sure. >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:354 >> + [clientCertificates addObject:object]; > > .. Then this line would go away (move above line 341). Fixed.

Jiewen Tan

Comment 16 2018-04-19 15:08:15 PDT

Created attachment 338364 [details] Part 1 for landing

WebKit Commit Bot

Comment 17 2018-04-19 16:52:03 PDT

Comment on attachment 338364 [details] Part 1 for landing Clearing flags on attachment: 338364 Committed r230827: <https://trac.webkit.org/changeset/230827>

Jiewen Tan

Comment 18 2018-04-25 01:40:06 PDT

Created attachment 338714 [details] Part 2

Jiewen Tan

Comment 19 2018-04-25 11:44:25 PDT

Created attachment 338763 [details] Part 2

Jiewen Tan

Comment 20 2018-04-25 13:09:43 PDT

Created attachment 338775 [details] Part 3 WIP

EWS Watchlist

Comment 21 2018-04-25 13:57:07 PDT

Comment on attachment 338775 [details] Part 3 WIP Attachment 338775 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7457673 Number of test failures exceeded the failure limit.

EWS Watchlist

Comment 22 2018-04-25 13:57:08 PDT

Created attachment 338789 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Brent Fulgham

Comment 23 2018-04-25 14:40:08 PDT

Comment on attachment 338763 [details] Part 2 View in context: https://bugs.webkit.org/attachment.cgi?id=338763&action=review Looks good! > Source/WebKit/ChangeLog:13 > + macOS Keychain. ... such that WebKitTestRunner tests will not modify the underlying key store (e.g., the macOS Keychain). > Source/WebKitLegacy/mac/ChangeLog:13 > + macOS Keychain. Ditto my comments above.

Jiewen Tan

Comment 24 2018-04-25 14:59:22 PDT

Comment on attachment 338763 [details] Part 2 View in context: https://bugs.webkit.org/attachment.cgi?id=338763&action=review Thanks Brent for r+ my patch. >> Source/WebKit/ChangeLog:13 >> + macOS Keychain. > > ... such that WebKitTestRunner tests will not modify the underlying key store (e.g., the macOS Keychain). Fixed. >> Source/WebKitLegacy/mac/ChangeLog:13 >> + macOS Keychain. > > Ditto my comments above. Fixed.

Jiewen Tan

Comment 25 2018-04-25 15:10:10 PDT

Committed r231024: <https://trac.webkit.org/changeset/231024>

Jiewen Tan

Comment 26 2018-04-25 15:11:31 PDT

Reopening to attach new patch.

Jiewen Tan

Comment 27 2018-04-25 15:11:33 PDT

Created attachment 338807 [details] Part 3

EWS Watchlist

Comment 28 2018-04-25 16:54:10 PDT

Comment on attachment 338763 [details] Part 2 Attachment 338763 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7460700 New failing tests: http/tests/misc/submit-post-keygen.html

EWS Watchlist

Comment 29 2018-04-25 16:54:21 PDT

Created attachment 338826 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Jiewen Tan

Comment 30 2018-04-25 17:44:15 PDT

Filed Bug 185018 to keep track of the Win implementations. Test expectations are updated in: Committed r231036: <https://trac.webkit.org/changeset/231036>

Brent Fulgham

Comment 31 2018-04-26 09:41:31 PDT

Comment on attachment 338763 [details] Part 2 Removing flags now that this part has landed.

Brent Fulgham

Comment 32 2018-04-26 09:43:40 PDT

Comment on attachment 338807 [details] Part 3 View in context: https://bugs.webkit.org/attachment.cgi?id=338807&action=review Awesome! r=me. > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:603 > + (literal "/private/var/db/DetachedSignatures")) Can these changes be made to the iOS-specific files (WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb)? The Keygen and other changes you made should be cross-platform and apply to iOS, too!

Jiewen Tan

Comment 33 2018-04-26 10:50:49 PDT

Comment on attachment 338807 [details] Part 3 View in context: https://bugs.webkit.org/attachment.cgi?id=338807&action=review Thanks Brent for r+ this patch. >> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:603 >> + (literal "/private/var/db/DetachedSignatures")) > > Can these changes be made to the iOS-specific files (WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb)? The Keygen and other changes you made should be cross-platform and apply to iOS, too! Sure, I was not aware that the iOS has its own sandbox profiles.

Jiewen Tan

Comment 34 2018-04-26 11:07:16 PDT

Comment on attachment 338807 [details] Part 3 View in context: https://bugs.webkit.org/attachment.cgi?id=338807&action=review >>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:603 >>> + (literal "/private/var/db/DetachedSignatures")) >> >> Can these changes be made to the iOS-specific files (WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb)? The Keygen and other changes you made should be cross-platform and apply to iOS, too! > > Sure, I was not aware that the iOS has its own sandbox profiles. I didn't discover anything similar to the above in the iOS-specific sandbox profile. Am I missing something?

WebKit Commit Bot

Comment 35 2018-04-26 11:20:56 PDT

Comment on attachment 338807 [details] Part 3 Clearing flags on attachment: 338807 Committed r231057: <https://trac.webkit.org/changeset/231057>

WebKit Commit Bot

Comment 36 2018-04-26 11:20:58 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Jiewen Tan

Reported

2018-04-09 14:40 PDT

Modified

2018-05-10 17:28 PDT History

CC List

10 users Show

URL

Keywords InRadar

Depends on

Blocks

185537

Dependencies

tree graph