Closed Bug 1184701 Opened 9 years ago Closed 5 years ago

PageThumbsProtocol (moz-page-thumb://) does not work in the content process

Categories

(Firefox :: New Tab Page, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 77
Tracking Status
firefox77 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 3 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

The PageThumbsProtocol calls into PageThumbsStorage.getFilePathForURL(url), which eventually tries to get a handle on the Profile Directory name in order to get a URI to a thumbnail in the profiledir/thumbnails folder. The content process is not able to determine what the Profile Directory is (because it's not supposed to be able to), and so it bails out.
Hey kmag, This is something that Activity Stream needs, and involves having the content process ultimately read images off of the file system from within the profile directory. In the context of sandboxing, and where sandboxing is going, obviously we don't want the content process to access the file system directly (and if I recall correctly, at least on Windows, we actively prevent that from occurring now). Do you have recommendations on how the Activity Stream could approach this? I'm asking you because you commented in https://groups.google.com/forum/#!topic/mozilla.dev.platform/afuc1xY_hik which seems relevant. You suggest using an nsIInputStreamChannel in the content process to implement a protocol handler that needs to get a resource from the parent... how would this work exactly? newChannel2 from nsIProtocolHandler needs to return an nsIChannel, which nsIInputStreamChannel does not necessarily inherit from. Can you give more details about what you meant in that mailing list post for our Activity Stream folks?
Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → usarracini
Sorry for the delay. My backlog is extremely long this week. We're currently working on remoting the moz-extension: protocol (bug 1334550), which may be able to share some implementation logic with this. I'll try to keep that in mind as I'm reviewing it today. That said, if we can't do that easily, the typical way to handle this from JS would be to register a nsIProtocolHandler in the child process. When newChannel is called in the child, you'd create a nsIInputStreamChannel tied to the output of a nsIPipe, and then send an async message to fetch the channel contents on the parent side. When the parent responds with the channel data, you'd write it to the pipe input, and then close the pipe. Also, ideally, that would rely bug 1354847, so we could avoid opening the channel in the parent until the child channel is actually *opened*, but I haven't had time to get back to implementing the JS side of that in the past few weeks.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1373258
The relevant part of the moz-extension remoting would be ExtensionProtocolHandler::NewStream() on the parent and ExtensionProtocolHandler::SubstituteRemoteFileChannel() on the child. NewStream() does some filesystem security checks because any data or URI's received from the child are not trusted to be valid. i.e., we should make sure that regardless of what the child requests, the parent will only ever return thumbnails from the profile that we want to allow content processes to access. ExtensionProtocolHandler::NewStream() returns a FileInputStream which is used to create an AutoIPCStream in NeckoParent::RecvGetExtensionStream(). The AutoIPCStream is sent back to the child. This does not stream data over IPC. Another approach to look into was explained here https://bugzilla.mozilla.org/show_bug.cgi?id=1334550#c35
Just dumping some thoughts here: I think this most likely is blocked on bug 1385306, there seems to be some principal stuff we need to sort out. Right now Activity Stream is a resource url which means it runs with a codeBase principal. The plan is to give Activity Stream a null principal in order to make it fully unprivileged. In the current implementation of the moz-page-thumb protocol handler, we set the protocol flag to URI_DANGEROUS_TO_LOAD (https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/toolkit/components/thumbnails/PageThumbsProtocol.cpp#50) which means we only allow showing thumbnails in pages with system principal (that's why it works with Tiles but not AS). So we either change the protocol to run with all principals i.e remove the URI_DANGEROUS_TO_LOAD flag in the handler (I don't know the security implications of this), or we change Activity Stream to have a system principal (probably not desirable) Gijs or Christoph, do you know if removing the URI_DANGEROUS_TO_LOAD flag for the page thumbs protocol will any security concerns that you can think of?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(ckerschb)
I did a bit more digging, and it turns out if we want to keep URI_SAFE_FOR_UNTRUSTED_CONTENT for newtab with Activity Stream (https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/browser/components/about/AboutRedirector.cpp#224), it's actually not enough to just remove URI_DANGEROUS_TO_LOAD in the protocol handler, but we have to replace it with URI_LOADABLE_BY_ANYONE.
(In reply to Ursula Sarracini (:ursula) from comment #4) > Gijs or Christoph, do you know if removing the URI_DANGEROUS_TO_LOAD flag > for the page thumbs protocol will any security concerns that you can think > of? Yes, it would basically expose the screenshot page thumbnail imagery to webpages. I don't think that's a good idea. I think what will need to happen is AS needs to use its privileged code in content scripts or similar to fetch the data, then stick it into Blobs which get a URL from the unprivileged AS page, then they can be used in the AS page the same way the moz-page-thumb stuff is used now. Ursula, do you think that could work?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(usarracini)
(In reply to Ursula Sarracini (:ursula) from comment #4) > Gijs or Christoph, do you know if removing the URI_DANGEROUS_TO_LOAD flag > for the page thumbs protocol will any security concerns that you can think > of? It seems Gijs already provided a good path forward here. Just to emphasize on what we already know anyway, we can now remove the URI_DANGEROUS_TO_LOAD to make it work, but using some privileged code to fetch the data sounds reasonable to me.
Flags: needinfo?(ckerschb)
Blocks: 1413078
Assignee: usarracini → jlim
Status: NEW → ASSIGNED
Flags: needinfo?(usarracini)
Flags: needinfo?(mconley)
Attached patch bug1184701-prototype.patch (obsolete) (deleted) — Splinter Review
I have attached a patch that allows the moz-page-thumb:// protocol to work in the content process. With this patch, moz-page-thumb:// URIs can only be loaded in the parent process or Activity Stream pages (about:newtab, about:home, about:welcome). I am not quite sure if we should allow other content pages to use moz-page-thumb:// URIs (See caps/nsScriptSecurityManager.cpp). PageThumbProtocolHandler.cpp is very similar to ExtensionProtocolHandler.cpp. Under the hood, it uses SubstitutingProtocolHandler as a base. At the moment, PageThumbProtocolHandler is coded in a way to restrict moz-page-thumb:// URIs to resolve to only file:// URIs. We do not intend to allow substitution rules for moz-page-thumb:// URIs (See ResolveSpecialCases in PageThumbProtocolHandler.cpp). There are two cases here when loading a moz-page-thumb:// URI: a) Loading from a parent process: moz-page-thumb:// URI resolves to a file:// URI and file is loaded. b) Loading from a content process: moz-page-thumb:// URI resolves to the same URI (but with the file:// scheme). We will then substitute the current channel with a remote channel so that the moz-page-thumb:// URI will be serialized and sent to the parent. The parent will then resolve the deserialized moz-page-thumb:// URI to the actual file URI on disk and returns an input stream to the child to load the file.
(In reply to Jay Lim [:imjching] from comment #8) > Created attachment 8991426 [details] [diff] [review] > bug1184701-prototype.patch > > With this patch, moz-page-thumb:// URIs can only be loaded in the parent > process or Activity Stream pages (about:newtab, about:home, about:welcome). > I am not quite sure if we should allow other content pages to use > moz-page-thumb:// URIs (See caps/nsScriptSecurityManager.cpp). I wonder if we could add a flag for it, like this flag: https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/netwerk/protocol/res/nsISubstitutingProtocolHandler.idl#20 but instead to allow the moz-page-thumb protocol in the parent process and privileged content process only (meaning that this should probably be blocked by bug 1472212). > b) Loading from a content process: moz-page-thumb:// URI resolves to the > same URI (but with the file:// scheme). We will then substitute the current > channel with a remote channel so that the moz-page-thumb:// URI will be > serialized and sent to the parent. The parent will then resolve the > deserialized moz-page-thumb:// URI to the actual file URI on disk and > returns an input stream to the child to load the file. That sounds like the right thing to do here, but we should probably get someone from Necko to look this over. Hey mayhemer, would you be the right person to evaluate the approach here?
Flags: needinfo?(mconley) → needinfo?(honzab.moz)
(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #9) > (In reply to Jay Lim [:imjching] from comment #8) > > Created attachment 8991426 [details] [diff] [review] > > bug1184701-prototype.patch > > > > With this patch, moz-page-thumb:// URIs can only be loaded in the parent > > process or Activity Stream pages (about:newtab, about:home, about:welcome). > > I am not quite sure if we should allow other content pages to use > > moz-page-thumb:// URIs (See caps/nsScriptSecurityManager.cpp). > > I wonder if we could add a flag for it, like this flag: > https://searchfox.org/mozilla-central/rev/ > a80651653faa78fa4dfbd238d099c2aad1cec304/netwerk/protocol/res/ > nsISubstitutingProtocolHandler.idl#20 > > but instead to allow the moz-page-thumb protocol in the parent process and > privileged content process only (meaning that this should probably be > blocked by bug 1472212). Hm. Potentially. We can also use the flag `URI_CAN_LOAD_IN_PRIVILEGED_CHILD` (https://reviewboard.mozilla.org/r/254640/diff/3#index_header) and do something similar to this: https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/caps/nsScriptSecurityManager.cpp#801-841. One thing to note is that the flag can only be applied to about:* URIs.
(In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #9) > (In reply to Jay Lim [:imjching] from comment #8) > > Created attachment 8991426 [details] [diff] [review] > > bug1184701-prototype.patch > > > > With this patch, moz-page-thumb:// URIs can only be loaded in the parent > > process or Activity Stream pages (about:newtab, about:home, about:welcome). > > I am not quite sure if we should allow other content pages to use > > moz-page-thumb:// URIs (See caps/nsScriptSecurityManager.cpp). > > I wonder if we could add a flag for it, like this flag: > https://searchfox.org/mozilla-central/rev/ > a80651653faa78fa4dfbd238d099c2aad1cec304/netwerk/protocol/res/ > nsISubstitutingProtocolHandler.idl#20 > > but instead to allow the moz-page-thumb protocol in the parent process and > privileged content process only (meaning that this should probably be > blocked by bug 1472212). We could do that in addition to having the checks in caps/ on a principal basis, but I'd be wary of relying on a flag for the privileged content process on its own - it sounds like that would be game over as soon as (untrusted) web content manages to load inside said privileged content process.
sorry for late answer, I've been quite busy lately. (In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from comment #9) > > b) Loading from a content process: moz-page-thumb:// URI resolves to the > > same URI (but with the file:// scheme). To be honest, I'm a bit confused. "same URI but with file schema"? And what exactly means "to resolve a URI"? > > We will then substitute the current > > channel with a remote channel how? > > so that the moz-page-thumb:// URI will be > > serialized and sent to the parent. The parent will then resolve the > > deserialized moz-page-thumb:// URI to the actual file URI on disk and > > returns an input stream to the child to load the file. > > That sounds like the right thing to do here, but we should probably get > someone from Necko to look this over. Hey mayhemer, would you be the right > person to evaluate the approach here? how exactly does the moz-page-thumb channel work in a single process scenario (on the parent process only)? as I understand it, you translate the URL to a file: uri and use that file: uri to construct an inner file channel, right? Note that file: channel is fully proxied, so you may try to simply open a file channel from a content process, same way as you were on the parent process, and it may transparently work. I don't think we restrict file: uris that much. are implementing nsISubstitutingProtocolHandler?
Flags: needinfo?(honzab.moz) → needinfo?(jlim)
(In reply to Honza Bambas (:mayhemer) from comment #12) > sorry for late answer, I've been quite busy lately. > > (In reply to Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th)) from > comment #9) > > > b) Loading from a content process: moz-page-thumb:// URI resolves to the > > > same URI (but with the file:// scheme). > > To be honest, I'm a bit confused. "same URI but with file schema"? And > what exactly means "to resolve a URI"? > The `PageThumbProtocolHandler` implements `nsISubstitutingProtocolHandler` and extends `SubstitutingProtocolHandler`. Every URI with the moz-page-thumb protocol will be resolved to another URI whenever we create a new channel (https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/netwerk/protocol/res/SubstitutingProtocolHandler.cpp#257-259). I believe this is how moz-extension works as well. URIs will be resolved to contents in a JAR file for packed extensions and a local directory for unpacked extensions. Here's an example of a moz-page-thumb URI: `moz-page-thumb://thumbnails/?url=https%3A//example.org/image.png&revision=2821`. When this URI is requested from: 1. the parent process, we'll resolve it to the actual thumbnail path in the profile directory. For example, `file:///Users/objdir-frontend/tmp/profile-default/thumbnails/example.png` 2. the content process, we'll resolve it to `file://thumbnails/?url=https%3A//example.org/image.png&revision=2821` by just replacing the scheme. > > > We will then substitute the current > > > channel with a remote channel > > how? > For the case of the content process, we'll substitute the channel with a "remote channel" (https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/netwerk/protocol/res/SubstitutingProtocolHandler.h#70-75 and https://pastebin.mozilla.org/9089944). The remote channel is a `SimpleChannel` created through `NS_NewSimpleChannel` and it includes a stream getter, which will request an input stream from the parent. > > > so that the moz-page-thumb:// URI will be > > > serialized and sent to the parent. The parent will then resolve the > > > deserialized moz-page-thumb:// URI to the actual file URI on disk and > > > returns an input stream to the child to load the file. > > > > That sounds like the right thing to do here, but we should probably get > > someone from Necko to look this over. Hey mayhemer, would you be the right > > person to evaluate the approach here? > > how exactly does the moz-page-thumb channel work in a single process > scenario (on the parent process only)? It will not substitute the channel with a `SimpleChannel`. I am not exactly sure how `NS_NewChannelInternal` works under the hood, but here's how I think it works at the moment. After resolving the moz-page-thumb URI to the actual thumbnail file, it will request a file based on the channel created in `NS_NewChannelInternal` (https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/netwerk/protocol/res/SubstitutingProtocolHandler.cpp#272-278). I suppose it will construct an inner file channel after this. > as I understand it, you translate the URL to a file: uri and use that file: > uri to construct an inner file channel, right? Note that file: channel is > fully proxied, so you may try to simply open a file channel from a content > process, same way as you were on the parent process, and it may > transparently work. I don't think we restrict file: uris that much. I think that is how it works for the parent process. For the child process, we'll replace the channel with a `SimpleChannel` to proxy the request to the parent to construct a file channel. > are implementing nsISubstitutingProtocolHandler? Yes What are your thoughts on this approach? The reason why I substituted moz-page-thumb with file is to make sure that it returns a result when we call `NS_NewChannelInternal`. Technically we might be able to skip this by creating some branching conditions and use a `SimpleChannel` right away, but that involves modifying the existing `SubstitutingProtocolHandler` that we have. When that initial channel from `NS_NewChannelInternal` gets substituted, we'll send the original moz-page-thumb URI to the parent, which will then be resolved to the actual thumbnail path.
Flags: needinfo?(jlim)
Flags: needinfo?(honzab.moz)
As I understand, on a content process, you cannot resolve `moz-page-thumb://thumbnails/my-example` to `file:///Users/user/profile/the-thumbnail-file.png` URI, right? You are simply missing the necessary information on the content process and hence, you must proxy to the parent process to find the actual file there. It is fine *for now*, but only when assuming the amount of data serialized back to the child process is not large and that it is not re-requested too often again (is smartly cached, maybe?) Ideally, it would be great to get the file handle on the content process to read it directly. I was involved in reviews of moz-extension protocol proxy here: https://reviewboard.mozilla.org/r/140834/diff/18#index_header It would be nicer to either share that code or go a similar way as a followup bug. It depends on perf and memory impact of the current approach (simple channel). I believe we have tools to measure these days (talos, awsy)? > 2. the content process, we'll resolve it to `file://thumbnails/?url=https%3A//example.org/image.png&revision=2821` by just replacing the scheme. I'm not sure why you have to do this when you still create your own simple channel, but I don't know how exactly the substituting handler works. I assume you SetSubstitutionWithFlags differently for child and parent?
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #14) > As I understand, on a content process, you cannot resolve > `moz-page-thumb://thumbnails/my-example` to > `file:///Users/user/profile/the-thumbnail-file.png` URI, right? You are > simply missing the necessary information on the content process and hence, > you must proxy to the parent process to find the actual file there. Yes, that's right. Technically, we can obtain the file name from the content process, but we won't be able to obtain the profile directory. > Ideally, it would be great to get the file handle on the content process to > read it directly. I was involved in reviews of moz-extension protocol proxy > here: https://reviewboard.mozilla.org/r/140834/diff/18#index_header Are you referring to the file descriptor, just like how JAR files are being loaded in the link that you have there? Do you know why we split into two different approaches (file descriptor and input stream) for the moz-extension protocol? Can we not load the unpacked extension folder using the file descriptor? Likewise, is there a reason to not load the packed JAR file using the input stream? > It would be nicer to either share that code or go a similar way as a > followup bug. It depends on perf and memory impact of the current approach > (simple channel). I believe we have tools to measure these days (talos, > awsy)? Do you know if there are any existing talos/awsy tests that measures the perf and memory impact of Necko channels? > > 2. the content process, we'll resolve it to `file://thumbnails/?url=https%3A//example.org/image.png&revision=2821` by just replacing the scheme. > > I'm not sure why you have to do this when you still create your own simple > channel, but I don't know how exactly the substituting handler works. I > assume you SetSubstitutionWithFlags differently for child and parent? The current implementation attempts to create an internal channel by calling `NS_NewChannelInternal` regardless of whether we will be substituting it in the future: https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/netwerk/protocol/res/SubstitutingProtocolHandler.cpp#257-280. I guess we could add a branching condition here to skip the internal channel creation if we know that we *will* be substituting it with a simple channel in the future.
Flags: needinfo?(honzab.moz)
(In reply to Jay Lim [:imjching] from comment #15) > (In reply to Honza Bambas (:mayhemer) from comment #14) > > As I understand, on a content process, you cannot resolve > > `moz-page-thumb://thumbnails/my-example` to > > `file:///Users/user/profile/the-thumbnail-file.png` URI, right? You are > > simply missing the necessary information on the content process and hence, > > you must proxy to the parent process to find the actual file there. > > Yes, that's right. Technically, we can obtain the file name from the content > process, but we won't be able to obtain the profile directory. > > > Ideally, it would be great to get the file handle on the content process to > > read it directly. I was involved in reviews of moz-extension protocol proxy > > here: https://reviewboard.mozilla.org/r/140834/diff/18#index_header > > Are you referring to the file descriptor, just like how JAR files are being > loaded in the link that you have there? Do you know why we split into two > different approaches (file descriptor and input stream) for the > moz-extension protocol? Can we not load the unpacked extension folder using > the file descriptor? Likewise, is there a reason to not load the packed JAR > file using the input stream? You should direct these questions to the author of that patch. My concern about input streams is that it wastes memory and slows down IPC messaging between the processes. If you have a descriptor directly it's usually cheaper to read data. But please, if the data amount is small (<1k? - totally arbitrary!) then use stream as a fix for this bug. > > > It would be nicer to either share that code or go a similar way as a > > followup bug. It depends on perf and memory impact of the current approach > > (simple channel). I believe we have tools to measure these days (talos, > > awsy)? > > Do you know if there are any existing talos/awsy tests that measures the > perf and memory impact of Necko channels? I don't know. > > > > 2. the content process, we'll resolve it to `file://thumbnails/?url=https%3A//example.org/image.png&revision=2821` by just replacing the scheme. > > > > I'm not sure why you have to do this when you still create your own simple > > channel, but I don't know how exactly the substituting handler works. I > > assume you SetSubstitutionWithFlags differently for child and parent? > > The current implementation attempts to create an internal channel by calling > `NS_NewChannelInternal` regardless of whether we will be substituting it in > the future: > https://searchfox.org/mozilla-central/rev/ > 6f86cc3479f80ace97f62634e2c82a483d1ede40/netwerk/protocol/res/ > SubstitutingProtocolHandler.cpp#257-280. I guess we could add a branching > condition here to skip the internal channel creation if we know that we > *will* be substituting it with a simple channel in the future. It would be better, yes. File channels may do some more work prior opening, that may have some perf impact potentially. And it will also save some memory.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #16) > (In reply to Jay Lim [:imjching] from comment #15) > > (In reply to Honza Bambas (:mayhemer) from comment #14) > > > As I understand, on a content process, you cannot resolve > > > `moz-page-thumb://thumbnails/my-example` to > > > `file:///Users/user/profile/the-thumbnail-file.png` URI, right? You are > > > simply missing the necessary information on the content process and hence, > > > you must proxy to the parent process to find the actual file there. > > > > Yes, that's right. Technically, we can obtain the file name from the content > > process, but we won't be able to obtain the profile directory. > > > > > Ideally, it would be great to get the file handle on the content process to > > > read it directly. I was involved in reviews of moz-extension protocol proxy > > > here: https://reviewboard.mozilla.org/r/140834/diff/18#index_header > > > > Are you referring to the file descriptor, just like how JAR files are being > > loaded in the link that you have there? Do you know why we split into two > > different approaches (file descriptor and input stream) for the > > moz-extension protocol? Can we not load the unpacked extension folder using > > the file descriptor? Likewise, is there a reason to not load the packed JAR > > file using the input stream? > > You should direct these questions to the author of that patch. > > My concern about input streams is that it wastes memory and slows down IPC > messaging between the processes. If you have a descriptor directly it's > usually cheaper to read data. > > But please, if the data amount is small (<1k? - totally arbitrary!) then use > stream as a fix for this bug. Thanks for clarifying! Hey Haik, could I get some thoughts on the questions above? We're trying to fix an existing protocol (moz-page-thumb) so that it works in the content process. Under the hood, it uses SubstitutingProtocolHandler, and the prototype/current patch uses a similar implementation as unpacked extensions in ExtensionProtocolHandler. We're trying to evaluate what's best for the moz-page-thumb protocol. When sending serialized URI over to the parent, I don't think we need to do any security checks other than the scheme itself since we will be mapping the query string to an actual file in the thumbnails directory (which is part of the profile directory) in the parent process.
Flags: needinfo?(haftandilian)
(In reply to Jay Lim [:imjching] from comment #15) > Are you referring to the file descriptor, just like how JAR files are being > loaded in the link that you have there? Do you know why we split into two > different approaches (file descriptor and input stream) for the > moz-extension protocol? The child process instance of ExtensionProtocolHandler requests an input stream when it has to load resources from an unpacked extension (which are bare files), but when it has to load a packed extension (JAR file) it requests an FD from the parent. We couldn't use the input stream approach for remoting access to JAR files because there wasn't support for creating a JARChannel in the child backed by an input stream. The JARChannel implementation depends on having an nsIFile. The solution we used was to have the parent send an FD which is used to create a FileDescriptorFile which is "sideloaded" into a JARChannel as the backing nsIFile. The input stream approach doesn't stream the file's contents over IPC. The remote file input stream uses an FD internally, but doesn't expose that to the child process which was needed for JAR remoting. I don't remember all the details, but check with :baku if you have questions about the efficiency of input stream remoting. > Can we not load the unpacked extension folder using > the file descriptor? A single FD can not be used to open a directory and all the files within it. (In reply to Jay Lim [:imjching] from comment #17) > Hey Haik, could I get some thoughts on the questions above? We're trying to > fix an existing protocol (moz-page-thumb) so that it works in the content > process. Under the hood, it uses SubstitutingProtocolHandler, and the > prototype/current patch uses a similar implementation as unpacked extensions > in ExtensionProtocolHandler. We're trying to evaluate what's best for the > moz-page-thumb protocol. When sending serialized URI over to the parent, I > don't think we need to do any security checks other than the scheme itself > since we will be mapping the query string to an actual file in the > thumbnails directory (which is part of the profile directory) in the parent > process. OK, thanks for the explanation. Sounds like you just need to make sure you can only resolve those URIs to files in the thumbnails directory, but I don't know if checking the scheme is sufficient. Which means the main question to answer is if a malformed URI sent from the child could result in a file being loaded from outside of the thumbnails directory. For example, if the substitution is set up to map from "thumbs://" -> "file:///Users/foo/thumbs/", what happens if a URI such as "thumbs:/../private.data" is sent from the child? The ExtensionProtocolHandler does its checks in ExtensionProtocolHandler::NewFD() and ExtensionProtocolHandler::NewStream(). For the JAR case, the host of the URI maps directly to a JAR file so we can call GetSubstitution() to get the JAR URI and not have to check it is within a certain place on the filesystem.
Flags: needinfo?(haftandilian)
(In reply to Haik Aftandilian [:haik] from comment #18) Thanks for the explanation above! > OK, thanks for the explanation. Sounds like you just need to make sure you > can only resolve those URIs to files in the thumbnails directory, but I > don't know if checking the scheme is sufficient. Which means the main > question to answer is if a malformed URI sent from the child could result in > a file being loaded from outside of the thumbnails directory. For example, > if the substitution is set up to map from "thumbs://" -> > "file:///Users/foo/thumbs/", what happens if a URI such as > "thumbs:/../private.data" is sent from the child? Noted on that. In the example above, it should resolve to an invalid file. A valid moz-page-thumb URI should contain three parts: 1) the scheme: "moz-page-thumb" 2) the host: "thumbnails" 3) the URL, which is part of the query string (i.e. ?url=https%3A//example.org/image.png). If any of those three parts are missing or there are mismatches in those literal strings, we will return an error to the child. Otherwise, the provided URL from (3) will be converted into a hash (filename), which should map to a file in the thumbnails directory of the profile directory. If the file is not found, it will fail to create a channel for the file.
(In reply to Jay Lim [:imjching] from comment #19) > Otherwise, the > provided URL from (3) will be converted into a hash (filename), which should > map to a file in the thumbnails directory of the profile directory. If the > file is not found, it will fail to create a channel for the file. Sounds good. I like the hash approach.
Blocks: 1497996

I don't think we want to expose this protocol to normal content processes - just to the privileged content process.

Depends on: 1522879
Assignee: jay → nobody
Status: ASSIGNED → NEW
Depends on: 1513045
No longer depends on: 1522879

Hello!

This bug has been closed due to inactivity and/or the potential for this bug to no longer be an issue with the new Discovery Stream-powered New Tab experience.

Please help us triage by reopening if this issue still persists and should be addressed.

Thanks!

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX

We might need this for bug 1614351.

Blocks: 1614351
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Marking as P2 to get it prioritized. Let us know if you'd like to P it differently depending on how you're organizing this work. Thanks!

:mconley - do you mind if we assign this bug to you so that it cleans up our triage/bug process for New Tab component?

Flags: needinfo?(mconley)
Priority: -- → P2

Yeah, I can take this for now, though it'll probably end up being a latter part of the about:home stuff I'm doing.

Assignee: nobody → mconley
Flags: needinfo?(mconley)
Attachment #9135828 - Attachment description: Bug 1184701 - [WIP] Make the moz-page-thumb protocol work in the privileged about content process. → Bug 1184701 - Make the moz-page-thumb protocol work in the privileged about content process. r?haik,valentin
Attachment #9135828 - Attachment description: Bug 1184701 - Make the moz-page-thumb protocol work in the privileged about content process. r?haik,valentin → Bug 1184701 - Make the moz-page-thumb protocol work in the privileged about content process. r?haik!,valentin!
Attachment #8991426 - Attachment is obsolete: true
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(valentin.gosu)

Thanks all, I'll land this after the next uplift.

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f753bddc4131 Make the moz-page-thumb protocol work in the privileged about content process. r=haik,valentin https://hg.mozilla.org/integration/autoland/rev/241a6d4f2f23 Use moz-page-thumb protocol for about: pages when using the privileged about content process. r=Mardak
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/557d5681e1bd Make the moz-page-thumb protocol work in the privileged about content process. r=haik,valentin https://hg.mozilla.org/integration/autoland/rev/27f9b71660e0 Use moz-page-thumb protocol for about: pages when using the privileged about content process. r=Mardak https://hg.mozilla.org/integration/autoland/rev/3bc7f9b08014 Adjust some newtab unit tests for new Screenshots behaviour. r=Mardak
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 77
Blocks: 1647486
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: