Bug 196213 – Do not terminate the NetworkProcess if a third party application sends a NSCredential with a SecIdentityRef

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Do not terminate the NetworkProcess if a third party application sends a NSCredential with a SecIdentityRef

[Alex Christensen]

Created attachment 365896 [details]
Patch

[Alex Christensen]

Created attachment 365898 [details]
Patch

[Alex Christensen]

rdar://problem/49156511

[Geoffrey Garen]

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?

[Alex Christensen]

(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.

[Alex Christensen]

Created attachment 365903 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365903 [details]
Patch

Clearing flags on attachment: 365903

Committed r243465: <https://trac.webkit.org/changeset/243465>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49231641>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 196226

[Alex Christensen]

Maybe the commit bot shouldn't commit a patch that doesn't build on open source iOS bots...

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 365946 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365946 [details]
Patch

Clearing flags on attachment: 365946

Committed r243487: <https://trac.webkit.org/changeset/243487>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

It would also be nice if the commit queue took less than 1.5 days to commit.

[Alex Christensen]

Comment on attachment 365946 [details]
Patch

Oh, it landed, it just didn't remove the cq+