Closed Bug 1293980 Opened 8 years ago Closed 8 years ago

NetMonitor fails to get security info of a request intercepted by service worker

Categories

(DevTools :: Netmonitor, defect)

defect
Not set
normal

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: jsnajdr, Assigned: jsnajdr)

References

Details

Attachments

(1 file)

Steps to reproduce:
1. Run the test devtools/client/netmonitor/test/browser_net_service-worker-status.js in debug/e10s mode
2. Watch the output

It reports an assertion in the content process:

INFO -  [Child 2154] ###!!! ASSERTION: Unimplemented on content process: 'Not Reached', file /builds/slave/try-m64-d-00000000000000000000/build/src/security/manager/ssl/nsNSSCertificateFakeTransport.cpp, line 78
INFO -  #01: NS_InvokeByIndex [xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:180]
INFO -  #02: CallMethodHelper::Call() [js/xpconnect/src/XPCWrappedNative.cpp:1393]
INFO -  #03: XPC_WN_GetterSetter(JSContext*, unsigned int, JS::Value*) [js/xpconnect/src/XPCWrappedNative.cpp:1360]

And a JS exception:

INFO -    Handler function threw an exception: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIX509Cert.commonName]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-helper.js :: NetworkHelper.parseCertificateInfo :: line 678"  data: no]
INFO -  Stack: NetworkHelper.parseCertificateInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-helper.js:678:9
INFO -  NetworkHelper.parseSecurityInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-helper.js:628:19
INFO -  NetworkResponseListener.prototype._getSecurityInfo<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:508:16
INFO -  exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
INFO -  NetworkResponseListener.prototype.onStartRequest@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:459:5

Why?

The test has a service worker that is making a network request, and it's inspecting this request with NetworkMonitor. This involves getting its security info, i.e., accessing the channel.securityInfo.SSLStatus.serverCert.commonName property (follow the call sequence from [1] to [2]).

But service worker requests happen in the content process, where the nsIX509Cert interface is defined as nsNSSCertificateFakeTransport (see [3]), which just asserts and throws NS_ERROR_NOT_IMPLEMENTED on all methods.

This means that it's impossible to get security info out of a request happening in the content process, i.e., in a service worker.

Ben, how do you think this could be fixed? Seems like NSS prohibits working with security-sensitive data in the content process, which is at odds with the service worker doing its work there.

Ryan, is it OK that a test that hits an assert can actually succeed? The behavior on asserts is governed by the XPCOM_DEBUG_BREAK environment variable, which is set to "warn" in this test suite. If it was "stack-and-abort", the test would fail on this assertion (and on many others). The log at [4] is a live example how this assertion happens in a 100% green try run.

[1] http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#459
[2] http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-helper.js#678
[3] http://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSModule.cpp#193-195
[4] https://treeherder.mozilla.org/logviewer.html#?job_id=25288635&repo=try#L13724
Flags: needinfo?(jryans)
Flags: needinfo?(bkelly)
Does it show the security info of the request that the service worker makes?  (I like to call that the inner request.)  That should follow normal network paths.

Its unclear to me that we should really expect the outer service worker intercepted request to have security info at all.  The service worker can manufacture results, provide old responses using an expired cert, etc.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #1)
> Does it show the security info of the request that the service worker makes?
> (I like to call that the inner request.)  That should follow normal network
> paths.
> 
> Its unclear to me that we should really expect the outer service worker
> intercepted request to have security info at all.  The service worker can
> manufacture results, provide old responses using an expired cert, etc.

I looked at it more closely and:
- the inner request's channel is inspected in the parent process, and its security info is OK
- it's the outer request that causes the exception as its channel is inspected in the child process (when the synthesized response arrives)

So, we should fix the NetMonitor to not ask for securityInfo in child process, as it doesn't have any meaning for the synthesized responses.

