1317921 - Handle nested file URIs with the file content process

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

Logic for choosing the file content process in bug 1147911 handles simple file:// (and view-source:file://) URIs.

Before we can start to remove read access from the web content process, we also need to work out which nested URIs that include file: will end up needing file access and make sure they end up in the file content process.

We need to be careful that this doesn't open up holes where web content can use nested URIs to get file read access.

Comment 1

[Bob Owen (:bobowen)]

Having looked at the various uses of nested URI and given that we want to be conservative about what gets run in the file content process, I think we should just check the innermost URI's scheme for nested URIs.

Comment 2

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1042b6ba5462886170716afb7e56c11708f48c89

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 1: Handle nested URIs properly in E10SUtils.jsm

This should hopefully be the last of these remote type reviews for a while. :-)

As a fair few of the code paths were now creating a real URI I thought it made sense to do that up front and create a separate function that we can transition to using.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 2: Check that nested URIs are loaded in the correct remote type

Comment 5

[:Gijs (out for now; he/him)]

Comment on attachment 8832814 [details] [diff] [review]
Part 1: Handle nested URIs properly in E10SUtils.jsm

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

This doesn't look wrong per se, but I have questions so no r+ for now...

::: browser/modules/E10SUtils.jsm
@@ +75,5 @@
> +      uri = Services.io.newURI(aURL);
> +    } catch (e) {
> +      // If we have an invalid URI we are going to fail to load or display an
> +      // error, safest thing is to return the default remote type.
> +      console.error(`Invalid URI string passed: ${aURL} : `, e);

Nit: we don't normally use console.error in chrome code. Consider Cu.reportError(new Error(`...`)); instead, though note that either way, this will have 'false positives', see below, so it might not make sense to throw an error here...

@@ +76,5 @@
> +    } catch (e) {
> +      // If we have an invalid URI we are going to fail to load or display an
> +      // error, safest thing is to return the default remote type.
> +      console.error(`Invalid URI string passed: ${aURL} : `, e);
> +      return DEFAULT_REMOTE_TYPE;

Uh, so, this comment is unfortunately not true. You can pass e.g. "google.com" and we'll probably correct that to "http://google.com" somewhere further down the line.

While ideally all that kind of correction/sanitization happens before this point, I don't think we can rely on that being the case right now.

I'm not sure what that means for what we do here. Note that there are cases which would have passed the previous code because they start with a particular scheme/protocol prefix, but that will fail the newURI constructor. Not sure if we shouldn't keep using the preferred remote type. Thoughts?

@@ +86,2 @@
>  
> +  getRemoteTypeForNsIURI(aURI, aMultiProcess,

Ah, camel-case and "nsI"... could we just use "getRemoteTypeForURIObject" ? That is slightly less ugly and matches what we use for e.g. document.documentURIObject (which also returns an nsIURI). :-)

@@ +149,5 @@
> +
> +      default:
> +        // For any other nested URIs, we use the innerURI to determine the
> +        // remote type. In theory we should use the innermost URI, but some
> +        // URIs have fake inner URIs so we can't. They will need to get caught

Can you elaborate on "fake inner URIs", and why that problem doesn't happen if we go "one step at a time", unwrapping the innerURIs, but does occur if we go straight for the innermostURI?

Comment 6

[:Gijs (out for now; he/him)]

Comment on attachment 8832815 [details] [diff] [review]
Part 2: Check that nested URIs are loaded in the correct remote type

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

This looks like it could/should be a unit (xpcshell) test in browser/modules/test(s?)/ instead. :-)

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 1: Handle nested URIs properly in E10SUtils.jsm

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8833924 [details] [diff] [review]
Part 1: Handle nested URIs properly in E10SUtils.jsm

Thanks Gijs.

