| Summary: | Check if NSURLAuthenticationChallengeSender has implemented optional methods before calling them | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | ap, beidson, bfulgham, cdumez, commit-queue, ddkilzer, ggaren, jer.noble, jonlee, pvollan, webkit-bug-importer | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=156947 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Alex Christensen
2016-07-26 23:20:42 PDT
Created attachment 284676 [details]
Patch
Created attachment 284677 [details]
Patch
Created attachment 284678 [details]
Patch
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? 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. Comment on attachment 284678 [details]
Patch
Clearing review flag, as this doesn't seem needed at this time.
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. > 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.
See also <rdar://problem/27814853>. <rdar://problem/27439617> 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.
(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. Is there some other defensive fix? I think that discussing the latest attached patch takes us further away from addressing the problem. (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. (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? (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>. I'm glad that I was blocking the patch :) 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. 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. Hopefully obsoleted by the fix in bug 167951. *** Bug 173372 has been marked as a duplicate of this bug. *** 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. 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. 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. Created attachment 324749 [details]
Patch
(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. > 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?
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.
Created attachment 324826 [details]
Patch
(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! 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. Created attachment 324836 [details]
Patch
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 .... 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. Created attachment 324849 [details]
Patch
(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! (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! (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? > 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.
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). 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>. > 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.
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.
(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! Comment on attachment 324849 [details] Patch Clearing flags on attachment: 324849 Committed r223993: <https://trac.webkit.org/changeset/223993> All reviewed patches have been landed. Closing bug. |