Remove access to keychain from the WebContent process.
Created attachment 337544 [details] Patch
<rdar://problem/13150903>
Created attachment 338193 [details] Patch
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.
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.
Created attachment 338258 [details] Patch
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)`
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.
Created attachment 338261 [details] Patch
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?
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..
Created attachment 338265 [details] Part 1
(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!
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).
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.
Created attachment 338364 [details] Part 1 for landing
Comment on attachment 338364 [details] Part 1 for landing Clearing flags on attachment: 338364 Committed r230827: <https://trac.webkit.org/changeset/230827>
Created attachment 338714 [details] Part 2
Created attachment 338763 [details] Part 2
Created attachment 338775 [details] Part 3 WIP
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.
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
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.
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.
Committed r231024: <https://trac.webkit.org/changeset/231024>
Reopening to attach new patch.
Created attachment 338807 [details] Part 3
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
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
Filed Bug 185018 to keep track of the Win implementations. Test expectations are updated in: Committed r231036: <https://trac.webkit.org/changeset/231036>
Comment on attachment 338763 [details] Part 2 Removing flags now that this part has landed.
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!
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.
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?
Comment on attachment 338807 [details] Part 3 Clearing flags on attachment: 338807 Committed r231057: <https://trac.webkit.org/changeset/231057>
All reviewed patches have been landed. Closing bug.