Bug 184428

Summary:

Remove access to keychain from the WebContent process

Product:

WebKit

Reporter:

Jiewen Tan <jiewen_tan>

Component:

WebCore Misc.

Assignee:

Jiewen Tan <jiewen_tan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, bfulgham, cdumez, commit-queue, conrad_shultz, ews-watchlist, jiewen_tan, mitz, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Bug Depends on:

Bug Blocks:

185537

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Part 1	bfulgham: review+
Part 1 for landing	none
Part 2	none
Part 2	none
Part 3 WIP	ews-watchlist: commit-queue-
Archive of layout-test-results from ews106 for mac-sierra-wk2	none
Part 3	none
Archive of layout-test-results from ews202 for win-future	none

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.