(In reply to :Gijs from comment #5)
> Comment on attachment 8832814 [details] [diff] [review]
> Part 1: Handle nested URIs properly in E10SUtils.jsm

> > +      console.error(`Invalid URI string passed: ${aURL} : `, e);
> 
> Nit: we don't normally use console.error in chrome code. Consider
> Cu.reportError(new Error(`...`)); instead, though note that either way, this
> will have 'false positives', see below, so it might not make sense to throw
> an error here...

Removed.

> @@ +76,5 @@
> > +    } catch (e) {
> > +      // If we have an invalid URI we are going to fail to load or display an
> > +      // error, safest thing is to return the default remote type.
> > +      console.error(`Invalid URI string passed: ${aURL} : `, e);
> > +      return DEFAULT_REMOTE_TYPE;
> 
> Uh, so, this comment is unfortunately not true. You can pass e.g.
> "google.com" and we'll probably correct that to "http://google.com"
> somewhere further down the line.
> 
> While ideally all that kind of correction/sanitization happens before this
> point, I don't think we can rely on that being the case right now.
> 
> I'm not sure what that means for what we do here. Note that there are cases
> which would have passed the previous code because they start with a
> particular scheme/protocol prefix, but that will fail the newURI
> constructor. Not sure if we shouldn't keep using the preferred remote type.
> Thoughts?

While we seem to call the fixup code a few times in the parent, we don't actually do/use the fixed-up URI until we get into the child.
That doesn't seem ideal, but not really to do with this bug.

I don't think we should use aPreferredRemoteType, in case it can ever happen that the fix-up resolves to something we wouldn't want to run there.
So using the default remote type, which is typically where any fixed-up URI would need to run anyway, seems the best option.

If it's something like ile:///c:/some/file, we end up doing an extra round trip through the child, but I think that's an acceptable trade-off.
 
> > +  getRemoteTypeForNsIURI(aURI, aMultiProcess,
> 
> Ah, camel-case and "nsI"... could we just use "getRemoteTypeForURIObject" ?

Definitely an improvement, thanks.

> @@ +149,5 @@
> > +
> > +      default:
> > +        // For any other nested URIs, we use the innerURI to determine the
> > +        // remote type. In theory we should use the innermost URI, but some
> > +        // URIs have fake inner URIs so we can't. They will need to get caught
> 
> Can you elaborate on "fake inner URIs", and why that problem doesn't happen
> if we go "one step at a time", unwrapping the innerURIs, but does occur if
> we go straight for the innermostURI?

Hopefully I've improved this comment.

Comment 9

[Bob Owen (:bobowen)]

Attachment: Part 2: Check that nested URIs are loaded in the correct remote type

I think this is my first xpcshell test. :-)

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 8833924 [details] [diff] [review]
Part 1: Handle nested URIs properly in E10SUtils.jsm

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

::: browser/modules/E10SUtils.jsm
@@ +151,5 @@
> +      default:
> +        // For any other nested URIs, we use the innerURI to determine the
> +        // remote type. In theory we should use the innermost URI, but some URIs
> +        // have fake inner URIs (e.g. about URIs with inner moz-safe-about) so
> +        // we can't. Any URIs like this will need to be handled in the cases

Ah, so maybe the comment should explicitly say something like: ... and if such URIs are wrapped in other nested schemes like view-source:, we don't want to "skip" past "about:" by going straight to the innermost URI.

Anyway, it makes more sense to me now.

@@ +156,5 @@
> +        // above, so we don't still end up using the fake inner URI here.
> +        if (aURI instanceof Ci.nsINestedURI) {
> +          let innerURI = aURI.QueryInterface(Ci.nsINestedURI).innerURI;
> +          return this.getRemoteTypeForURIObject(innerURI, aMultiProcess,
> +                                             aPreferredRemoteType);

Nit: indentation.

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8833931 [details] [diff] [review]
Part 2: Check that nested URIs are loaded in the correct remote type

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

::: browser/modules/test/xpcshell/test_E10SUtils_nested_URIs.js
@@ +63,5 @@
> +]
> +
> +for (let testCase of TEST_CASES) {
> +  for (let preferredRemoteType of TEST_PREFERRED_REMOTE_TYPES) {
> +    add_task(function*() {

There's no yielding, so I don't think you need the add_task? You can probably just stick all of this in a run_test() function

@@ +87,5 @@
> +      let plainStr = plainUri.scheme == "about" ? plainUri.spec
> +                                                : plainUri.scheme + ":";
> +      equal(nestedRemoteType, plainRemoteType,
> +            `Check that ${nestedStr} loads in same remote type as ${plainStr}`
> +            + ` with preferred remote type: ${preferredRemoteType}`);

Note that you could, I think, just used nestedURL and plainURL.spec here? I don't think it'd make the assertions much less clear, and it saves you work. :-)

Comment 12

[Bob Owen (:bobowen)]

(In reply to :Gijs from comment #10)
> Comment on attachment 8833924 [details] [diff] [review]

> > +        // For any other nested URIs, we use the innerURI to determine the
> > +        // remote type. In theory we should use the innermost URI, but some URIs
> > +        // have fake inner URIs (e.g. about URIs with inner moz-safe-about) so
> > +        // we can't. Any URIs like this will need to be handled in the cases
> 
> Ah, so maybe the comment should explicitly say something like: ... and if
> such URIs are wrapped in other nested schemes like view-source:, we don't
> want to "skip" past "about:" by going straight to the innermost URI.

Added this extra bit.

> @@ +156,5 @@
> > +        // above, so we don't still end up using the fake inner URI here.
> > +        if (aURI instanceof Ci.nsINestedURI) {
> > +          let innerURI = aURI.QueryInterface(Ci.nsINestedURI).innerURI;
> > +          return this.getRemoteTypeForURIObject(innerURI, aMultiProcess,
> > +                                             aPreferredRemoteType);
> 
> Nit: indentation.

Doh, fixed this and the other two cases, thanks,

(In reply to :Gijs from comment #11)
> Comment on attachment 8833931 [details] [diff] [review]

> > +for (let testCase of TEST_CASES) {
> > +  for (let preferredRemoteType of TEST_PREFERRED_REMOTE_TYPES) {
> > +    add_task(function*() {
> 
> There's no yielding, so I don't think you need the add_task? You can
> probably just stick all of this in a run_test() function

Ah, yes that works fine, thanks.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/662482ab9459
Part 1: Handle nested URIs properly in E10SUtils.jsm. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7651c30f604
Part 2: Check that nested URIs are loaded in the correct remote type. r=Gijs

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/662482ab9459
https://hg.mozilla.org/mozilla-central/rev/d7651c30f604