Bug 160234 – Check if NSURLAuthenticationChallengeSender has implemented optional methods before calling them

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Check if NSURLAuthenticationChallengeSender has implemented optional methods before calling them

[Alex Christensen]

Created attachment 284676 [details]
Patch

[Alex Christensen]

Created attachment 284677 [details]
Patch

[Alex Christensen]

Created attachment 284678 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 284678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284678&action=review

> Source/WebCore/ChangeLog:12
> +        This is clear from the stack traces in the radars and the fact that only the two optional methods are causing crashes.

Wasn't that WebCore challenge sender, which you already fixed?

[Alexey Proskuryakov]

Comment on attachment 284678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284678&action=review

> Source/WebCore/ChangeLog:16
> +        If something is exploitable, then it does not become more exploitable with this check.

This is a strong statement when dealing with an impossible situation.

[Alexey Proskuryakov]

Comment on attachment 284678 [details]
Patch

Clearing review flag, as this doesn't seem needed at this time.

[David Kilzer (:ddkilzer)]

Comment on attachment 284678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284678&action=review

>> Source/WebCore/ChangeLog:12
>> +        This is clear from the stack traces in the radars and the fact that only the two optional methods are causing crashes.
> 
> Wasn't that WebCore challenge sender, which you already fixed?

How do you know that WebCoreAuthenticationClientAsChallengeSender is the only type of object used in this code path?

The NSProtocol definition of NSURLAuthenticationChallengeSender states that the following methods are optional:

- performDefaultHandlingForAuthenticationChallenge:
- rejectProtectionSpaceAndContinueWithChallenge:

<https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Protocols/NSURLAuthenticationChallengeSender_Protocol/>

I don't understand the logic in assuming these optional methods always exist on whatever object is passed into this method unless it's provable that it's always a WebCoreAuthenticationClientAsChallengeSender object.

>> Source/WebCore/ChangeLog:16
>> +        If something is exploitable, then it does not become more exploitable with this check.
> 
> This is a strong statement when dealing with an impossible situation.

NOTE: This statement assumes that the only type of object ever passed into this method is a WebCoreAuthenticationClientAsChallengeSender created by WebCore.

[Alexey Proskuryakov]

> The NSProtocol definition of NSURLAuthenticationChallengeSender states that the following methods are optional:

We are not allowing arbitrary NSURLAuthenticationChallengeSenders here, so it is not relevant whether these methods are marked as optional in the protocol.

[David Kilzer (:ddkilzer)]

See also <rdar://problem/27814853>.

<rdar://problem/27439617>

[David Kilzer (:ddkilzer)]

Comment on attachment 284678 [details]
Patch

r-, but only because I think we need to add logging for the type of object being used that doesn't respond to each of the selectors.

Then this patch will prevent crashes due to unimplemented selectors, and when unexpected authentication failures occur (as Alex notes in the ChangeLog), we'll capture what type of object is being used with a sysdiagnose.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> > The NSProtocol definition of NSURLAuthenticationChallengeSender states that the following methods are optional:
> 
> We are not allowing arbitrary NSURLAuthenticationChallengeSenders here, so
> it is not relevant whether these methods are marked as optional in the
> protocol.

The crashes are still occurring in iOS 10.x, so it seems like being defensive (plus logging to determine what the type of the failing objects is) would be a good path forward here.

[Alexey Proskuryakov]

Is there some other defensive fix? I think that discussing the latest attached patch takes us further away from addressing the problem.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> Is there some other defensive fix?

I don't know of one.  Do you have any?  I feel you're blocking 

> I think that discussing the latest
> attached patch takes us further away from addressing the problem.

