Bug 184428 - Remove access to keychain from the WebContent process
Summary: Remove access to keychain from the WebContent process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 185537
  Show dependency treegraph
 
Reported: 2018-04-09 14:40 PDT by Jiewen Tan
Modified: 2018-05-10 17:28 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-04-09 14:40:46 PDT
Remove access to keychain from the WebContent process.
Comment 1 Jiewen Tan 2018-04-09 14:48:12 PDT
Created attachment 337544 [details]
Patch
Comment 2 Jiewen Tan 2018-04-09 14:50:15 PDT
<rdar://problem/13150903>
Comment 3 Jiewen Tan 2018-04-17 21:17:58 PDT
Created attachment 338193 [details]
Patch
Comment 4 Conrad Shultz 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.
Comment 5 Jiewen Tan 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.
Comment 6 Jiewen Tan 2018-04-18 13:55:07 PDT
Created attachment 338258 [details]
Patch
Comment 7 Conrad Shultz 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)`
Comment 8 Jiewen Tan 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.
Comment 9 Jiewen Tan 2018-04-18 14:02:17 PDT
Created attachment 338261 [details]
Patch
Comment 10 Conrad Shultz 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?
Comment 11 Jiewen Tan 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..
Comment 12 Jiewen Tan 2018-04-18 14:48:33 PDT
Created attachment 338265 [details]
Part 1
Comment 13 Conrad Shultz 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!
Comment 14 Brent Fulgham 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).
Comment 15 Jiewen Tan 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.
Comment 16 Jiewen Tan 2018-04-19 15:08:15 PDT
Created attachment 338364 [details]
Part 1 for landing
Comment 17 WebKit Commit Bot 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>
Comment 18 Jiewen Tan 2018-04-25 01:40:06 PDT
Created attachment 338714 [details]
Part 2
Comment 19 Jiewen Tan 2018-04-25 11:44:25 PDT
Created attachment 338763 [details]
Part 2
Comment 20 Jiewen Tan 2018-04-25 13:09:43 PDT
Created attachment 338775 [details]
Part 3 WIP
Comment 21 EWS Watchlist 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.
Comment 22 EWS Watchlist 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
Comment 23 Brent Fulgham 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.
Comment 24 Jiewen Tan 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.
Comment 25 Jiewen Tan 2018-04-25 15:10:10 PDT
Committed r231024: <https://trac.webkit.org/changeset/231024>
Comment 26 Jiewen Tan 2018-04-25 15:11:31 PDT
Reopening to attach new patch.
Comment 27 Jiewen Tan 2018-04-25 15:11:33 PDT
Created attachment 338807 [details]
Part 3
Comment 28 EWS Watchlist 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
Comment 29 EWS Watchlist 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
Comment 30 Jiewen Tan 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>
Comment 31 Brent Fulgham 2018-04-26 09:41:31 PDT
Comment on attachment 338763 [details]
Part 2

Removing flags now that this part has landed.
Comment 32 Brent Fulgham 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!
Comment 33 Jiewen Tan 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.
Comment 34 Jiewen Tan 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?
Comment 35 WebKit Commit Bot 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>
Comment 36 WebKit Commit Bot 2018-04-26 11:20:58 PDT
All reviewed patches have been landed.  Closing bug.