Bug 134666 – NetworkProcess sometimes hangs under copyDefaultCredentialForProtectionSpace

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

NetworkProcess sometimes hangs under copyDefaultCredentialForProtectionSpace

Looking at the backtrace, it's hanging forever inside the SecItem shim, waiting for a response from secItemRequest.

Looking at the SecItemShimProxy in the secItemRequest handler, it seems the only possible way a response would not be sent is if the request type was SecItemRequestData::Invalid.  That condition does an early return instead of sending back a response.

At the very least, the UIProcess should send back a response indicating "invalid request" so the NetworkProcess doesn't hang forever.

<rdar://problem/17398060>

[Brady Eidson]

Created attachment 234467 [details]
Patch v1 - Speculative fix

[Tim Horton]

(In reply to comment #0)
> At the very least, the UIProcess should send back a response indicating "invalid request" so the NetworkProcess doesn't hang forever.

Our approach when the WebProcess sends us a message we really don't expect to ever happen is that we kill it, assuming compromise. Should we do the same to the NetworkProcess in this situation? IIRC there is literally no situation in which we ever send an "Invalid".

[Brady Eidson]

(In reply to comment #2)
> (In reply to comment #0)
> > At the very least, the UIProcess should send back a response indicating "invalid request" so the NetworkProcess doesn't hang forever.
> 
> Our approach when the WebProcess sends us a message we really don't expect to ever happen is that we kill it, assuming compromise. Should we do the same to the NetworkProcess in this situation? IIRC there is literally no situation in which we ever send an "Invalid".

Killing the NetworkProcess isn't quite a recoverable scenario the same way killing a WebProcess is.

That said:  Your comment encouraged me to look at all the possible scenarios in a little more detail.

The current "invalid" handler is definitely one way the UIProcess would never respond, if a request of type "invalid" was received.

But, as you said, we never explicitly send an invalid request.

Another way the UIProcess might not ever respond is if the message failed to decode.

Looking at SecItemRequestData::decode and comparing it to SecItemRequestData::encode there's an obvious way that the decode could fail - if the CFDictionaryRef for the original query was null.

We'd happily encode such a case, but fail to decode it.

[Tim Horton]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > At the very least, the UIProcess should send back a response indicating "invalid request" so the NetworkProcess doesn't hang forever.
> > 
> > Our approach when the WebProcess sends us a message we really don't expect to ever happen is that we kill it, assuming compromise. Should we do the same to the NetworkProcess in this situation? IIRC there is literally no situation in which we ever send an "Invalid".
> 
> Killing the NetworkProcess isn't quite a recoverable scenario the same way killing a WebProcess is.

Ah, I suppose! Losing all your downloads and whatnot.

> Looking at SecItemRequestData::decode and comparing it to SecItemRequestData::encode there's an obvious way that the decode could fail - if the CFDictionaryRef for the original query was null.
> 
> We'd happily encode such a case, but fail to decode it.

Aha! Sounds somewhat more promising than the Invalid thing.

[Brady Eidson]

We might've assumed that passing in a null query dictionary is invalid, and therefore a case we didn't need to support...  but maybe we need to support it to catch these invalid uses.

[Brady Eidson]

Created attachment 234468 [details]
Patch v2 - A way better speculative fix

[Tim Horton]

Comment on attachment 234468 [details]
Patch v2 - A way better speculative fix

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

> Source/WebKit2/Shared/mac/SecItemRequestData.cpp:70
> +    bool expectQuery;

maybe "hasQuery"? either way is fine.

[WebKit Commit Bot]

Comment on attachment 234468 [details]
Patch v2 - A way better speculative fix

Rejecting attachment 234468 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
cess/mac/SecItemShimProxy.cpp:66:40: error: use of undeclared identifier 'errSecParam'
        response = SecItemResponseData(errSecParam, nullptr);
                                       ^
1 error generated.

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit2.build/Release/WebKit.build/Objects-normal/x86_64/SecItemShimProxy.o UIProcess/mac/SecItemShimProxy.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.appspot.com/results/5929684067942400

[Brady Eidson]

Fixed that locally, landed in http://trac.webkit.org/changeset/170831

The problem isn’t fixed, and I have a much-less-speculative fix. Patch forthcoming.

Created attachment 234841 [details]
Add SecAccessControlRef support to ArgumentCodersCF

[Tim Horton]

Comment on attachment 234841 [details]
Add SecAccessControlRef support to ArgumentCodersCF

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

> Source/WebKit2/ChangeLog:11
> +        by ArgumentCodersCF. In debug builds, trying to encode a CFDictionary containing a value of
> +        unsupprted type causes an assertion to fail, but in release builds encoding succeeds, and
> +        only decoding fails, in this case silently, simply not delivering the

This seems like a pretty unfortunate failure mode...

> Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:731
> +    result = adoptCF(SecAccessControlCreateFromData(kCFAllocatorDefault, data.get(), nullptr));

Do we know that these things are sufficiently safe to IPC as raw data?

(In reply to comment #12)
> (From update of attachment 234841 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234841&action=review
> 
> > Source/WebKit2/ChangeLog:11
> > +        by ArgumentCodersCF. In debug builds, trying to encode a CFDictionary containing a value of
> > +        unsupprted type causes an assertion to fail, but in release builds encoding succeeds, and
> > +        only decoding fails, in this case silently, simply not delivering the
> 
> This seems like a pretty unfortunate failure mode...

Agreed.

> 
> > Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:731
> > +    result = adoptCF(SecAccessControlCreateFromData(kCFAllocatorDefault, data.get(), nullptr));
> 
> Do we know that these things are sufficiently safe to IPC as raw data?

Yes.

Thanks for the review.

Fixed in <http://trac.webkit.org/r171060>.

[Brady Eidson]

Great find!  Thanks!