| Summary: | Do not terminate the NetworkProcess if a third party application sends a NSCredential with a SecIdentityRef | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | ap, commit-queue, ggaren, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 196226 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Alex Christensen
2019-03-25 13:58:19 PDT
Created attachment 365896 [details]
Patch
Created attachment 365898 [details]
Patch
Comment on attachment 365898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365898&action=review r=me > Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:719 > + if (!hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials)) { > + result = nullptr; > + return true; > + } It seems a little sloppy that the !hasKey case above and the false if (key) case below return true without assigning nullptr to result, while here we do assign nullptr to result. I'd like us to be consistent. Looking at our caller, nullptr is the default value. So, both choices can work. Can you pick one and apply it consistently in this function? (In reply to Geoffrey Garen from comment #4) > Comment on attachment 365898 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=365898&action=review > > r=me > > > Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:719 > > + if (!hasProcessPrivilege(ProcessPrivilege::CanAccessCredentials)) { > > + result = nullptr; > > + return true; > > + } > > It seems a little sloppy that the !hasKey case above and the false if (key) > case below return true without assigning nullptr to result, while here we do > assign nullptr to result. I'd like us to be consistent. > > Looking at our caller, nullptr is the default value. So, both choices can > work. Can you pick one and apply it consistently in this function? The nullptr assignment is redundant since the default constructor of RetainPtr<SecIdentityRef> has already been called. I'll remove it. Created attachment 365903 [details]
Patch
Comment on attachment 365903 [details] Patch Clearing flags on attachment: 365903 Committed r243465: <https://trac.webkit.org/changeset/243465> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 196226 Maybe the commit bot shouldn't commit a patch that doesn't build on open source iOS bots... Hmmm. SecKeychainRef is unavailable on open source iOS builds, and iOS doesn't really seem to have the ability to handle keychains like I do in this test (SecIdentityCreateWithCertificate, SecKeychainCreate unavailable). I'll just make the test Mac only and figure out how to write client certificate tests for iOS once I start writing more in-depth tests. This test as it is now already shows the assertion can be reached, so it would've prevented the original regression. Created attachment 365946 [details]
Patch
Comment on attachment 365946 [details] Patch Clearing flags on attachment: 365946 Committed r243487: <https://trac.webkit.org/changeset/243487> All reviewed patches have been landed. Closing bug. It would also be nice if the commit queue took less than 1.5 days to commit. Comment on attachment 365946 [details]
Patch
Oh, it landed, it just didn't remove the cq+
|