I think the latest patch would fix the crash, and with the additional logging then any user-visible side-effect would be obvious and more easily fixed.  As it stands, we have no known individual bug reports about this crash.

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> (In reply to comment #12)
> > Is there some other defensive fix?
> 
> I don't know of one.  Do you have any?  I feel you're blocking 

...progress on fixing this crash without having a specific direction in mind.

I agree that this patch avoids the crash and doesn't address the root cause, but I also feel that not crashing is better than crashing in this case.

Do you feel that it's still better to crash in this case?

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Is there some other defensive fix?
> > 
> > I don't know of one.  Do you have any?  I feel you're blocking 
> 
> ...progress on fixing this crash without having a specific direction in mind.
> 
> I agree that this patch avoids the crash and doesn't address the root cause,
> but I also feel that not crashing is better than crashing in this case.
> 
> Do you feel that it's still better to crash in this case?

Just found some internal crashes that include the class name!

Taking discussion to <rdar://problem/27439617>.

[Alexey Proskuryakov]

I'm glad that I was blocking the patch :)

[Brent Fulgham]

Comment on attachment 284678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284678&action=review

>>> Source/WebCore/ChangeLog:12
>>> +        This is clear from the stack traces in the radars and the fact that only the two optional methods are causing crashes.
>> 
>> Wasn't that WebCore challenge sender, which you already fixed?
> 
> How do you know that WebCoreAuthenticationClientAsChallengeSender is the only type of object used in this code path?
> 
> The NSProtocol definition of NSURLAuthenticationChallengeSender states that the following methods are optional:
> 
> - performDefaultHandlingForAuthenticationChallenge:
> - rejectProtectionSpaceAndContinueWithChallenge:
> 
> <https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Protocols/NSURLAuthenticationChallengeSender_Protocol/>
> 
> I don't understand the logic in assuming these optional methods always exist on whatever object is passed into this method unless it's provable that it's always a WebCoreAuthenticationClientAsChallengeSender object.

Since the two methods are not required, there is probably nothing in Xcode that notifies developers that their sender object is missing these delegate methods.

Given that these are documented to be optional, it really seems correct that the changes in ResourceHandleMac.mm and AuthenticationManagerCocoa.mm are NEEDED.

[Alexey Proskuryakov]

Brent, I don't think that this is the case. 3rd party clients don't have a way to run custom code in the Networking process, so they can't introduce delegates that don't implement these methods.

[Alexey Proskuryakov]

Hopefully obsoleted by the fix in bug 167951.

[Alex Christensen]

*** Bug 173372 has been marked as a duplicate of this bug. ***

[Jon Lee]

rdar://problem/30675510

[Chris Dumez]

Comment on attachment 284678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284678&action=review

>>>> Source/WebCore/ChangeLog:12
>>>> +        This is clear from the stack traces in the radars and the fact that only the two optional methods are causing crashes.
>>> 
>>> Wasn't that WebCore challenge sender, which you already fixed?
>> 
>> How do you know that WebCoreAuthenticationClientAsChallengeSender is the only type of object used in this code path?
>> 
>> The NSProtocol definition of NSURLAuthenticationChallengeSender states that the following methods are optional:
>> 
>> - performDefaultHandlingForAuthenticationChallenge:
>> - rejectProtectionSpaceAndContinueWithChallenge:
>> 
>> <https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Protocols/NSURLAuthenticationChallengeSender_Protocol/>
>> 
>> I don't understand the logic in assuming these optional methods always exist on whatever object is passed into this method unless it's provable that it's always a WebCoreAuthenticationClientAsChallengeSender object.
> 
> Since the two methods are not required, there is probably nothing in Xcode that notifies developers that their sender object is missing these delegate methods.
> 
> Given that these are documented to be optional, it really seems correct that the changes in ResourceHandleMac.mm and AuthenticationManagerCocoa.mm are NEEDED.

Even if the ChallengeSender was WebCoreAuthenticationClientAsChallengeSender, as far as I can tell, the implementation of [WebCoreAuthenticationClientAsChallengeSender rejectProtectionSpaceAndContinueWithChallenge] ends up calling AuthenticationClient::receivedChallengeRejection(). The AuthenticationClient I believe is the ResourceHandle. On Cocoa, ResourceHandle::receivedChallengeRejection() ends up calling:
[[d->m_currentMacChallenge sender] rejectProtectionSpaceAndContinueWithChallenge:d->m_currentMacChallenge];

It looks to me we are merely proxying to CFNetwork, no? And rejectProtectionSpaceAndContinueWithChallenge is unimplemented in CFNetwork.

[Chris Dumez]

Comment on attachment 284678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284678&action=review

>>>>> Source/WebCore/ChangeLog:12
>>>>> +        This is clear from the stack traces in the radars and the fact that only the two optional methods are causing crashes.
>>>> 
>>>> Wasn't that WebCore challenge sender, which you already fixed?
>>> 
>>> How do you know that WebCoreAuthenticationClientAsChallengeSender is the only type of object used in this code path?
>>> 
>>> The NSProtocol definition of NSURLAuthenticationChallengeSender states that the following methods are optional:
>>> 
>>> - performDefaultHandlingForAuthenticationChallenge:
>>> - rejectProtectionSpaceAndContinueWithChallenge:
>>> 
>>> <https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Protocols/NSURLAuthenticationChallengeSender_Protocol/>
>>> 
>>> I don't understand the logic in assuming these optional methods always exist on whatever object is passed into this method unless it's provable that it's always a WebCoreAuthenticationClientAsChallengeSender object.
>> 
>> Since the two methods are not required, there is probably nothing in Xcode that notifies developers that their sender object is missing these delegate methods.
>> 
>> Given that these are documented to be optional, it really seems correct that the changes in ResourceHandleMac.mm and AuthenticationManagerCocoa.mm are NEEDED.
> 
> Even if the ChallengeSender was WebCoreAuthenticationClientAsChallengeSender, as far as I can tell, the implementation of [WebCoreAuthenticationClientAsChallengeSender rejectProtectionSpaceAndContinueWithChallenge] ends up calling AuthenticationClient::receivedChallengeRejection(). The AuthenticationClient I believe is the ResourceHandle. On Cocoa, ResourceHandle::receivedChallengeRejection() ends up calling:
> [[d->m_currentMacChallenge sender] rejectProtectionSpaceAndContinueWithChallenge:d->m_currentMacChallenge];
> 
> It looks to me we are merely proxying to CFNetwork, no? And rejectProtectionSpaceAndContinueWithChallenge is unimplemented in CFNetwork.

Actually, WebCoreAuthenticationClientAsChallengeSender is not used at all in the WebKit2 with NETWORK_SESSION enabled case.

[Geoffrey Garen]

We have an explanation for how we can end up with an NSURLAuthenticationChallengeSender that is not WebCoreAuthenticationClientAsChallengeSender: AVFoundation can supply its own challenge and sender. For example, we see in MediaPlayerPrivateAVFoundationObjC::shouldWaitForResponseToAuthenticationChallenge that AVFoundation provides its own NSURLAuthenticationChallenge.

[Per Arne Vollan]

Created attachment 324749 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #25)
> Created attachment 324749 [details]
> Patch

This is basically just a rebase of Alex's original patch, without the parts of it that are not strictly needed.

[Alexey Proskuryakov]

> For example, we see in MediaPlayerPrivateAVFoundationObjC::shouldWaitForResponseToAuthenticationChallenge that AVFoundation provides its own NSURLAuthenticationChallenge.

Isn't that in the WebContent process? How does that explain Networking process crashes?

[Alexey Proskuryakov]

Comment on attachment 324749 [details]
Patch

From previous discussions with Alex, I thought that we had a different plan of attack, what changed?

In particular, it's not only wrong to call these methods, it's wrong to call any methods on the sender object.

[Per Arne Vollan]

Created attachment 324826 [details]
Patch

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #28)
> Comment on attachment 324749 [details]
> Patch
> 
> From previous discussions with Alex, I thought that we had a different plan
> of attack, what changed?
> 
> In particular, it's not only wrong to call these methods, it's wrong to call
> any methods on the sender object.

I have uploaded a new patch, where no methods are called on the authentication challenge sender when network session is used.

In the USE(NETWORK_SESSION) case, I believe we never need to call methods on the challenge sender, since the challenge sender in the network process will always be the internal CFNetwork challenge sender. The challenge sender is deprecated when network session is used, and only contains empty implementations, and also does not implement all methods. Is this a correct assumption?

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 324826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324826&action=review

> Source/WebKit/ChangeLog:3
> +        Check if NSURLAuthenticationChallengeSender has implemented optional methods before calling them

We should probably change this title, as the focus of the change has shifted.

[Per Arne Vollan]

Created attachment 324836 [details]
Patch

[Brent Fulgham]

Comment on attachment 324836 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324836&action=review

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=160234

Please include the relevant <rdar> here as well.

> Source/WebKit/ChangeLog:9
> +        challenge sender, which has not implemented the optional method. The methods on the authentication challenge

which DOES NOT implement THIS optional method.

> Source/WebKit/ChangeLog:10
> +        sender has been deprecated when network session is used, so we should not call them in that case.

sender ARE deprecated when ....

[Geoffrey Garen]

Are we still surprised to receive a CFNetwork challenge sender, and do we still believe that might indicate a bug?

If so, we can also add an internal-build-only RELEASE_ASSERT at the point we first encounter a challenge to assert that it has the type we expect. That will give us backtraces to explain where these surprising challenges come from.

[Per Arne Vollan]

Created attachment 324849 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #33)
> Comment on attachment 324836 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324836&action=review
> 
> > Source/WebKit/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=160234
> 
> Please include the relevant <rdar> here as well.
> 
> > Source/WebKit/ChangeLog:9
> > +        challenge sender, which has not implemented the optional method. The methods on the authentication challenge
> 
> which DOES NOT implement THIS optional method.
> 
> > Source/WebKit/ChangeLog:10
> > +        sender has been deprecated when network session is used, so we should not call them in that case.
> 
> sender ARE deprecated when ....

Updated. Thanks for reviewing!

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #34)
> Are we still surprised to receive a CFNetwork challenge sender, and do we
> still believe that might indicate a bug?
> 
> If so, we can also add an internal-build-only RELEASE_ASSERT at the point we
> first encounter a challenge to assert that it has the type we expect. That
> will give us backtraces to explain where these surprising challenges come
> from.

No, I believe we always should receive a CFNetwork challenge sender in the network process in the USE(NETWORK_SESSION) case. This patch is based on that assumption. If that assumption is incorrect, then this patch is incorrect. What is surprising is that both the completion handler and the authentication client is null, which makes us fall through to the place where we call the unimplemented method on the CFNetwork challenge sender. I can replace the ASSERTS in the current patch with RELEASE_ASSERTs for internal builds. Thanks for reviewing!

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #34)
> Are we still surprised to receive a CFNetwork challenge sender, and do we
> still believe that might indicate a bug?
> 
> If so, we can also add an internal-build-only RELEASE_ASSERT at the point we
> first encounter a challenge to assert that it has the type we expect. That
> will give us backtraces to explain where these surprising challenges come
> from.

I can't seem to find any examples of RELEASE_ASSERTs for internal builds only, how would you do this?

[Geoffrey Garen]

> I can't seem to find any examples of RELEASE_ASSERTs for internal builds
> only, how would you do this?

The best way to write internal-only behavior is to use <os/variant_private.h>. I believe the right check in this case would be os_variant_has_internal_diagnostics.

That said, it sounds like your analysis is that a CFNetwork challenge sender is expected, and we were just handling it incorrectly before. So, an internal-only assert for the sake of investigation may not be necessary.

[Alexey Proskuryakov]

Here is my current understanding after talking to Alex.

This code should not be reachable at all on modern OS versions. As Per Arne said above, we get here because both completion handler and authentication handler are null. A couple ways that could happen are:

- bookkeeping problems in our networking code, so we lose track of the completion handler too early;
- incomplete/unexpected messages coming from media code in WebContent (but again, that shouldn't be happening in modern OS versions).

[Geoffrey Garen]

It looks like we encountered a very similar problem before, and tried to work around its low-level consequences. See <https://trac.webkit.org/changeset/197865/webkit>.

[Geoffrey Garen]

> It looks like we encountered a very similar problem before, and tried to
> work around its low-level consequences. See
> <https://trac.webkit.org/changeset/197865/webkit>.

The central trigger point for this kind of bug might be NetworkDataTaskCocoa::didReceiveChallenge.

[Geoffrey Garen]

Comment on attachment 324849 [details]
Patch

r=me

I think this is an improvement because it clarifies that NETWORK_SESSION code is never supposed to invoke the deprecated default... selector, and it removes the crash from customer builds.

To continue investigating the mystery of why we end up with an authentication challenge that has no client or completion handler, I'd suggest adding code like this to NetworkDataTaskCocoa::didReceiveChallenge:

        if (internal_build())
            RELEASE_ASSERT_NOT_REACHED();
        else
            ASSERT_NOT_REACHED();

If we discover that we use this kind of technique more often, consider making a shared macro in Assertions.h that encapsulates this behavior.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #43)
> Comment on attachment 324849 [details]
> Patch
> 
> r=me
> 
> I think this is an improvement because it clarifies that NETWORK_SESSION
> code is never supposed to invoke the deprecated default... selector, and it
> removes the crash from customer builds.
> 
> To continue investigating the mystery of why we end up with an
> authentication challenge that has no client or completion handler, I'd
> suggest adding code like this to NetworkDataTaskCocoa::didReceiveChallenge:
> 
>         if (internal_build())
>             RELEASE_ASSERT_NOT_REACHED();
>         else
>             ASSERT_NOT_REACHED();
> 
> If we discover that we use this kind of technique more often, consider
> making a shared macro in Assertions.h that encapsulates this behavior.

I'll create a separate patch for this. Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 324849 [details]
Patch

Clearing flags on attachment: 324849

Committed r223993: <https://trac.webkit.org/changeset/223993>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.