Originally, I thought it's the inner request that's causing the exception. Was there any recent change regarding in which process is the inner request executed? Two months ago, it was in the child (IIRC) and now it's in the parent...
Flags: needinfo?(bkelly)
(In reply to Jarda Snajdr [:jsnajdr] from comment #2)
> Originally, I thought it's the inner request that's causing the exception.
> Was there any recent change regarding in which process is the inner request
> executed? Two months ago, it was in the child (IIRC) and now it's in the
> parent...

The fetch() call still happens in the child process (just like a fetch from a content script), but necko performs the network operations in the parent.  AFAIK there has been no change in this setup.  Its always been like this.

Maybe we are firing observer events in different places now?
Flags: needinfo?(bkelly)
Blocks: 1273795
(In reply to Ben Kelly [:bkelly] from comment #3)
> The fetch() call still happens in the child process (just like a fetch from
> a content script), but necko performs the network operations in the parent. 
> AFAIK there has been no change in this setup.  Its always been like this.
> 
> Maybe we are firing observer events in different places now?

OK, I'll checkout the old sources and will refresh my memory.

One question: when a request is served from cache and has never hit the network, netmonitor still displays its security info - the TLS ciphersuite used, certificates... How did it get there? Is it a part of the data stored in cache? Or does it mean the request did hit the network and I'm wrong in assuming it didn't?
Flags: needinfo?(bkelly)
(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> One question: when a request is served from cache and has never hit the
> network, netmonitor still displays its security info - the TLS ciphersuite
> used, certificates... How did it get there? Is it a part of the data stored
> in cache? Or does it mean the request did hit the network and I'm wrong in
> assuming it didn't?

Its stored on disk just like we do for service worker Cache API.  The difference is http cache is performed in the parent process.
Flags: needinfo?(bkelly)
(In reply to Jarda Snajdr [:jsnajdr] from comment #0)
> Ryan, is it OK that a test that hits an assert can actually succeed? The
> behavior on asserts is governed by the XPCOM_DEBUG_BREAK environment
> variable, which is set to "warn" in this test suite. If it was
> "stack-and-abort", the test would fail on this assertion (and on many
> others). The log at [4] is a live example how this assertion happens in a
> 100% green try run.

I agree it's unfortunate.  My guess is it's been set that way for quite a long time.  It might be good to open a separate bug to explore tightening this down, at least for the DevTools subsuites of mochitest browser.  It's possible though that there will be many assertions to fight through since we've been running in warn only mode for so long.
Flags: needinfo?(jryans)
Assignee: nobody → jsnajdr
Summary: NetMonitor cannot show security info of HTTP request issued by service worker → NetMonitor fails to get security info of a request intercepted by service worker
Comment on attachment 8780001 [details]
Bug 1293980 - NetMonitor fails to get security info of a request intercepted by service worker

Don't retrieve the request's security info if in content process, and leave it null. This means that requests intercepted by a service worker will never have the "Security" tab displayed.

Should fix the intermittent in bug 1273795. I submitted a try run together with patches from bug 1269102 (copy-as-curl header ordering), because the changes introduced there turn that intermittent into a permafail.
Comment on attachment 8780001 [details]
Bug 1293980 - NetMonitor fails to get security info of a request intercepted by service worker

https://reviewboard.mozilla.org/r/70856/#review68258

Thanks again Jarda.

::: devtools/shared/webconsole/network-monitor.js:505
(Diff revision 1)
>     * Parse security state of this request and report it to the client.
>     */
>    _getSecurityInfo: DevToolsUtils.makeInfallible(function () {
> +    // Many properties of the securityInfo (e.g., the server certificate or HPKP
> +    // status) are not available in the content process and are dangerous to even touch,
> +    // because their C++ getters trigger assertions. This function is called in content

I wouldn't use the word "dangerous", but only mention it triggers assertions.
Attachment #8780001 - Flags: review?(poirot.alex) → review+
Comment on attachment 8780001 [details]
Bug 1293980 - NetMonitor fails to get security info of a request intercepted by service worker

https://reviewboard.mozilla.org/r/70856/#review68298

::: devtools/shared/webconsole/network-monitor.js:505
(Diff revision 1)
>     * Parse security state of this request and report it to the client.
>     */
>    _getSecurityInfo: DevToolsUtils.makeInfallible(function () {
> +    // Many properties of the securityInfo (e.g., the server certificate or HPKP
> +    // status) are not available in the content process and are dangerous to even touch,
> +    // because their C++ getters trigger assertions. This function is called in content

Changed to "can't be touched safely"
Keywords: checkin-needed
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3b1125efa8bd
NetMonitor fails to get security info of a request intercepted by service worker r=ochameau
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b1125efa8bd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: