Closed Bug 1178163 Opened 5 years ago Closed 5 years ago

[Control Center] file:// URIs and some internal pages show red "Connection is not secure"

Categories

(Firefox :: General, defect, P1)

defect
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.3 - Aug 10
Tracking Status
firefox41 --- affected
firefox42 --- verified

People

(Reporter: ttaubert, Assigned: bgrins)

References

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(2 files, 1 obsolete file)

We need to figure out what we want to do with file:// URIs and internal ones such as "about:robots" or pages added by add-ons. Showing the red "Connection is not secure" label seems a little too much.

What could we say for local (file://, jar://) pages? Probably just a default-colored text that it's an internal/local page?

What's the security model for file:// pages? I probably could convince a user to open an .html file and do weird stuff there, maybe an <iframe> with Amazon in it. But I could also just try to convince the user to open an .exe file then?
Flags: qe-verify+
Flags: needinfo?(agrigas)
Flags: firefox-backlog?
(In reply to Tim Taubert [:ttaubert] from comment #0)
> What could we say for local (file://, jar://) pages? Probably just a
> default-colored text that it's an internal/local page?

Yeah, explain that it's local or say that the security status is unknown?

> What's the security model for file:// pages?

There is some info at https://developer.mozilla.org/en-US/docs/Same-origin_policy_for_file%3A_URIs. File URIs aren't same-origin with HTTP(S) ones. Whether or not the path matters when deciding whether two file URIs are same-origin varies by browser.

> I probably could convince a
> user to open an .html file and do weird stuff there, maybe an <iframe> with
> Amazon in it. But I could also just try to convince the user to open an .exe
> file then?

Yeah, probably.
Maybe it could say "This page is stored on your computer" ? (where I'd want something nicer than "page" because it could be an image/video/something-the-user-thinks-is-an-app-type-thing (about: pages / local web app) but can't think what that would be...)
Im ok saying 'This page is stored on your computer' and use the small logo (firefox logo for example where the lock would show in the drop-down) Even if its a different type of asset, a user's mental model is probably that its embedded in a page anyways.
Flags: needinfo?(agrigas)
Points: --- → 2
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P1
Rank: 1
Just leaving a note that we probably want to hide the TP section in the control center as well.
QA Contact: mwobensmith
(In reply to Tim Taubert [:ttaubert] from comment #4)
> Just leaving a note that we probably want to hide the TP section in the
> control center as well.

And another note that maybe we shouldn't do that, a local file:// URI might still contain a tracker.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 42.2 - Jul 27
Keep things like data: URIs in mind too.
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Attached patch file-uri-cc.patch (obsolete) — Splinter Review
The code to do this seems relatively straightforward, but I'm unsure how to deal with a variety of URI types:

1. file URI - showing the "This page is stored on your computer" message
2. data URI - I'm hiding all messages in this patch.  We could alternatively add a new string for this, but it seems ok to not have much explanation since there isn't really any identity information to display
3. chrome URI - Unsure. I haven't changed the behavior here yet, but we could do the same thing that we are doing with files.  Matt mentioned that there could possibly be cases where saying "This page is stored on your computer" might give a false sense of security for pages loaded by addons.  Additionally, are there any cases where chrome URIs are not in fact stored on the computed?
4. jar URI - I omitted these from the patch because FWIU there is a case where a jar can in fact load a remote resource and we would definitely *not* want to incorrectly report that it's local.
Attachment #8636956 - Flags: review?(ttaubert)
Comment on attachment 8636956 [details] [diff] [review]
file-uri-cc.patch

Review of attachment 8636956 [details] [diff] [review]:
-----------------------------------------------------------------

I think this isn't quite the right solution, I had something in mind like this:

In checkIdentity() we would do (whenever !isChromeUI):

> let chan = Services.io.newChannelFromURI(uri);

Now |chan.originalURI| is the URI that we passed in, e.g. http://www.mozilla.org/ or about:robots.

Now however we can check |chan.URI| as that points to the resource that the originalURI resolves to.

http://www.mozilla.org/ -> http://www.mozilla.org/
about:robots -> file:///Users/tim/workspace/mozilla-central/browser/base/content/aboutRobots.xhtml

So everything that resolves to file: (or data:) and where the originalURI is not in the whitelist should get the "internal" state.
Attachment #8636956 - Flags: review?(ttaubert)
It would be nicer to just do |aRequest.QueryInterface(nsIChannel)| in XULBrowserWindow.onSecurityChange() and pass on the respective URIs but unfortunately the <tabbrowser> sends fake onSecurityChange notifications that pass aRequest=null.
newChannelFromURI() is deprecated. Looks like we should be using newChannelFromURI2().

> Services.io.newChannelFromURI2(uri, null, null, Ci.nsILoadInfo.SEC_NORMAL, Ci.nsIContentPolicy.TYPE_OTHER);

I think the flags here don't really matter because we're not going to actually use the channel.
Bug 1178163 - Show connection info for file and data URIs in control center;r=ttaubert
Attachment #8637662 - Flags: review?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #8)
> Comment on attachment 8636956 [details] [diff] [review]
> file-uri-cc.patch
> 
> Review of attachment 8636956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this isn't quite the right solution, I had something in mind like
> this:
> 
> In checkIdentity() we would do (whenever !isChromeUI):
> 
> > let chan = Services.io.newChannelFromURI(uri);
> 
> Now |chan.originalURI| is the URI that we passed in, e.g.
> http://www.mozilla.org/ or about:robots.
> 
> Now however we can check |chan.URI| as that points to the resource that the
> originalURI resolves to.
> 
> http://www.mozilla.org/ -> http://www.mozilla.org/
> about:robots ->
> file:///Users/tim/workspace/mozilla-central/browser/base/content/aboutRobots.
> xhtml
> 
> So everything that resolves to file: (or data:) and where the originalURI is
> not in the whitelist should get the "internal" state.

Good idea, I've added a patch that does this.  I'm still trying to work out an issue on try but I think it'll just be a minor update to the test.
Attachment #8636956 - Attachment is obsolete: true
Attachment #8637662 - Flags: review?(ttaubert)
Comment on attachment 8637662 [details]
MozReview Request: Bug 1178163 - Show connection info for file and data URIs in control center;r=ttaubert

https://reviewboard.mozilla.org/r/13959/#review12501

::: browser/base/content/browser.js:6816
(Diff revision 1)
> +      let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> +      let chanURI = Services.io.newChannelFromURI2(uri,
> +                                                   null,
> +                                                   systemPrincipal,

Do we actually need to pass the system principal here?

::: browser/base/content/browser.js:6831
(Diff revision 1)
> +    } else if (isDataURI) {
> +      this.setMode(this.IDENTITY_MODE_DATA_URI);

Do we really need a separate state for data: URIs? I would probably just let them hit MODE_UNKNOWN. It seems like the average user will never intentionally open a data: URI in a tab, I would expect some shady player trying something weird if that happens.

::: browser/base/content/browser.js:6829
(Diff revision 1)
> +    } else if (isFileURI) {
> +      this.setMode(this.IDENTITY_MODE_FILE_URI);

I have a hunch that we should let file: URIs where also the originalURI has the file: scheme hit MODE_UNKNOWN. Only if !uri.schemeIs("file") && chanURI.schemeIs("file") we should not show a warning as that would be true for all built-in schemes including add-ons. Tricking a user per mail into unzipping a file and opening a local .html file pretending to be some page sounds too easy.

IOW: I think this bug should only be about not showing a red "connection is not secure" for internal pages stored on the user's machine.
(In reply to Tim Taubert [:ttaubert] from comment #13)
> IOW: I think this bug should only be about not showing a red "connection is
> not secure" for internal pages stored on the user's machine.

So maybe we'd only want to introduce a single new "internal" state and change the copy to say something like "This is an internal page."?
(In reply to Tim Taubert [:ttaubert] from comment #13)
> Comment on attachment 8637662 [details]
> MozReview Request: Bug 1178163 - Show connection info for file and data URIs
> in control center;r=ttaubert
> 
> https://reviewboard.mozilla.org/r/13959/#review12501
> 
> ::: browser/base/content/browser.js:6816
> (Diff revision 1)
> > +      let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> > +      let chanURI = Services.io.newChannelFromURI2(uri,
> > +                                                   null,
> > +                                                   systemPrincipal,
> 
> Do we actually need to pass the system principal here?

I wondered the same thing.  According to http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIIOService.idl#87:

91   * This defaults to the principal of aLoadingNode, so when aLoadingNode
92   * is passed this can be left as null. However for loads where
93   * aLoadingNode is null this argument must be passed.
...
97   * This principal should almost always be the system principal if
98   * aLoadingNode is null. The only exception to this is for loads
99   * from WebWorkers since they don't have any nodes to be passed as
100  * aLoadingNode.

In fact, if I pass null it seems to work on the pages that I tested it on, but I was being cautious based on the documentation (and a search of other uses in dxr) since I wondered if there was possibly some situation where it would be required.

Christoph, we are using NewChannelFromURI2 to take a URI and resolve it to the resource that's going to be requested (ie about:robots -> file:///Users/tim/workspace/mozilla-central/browser/base/content/aboutRobots.xhtml).  First, is that the best way to get that information?  And if so, do we still need to pass in the system principal as the loadingPrincipal if we are never going to use the channel for anything else except for getting the URI?
Flags: needinfo?(mozilla)
(In reply to Brian Grinstead [:bgrins] from comment #15)
> (In reply to Tim Taubert [:ttaubert] from comment #13)
> > Comment on attachment 8637662 [details]
> > MozReview Request: Bug 1178163 - Show connection info for file and data URIs
> > in control center;r=ttaubert
> > 
> > https://reviewboard.mozilla.org/r/13959/#review12501
> > 
> > ::: browser/base/content/browser.js:6816
> > (Diff revision 1)
> > > +      let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> > > +      let chanURI = Services.io.newChannelFromURI2(uri,
> > > +                                                   null,
> > > +                                                   systemPrincipal,
> > 
> > Do we actually need to pass the system principal here?
> 
> I wondered the same thing.  According to
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIIOService.
> idl#87:
> 
> 91   * This defaults to the principal of aLoadingNode, so when aLoadingNode
> 92   * is passed this can be left as null. However for loads where
> 93   * aLoadingNode is null this argument must be passed.
> ...
> 97   * This principal should almost always be the system principal if
> 98   * aLoadingNode is null. The only exception to this is for loads
> 99   * from WebWorkers since they don't have any nodes to be passed as
> 100  * aLoadingNode.
> 
> In fact, if I pass null it seems to work on the pages that I tested it on,
> but I was being cautious based on the documentation (and a search of other
> uses in dxr) since I wondered if there was possibly some situation where it
> would be required.
> 
> Christoph, we are using NewChannelFromURI2 to take a URI and resolve it to
> the resource that's going to be requested (ie about:robots ->
> file:///Users/tim/workspace/mozilla-central/browser/base/content/aboutRobots.
> xhtml).  First, is that the best way to get that information?  And if so, do
> we still need to pass in the system principal as the loadingPrincipal if we
> are never going to use the channel for anything else except for getting the
> URI?

Ideally you would use NetUtil.newChannel() API [1]. I know it's confusing because there are so many different ways to create a channel within Gecko. Rule of thumb, use the API within NetUtil.cpp for C++ and NetUtil.jsm for JS to create a channel.

To answer your question: If you are just resolving the URI, it's fine to use the systemPrincipal. From what I have seen in the patch you never call asyncOpen() on the channel. So that's fine.

JWYI: We just extended the nsiChannel API to also include asyncOpen2(), and now are are going to convert all the callsites from asyncOpen() to asyncOpen2() which will then perform all the necessary security checks before actually opening the channel. If at some point you actually need to open that channel, then the systemPrincipal will bypass all the security checks. But do not worry, we will inspect each callsite that opens a channel manually.

To sum it up, it's fine to use SystemPrincipal for now.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#280
Flags: needinfo?(mozilla)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> Ideally you would use NetUtil.newChannel() API [1]. I know it's confusing
> because there are so many different ways to create a channel within Gecko.
> Rule of thumb, use the API within NetUtil.cpp for C++ and NetUtil.jsm for JS
> to create a channel.
> 
> To answer your question: If you are just resolving the URI, it's fine to use
> the systemPrincipal. From what I have seen in the patch you never call
> asyncOpen() on the channel. So that's fine.
> 
> JWYI: We just extended the nsiChannel API to also include asyncOpen2(), and
> now are are going to convert all the callsites from asyncOpen() to
> asyncOpen2() which will then perform all the necessary security checks
> before actually opening the channel. If at some point you actually need to
> open that channel, then the systemPrincipal will bypass all the security
> checks. But do not worry, we will inspect each callsite that opens a channel
> manually.
> 
> To sum it up, it's fine to use SystemPrincipal for now.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#280

Thanks, NetUtil.newChannel(uri).URI replaces the big block needed to do the same thing with the other API
(In reply to Tim Taubert [:ttaubert] from comment #13)
> ::: browser/base/content/browser.js:6831
> (Diff revision 1)
> > +    } else if (isDataURI) {
> > +      this.setMode(this.IDENTITY_MODE_DATA_URI);
> 
> Do we really need a separate state for data: URIs? I would probably just let
> them hit MODE_UNKNOWN. It seems like the average user will never
> intentionally open a data: URI in a tab, I would expect some shady player
> trying something weird if that happens.
> 

I'm fine with removing data uri handling from this bug.

> ::: browser/base/content/browser.js:6829
> (Diff revision 1)
> > +    } else if (isFileURI) {
> > +      this.setMode(this.IDENTITY_MODE_FILE_URI);
> 
> I have a hunch that we should let file: URIs where also the originalURI has
> the file: scheme hit MODE_UNKNOWN. Only if !uri.schemeIs("file") &&
> chanURI.schemeIs("file") we should not show a warning as that would be true
> for all built-in schemes including add-ons. Tricking a user per mail into
> unzipping a file and opening a local .html file pretending to be some page
> sounds too easy.
> 
> IOW: I think this bug should only be about not showing a red "connection is
> not secure" for internal pages stored on the user's machine.

I don't know, a file:// URI (including those loaded from a downloads folder) *is* stored on your computer, so the string is accurate.  I'd argue that it's safer than a chrome:// uri injected from an addon which could (accidentally or purposefully) do more damage because it can execute code at a higher privilege level than a normal file:// uri.  But I'll defer to your opinion about what to show.

Are you hoping that this bug will allow a chrome:// uri injected from an addon to show the "This page is stored on your computer"?  Because the code will need to change to support that - the channel URI ends up being something like this:

jar:file:///Users/foo/Library/Application%20Support/Firefox/Profiles/ddby3ds8.dev/extensions/support@lastpass.com/chrome/lastpass.jar!/content/home2.xul
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #14)
> (In reply to Tim Taubert [:ttaubert] from comment #13)
> > IOW: I think this bug should only be about not showing a red "connection is
> > not secure" for internal pages stored on the user's machine.
> 
> So maybe we'd only want to introduce a single new "internal" state and
> change the copy to say something like "This is an internal page."?

I'm happy with the original proposal which is to have two different states.  I think there's an important distinction between our whitelisted 'about' pages and a chrome:// uri installed by an addon.  If we went with one message then we would have to soften the wording for the whitelisted 'about' pages, getting rid of the "This is a secure Firefox page" message.
(In reply to Brian Grinstead [:bgrins] from comment #18)
> I don't know, a file:// URI (including those loaded from a downloads folder)
> *is* stored on your computer, so the string is accurate.  I'd argue that
> it's safer than a chrome:// uri injected from an addon which could
> (accidentally or purposefully) do more damage because it can execute code at
> a higher privilege level than a normal file:// uri.  But I'll defer to your
> opinion about what to show.

Yeah, I think that makes sense. So we keep continue showing "not secure" for data: URIs but show "stored on your machine" for non-whitelisted URIs resolving to file:, correct?

Should we only check the final URI with a channel when |unknown=true|? It seems that whenever the URL scheme has a host it won't be local file, excluding localhost that we don't really care about here.

> Are you hoping that this bug will allow a chrome:// uri injected from an
> addon to show the "This page is stored on your computer"?  Because the code
> will need to change to support that - the channel URI ends up being
> something like this:
> 
> jar:file:///Users/foo/Library/Application%20Support/Firefox/Profiles/
> ddby3ds8.dev/extensions/support@lastpass.com/chrome/lastpass.jar!/content/
> home2.xul

Oh, right. Well I don't think this is too important for now and we can take a look at this much later if we want.
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #20)
> Should we only check the final URI with a channel when |unknown=true|? It
> seems that whenever the URL scheme has a host it won't be local file,
> excluding localhost that we don't really care about here.

I actually think that `unknown` code isn't doing anything useful right now.  Since it's only detecting an exception when getting host, `unknown` is false for file and chrome uris - the only case I've seen it be true is for data uris.  But in this case, there is an `else { this.setMode(this.IDENTITY_MODE_UNKNOWN); }` condition at the bottom of the block that a data uri falls through to (doing the same thing that happens `if (unknown)`).

The only case it would change the logic of the fn is if `uri.host` throws *and* `state & nsIWebProgressListener.STATE_IS_SECURE` or `state & nsIWebProgressListener.STATE_IS_BROKEN` is true.  I haven't found any case where that would happen, even after spending  some time looking through the history of the change (https://hg.mozilla.org/mozilla-central/rev/14de59ee10a9#l1.109 and Bug  666809).  I would like to remove it, but it seems outside the scope of this bug and I'm not 100% sure it doesn't do anything.

But we could move this logic down into the final `else` to prevent it from creating the channel on every call to checkIdentity even when it's obviously not going to be a file uri.
Comment on attachment 8637662 [details]
MozReview Request: Bug 1178163 - Show connection info for file and data URIs in control center;r=ttaubert

Bug 1178163 - Show connection info for file and data URIs in control center;r=ttaubert
Attachment #8637662 - Flags: review?(ttaubert)
Bug 1178163 - Convert browser_bug590206.js to add_task format;r=ttaubert
Attachment #8639102 - Flags: review?(ttaubert)
Updated the patch based on my best understanding of what we should be showing, let me know if I should change anything about it.  Also added a second patch that just switches that test file to the add_task format.
Looking at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0012902f8711, it looks like there are semi-frequent test timeouts for browser_bug590206.js in Linux debug e10s.  Looking at the logs like [0] it looks like it's probably legitimately timing out and we should either split it or requestLongerTimeout, but the timestamps on the INFO messages seem wrong:

21:01:47 - Test start
21:02:34 - First assertion
21:02:34 - Last assertion (which happens to also be the last one in the test) 
21:02:34 - TEST-UNEXPECTED-FAIL due to timeout

[0]: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bgrinstead@mozilla.com-0012902f8711/try-linux-debug/try_ubuntu32_vm-debug_test-mochitest-e10s-browser-chrome-1-bm03-tests1-linux32-build904.txt.gz
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
Comment on attachment 8639102 [details]
MozReview Request: Bug 1178163 - Convert browser_bug590206.js to add_task format;r=ttaubert

https://reviewboard.mozilla.org/r/14171/#review12889

Ship It!
Attachment #8639102 - Flags: review?(ttaubert) → review+
Attachment #8637662 - Flags: review?(ttaubert) → review+
Comment on attachment 8637662 [details]
MozReview Request: Bug 1178163 - Show connection info for file and data URIs in control center;r=ttaubert

https://reviewboard.mozilla.org/r/13959/#review12891

::: browser/base/content/browser.js:6848
(Diff revision 2)
> +      let resolvedURI = NetUtil.newChannel({uri,loadUsingSystemPrincipal:true}).URI;

nit: {uri, loadUsingSystemPrincipal: true}

::: browser/base/content/browser.js:6855
(Diff revision 2)
> +      if (resolvedURI.schemeIs("file")) {
> +        this.setMode(this.IDENTITY_MODE_FILE_URI);
> +      } else {

Couldn't we just check if (resolvedURI.schemeIs("file") || resolvedURI.schemeIs("jar")) and skip the double-resolve? It seems like jar: would always point to local file and nothing else, no?
(In reply to Tim Taubert [:ttaubert] from comment #27)
> ::: browser/base/content/browser.js:6855
> (Diff revision 2)
> > +      if (resolvedURI.schemeIs("file")) {
> > +        this.setMode(this.IDENTITY_MODE_FILE_URI);
> > +      } else {
> 
> Couldn't we just check if (resolvedURI.schemeIs("file") ||
> resolvedURI.schemeIs("jar")) and skip the double-resolve? It seems like jar:
> would always point to local file and nothing else, no?

I don't think think so:

jar:http://mochi.test:8888/tests/docshell/test/bug369814.zip!/iframes.html
That's crazy. Well let's just keep it then I guess.
https://hg.mozilla.org/mozilla-central/rev/5f27cc13a2c2
https://hg.mozilla.org/mozilla-central/rev/b1151fef26d4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
about:robots still shows "Connection is not secure" is this expected?
Flags: needinfo?(bgrinstead)
I guess this isn't expected but will be fixed in bug 1175702.
Flags: needinfo?(bgrinstead)
I'll add a test for about:robots.
(In reply to Tim Taubert [:ttaubert] from comment #34)
> I'll add a test for about:robots.

Thanks, I think there's an issue with that because uri.host throws so we end up in the `unknown` state.
(In reply to Brian Grinstead [:bgrins] from comment #35)
> (In reply to Tim Taubert [:ttaubert] from comment #34)
> > I'll add a test for about:robots.
> 
> Thanks, I think there's an issue with that because uri.host throws so we end
> up in the `unknown` state.

Yeah, exactly. My patch accidentally fixes that :)
(In reply to Catalin Varga [QA][:VarCat] from comment #32)
> about:robots still shows "Connection is not secure" is this expected?
(In reply to Tim Taubert [:ttaubert] (on PTO, back Aug 31st) from comment #33)
> I guess this isn't expected but will be fixed in bug 1175702.
"Connection is not secure" also on about:cache
Flags: needinfo?(ttaubert)
(In reply to Paul Silaghi, QA [:pauly] from comment #37)
> (In reply to Catalin Varga [QA][:VarCat] from comment #32)
> > about:robots still shows "Connection is not secure" is this expected?
> (In reply to Tim Taubert [:ttaubert] (on PTO, back Aug 31st) from comment
> #33)
> > I guess this isn't expected but will be fixed in bug 1175702.
> "Connection is not secure" also on about:cache

Tim is on PTO. Which version of Firefox did you see this in, and can you please file a new bug for it, and needinfo :tanvi on it?
Flags: needinfo?(ttaubert) → needinfo?(paul.silaghi)
(In reply to :Gijs Kruitbosch from comment #38)
> please file a new bug for it, and needinfo :tanvi on it?
bug 1195757
Flags: needinfo?(paul.silaghi)
Marking this bug as verified fixed on 42.0a2 (2015-08-23).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.