Since we are in the process of rolling out a sandbox for each platform, it is not only important to make it as tight as possible but also make sure that we don't have any unintended behavior that would allow to bypass the sandbox restrictions. IPC is probably going to be the first point of attack when trying to escalate from a compromised content process. Vulnerable Part ==================== I took a look at one particular IPC message from the PBackground protocol, called PFileSystemRequest [1] due to its name. The argument for that message is defined as follows: > union FileSystemParams > { > FileSystemCreateDirectoryParams; > FileSystemCreateFileParams; > FileSystemGetDirectoryListingParams; > FileSystemGetFilesParams; > FileSystemGetFileOrDirectoryParams; > FileSystemRemoveParams; > }; Now looking at that protocol in more detail, it is basically possible to bypass all of our file system restrictions implemented in our sandbox (no matter the platform), currently write access to the file system is restricted on the latest Linux nightly build. However, given a compromised content process, this restriction can be bypassed using the above mentioned message. I looked at one request in particular namely FileSystemCreateFileParams. Now there are some security checks performed on the parent side, but only for devicestorage access. However, the critical part here is that no security checks whatsoever are performed for native file system access. I.e. if the compromised child supplies a path like "/home/<user>/.bashrc" as part of FileSystemCreateFileParam, it can write whatever it likes into that file, since the file content to be written is supplied with that message as well. The API is powerful enough to even allow overwriting existing files... This functions [2] checks the path coming from the child, if it doesn't start with "devicestorage-" a OSFileSystemParent object is created and returned, and as part of the constructor, a member variable is set which gives unrestricted access [3]. Furthermore, a last check before any operation is performed on the file path coming from the child here [4] leads to a simple 'return true' here [5]. Thus allowing any file to be overwritten/created as long as the user that is running Firefox has access. This in itself is already a problem and ideally this message should be removed entirely, or at least get a whitelist restriction on the parent side. Privilege Escalation ==================== Since I wanted to see if it is possible to gain code execution, I decided to get a PoC that would allow me to run arbitrary code with the parent privileges. Attached is a patch that allows to trigger the PoC, following some simple steps. 1. Compile Firefox with the patch applied 2. Run firefox and open some website (e.g. google.com) 3. Open the web console (Tools -> Web Developer -> Web Console) 4. Run inside the web console: window.Directory.triggerIPC("<path to file you want to create/overwrite>"); When calling triggerIPC(), the PFileSystemRequest message will be sent accross the channel to the parent, the targeted file is specified as the first argument. The content of the file is an ELF shared library that declares a constructor: > #include <stdlib.h> > > __attribute__((constructor)) static void constr(void) > { > unsetenv("LD_PRELOAD"); > unsetenv("LD_LIBRARY_PATH"); > system("id"); > } I decided to overwrite one of the libraries that are shipped with Firefox (libxul.so) because the parent process is probably allowed to write in that directory where those files reside and given a compromised content process, figuring out where the Firefox root directory is located is a trivial task. So as a path I supplied: > "/home/tedd/projects/build-dev/toolkit/library/libxul.so" This overwrites the existing libxul.so and the next time Firefox is started, it will load that library, and as part of the initialization, the libraries constructor function is executed. For test purpose, I execute 'id'. Firefox will not be able to start, but you will see the output of 'id' on your terminal. I decided to go with a Firefox library because it is probably the most reliable way, but there may be other more clever ways to do it. [1] http://searchfox.org/mozilla-central/rev/9bbb2bd7c715d9a76e0a0bde043dad41021ea7c9/ipc/glue/PBackground.ipdl#105 [2] http://searchfox.org/mozilla-central/rev/9bbb2bd7c715d9a76e0a0bde043dad41021ea7c9/dom/filesystem/FileSystemBase.cpp#18 [3] http://searchfox.org/mozilla-central/rev/9bbb2bd7c715d9a76e0a0bde043dad41021ea7c9/dom/filesystem/OSFileSystem.cpp#115 [4] http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/dom/filesystem/CreateFileTask.cpp#271 [5] http://searchfox.org/mozilla-central/rev/9bbb2bd7c715d9a76e0a0bde043dad41021ea7c9/dom/filesystem/OSFileSystem.h#94

Looks like some sort of B2G api that's still around. According to the docs it's supposed to be virtualized and you're not supposed to be able to create files. "The API doesn't give you access to the local file system, nor is the sandbox really a section of the file system. Instead, it is a virtualized file system that looks like a full-fledged file system to the web app. It does not necessarily have a relationship to the local file system outside the browser." https://developer.mozilla.org/en-US/docs/Web/API/File_and_Directory_Entries_API/Introduction https://developer.mozilla.org/en-US/docs/Web/API/File_and_Directory_Entries_API https://developer.mozilla.org/en-US/docs/Web/API/FileSystem Baku, can you provide some background on this api?

Jim, you are right. This is a B2G API. It was still in use for Connected-Devices, but I guess now we can remove it. We keep the read-only part of it, because it's used by Entries API and Directory Upload API.

:baku, keeping the read-only part should then be re-written somehow, because the next step for sandboxing (at least on Linux) is to lock down read access and this would allow to bypass the sandbox.

(ni for comment 3)

This is in 52 which is the subject version for pwn2own in two weeks. Wondering if we should hold up the 52 release or potentially target a point release for a fix here.

Baku, is there a pref associated with this feature? Poking through the code I don't see one, but I might have missed something.

(In reply to Jim Mathies [:jimm] from comment #6) > Baku, is there a pref associated with this feature? Poking through the code > I don't see one, but I might have missed something. Just found 'device.storage.enabled' which is false for the desktop browser. Which may mean this api is disabled by default. Need to confirm this however through some testing.

hey :jimm, 'device.storage.enabled' does not fix this, the only it does (when set to true) expose more to JavaScript, see [1]. So with device storage enabled you could probably access this with legitimate JS code, if you could still create/write arbitrary files, I don't know, I haven't paid attention to that when auditing the code, but I saw that there was some checking done and the child code didn't seem to allow full OS paths. However, the problem here is that the underlying IPC/IPDL protocol is compiled into the code and since PFileSystemRequest is part of PBackground a compromised child still has access to it. [1] http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/dom/webidl/Directory.webidl#45

Tracking for 52 onwards. Would pwn2own use 52 or, if we end up with a dot release/chemspill for this, would they use 52.0.1?

Heads up to Julien.

(In reply to Jim Mathies [:jimm] from comment #5) > This is in 52 which is the subject version for pwn2own in two weeks. > Wondering if we should hold up the 52 release or potentially target a point > release for a fix here. I don't see a patch, how much work/risk would it be to fix? I'd hate to make all the other good security fixes in 52 wait (especially an ASLR bypass fix). Since this is an API it's hard to imagine it's going to be a one-liner or other small fix, unless the fix is just removing the problem code because we don't need it. Sounds like a better idea for a point release. We're only 9 or 10 days from the pwn2own conference though so no guarantee a point release is going to make it on to their contest VMs (in fact I'm only "reasonably" confident 52 itself will).

Minimally-invasive patch that makes the parent drop requests for file/dir creation and removals, but doesn't remove all the supporting code. This prevented a Mac version of the PoC code from working for me, but I don't have a good of sense of whether or these API's are needed right now. The idea being use this as a quick fix and follow up with a complete fix removing all supporting code. Comments?

This bug is now resolved in nightly, I removed DeviceStorage API.

With this patch, the content process is killed if one of this IPC called is used when devicestorage is not enabled. To be applied on m-a, m-b and ESR-52.

:baku, I gave this a quick look and I don't think this is going to fix the issue. Since that code path is not taken (I believe) I will apply the patch and see what is happening.

Comment on attachment 8845326 [details] [diff] [review] kill.patch Just confirmed, this patch does not fix the issue. So you CheckPermission runnable is instantiated and dispatched here [1]. However the actual code path taken is that if-statement in that same function body here [2]. So the 'exploit' is taking a different path. [1] http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/ipc/glue/BackgroundParentImpl.cpp#881-885 [2] http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/ipc/glue/BackgroundParentImpl.cpp#862-865

The code path is as follows: 1. AllocPFileSystemRequestParent() [1] is called when PFileSysteRequestConstructor message is sent (note this is called before RecvPFileSystemRequestConstructor() is called. 2. A new FileSystemRequestParent object is created and Initialize() [1] is called. 3. Initialize() dispatches based on the type [2] 4. mFileSystem is initialized with the return value of DeserializeDOMPath() [3] Now here comes the important part, unless the path starts with "devicestorage-" (which it doesn't, let's assume it is "/home/tedd/somefile") a new OSFileSystemParent object is created [3]. 5. The constructor sets the variable mPermissionCheckType to ePermissionCheckNotRequired[4] This leads to the path taken that I mentioned in Comment 16 because this check: > if (actor->PermissionCheckType() == FileSystemBase::ePermissionCheckNotRequired) { is true. [1] http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/ipc/glue/BackgroundParentImpl.cpp#837,845 [2] http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/dom/filesystem/FileSystemRequestParent.cpp#33,53 [3] http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/dom/filesystem/FileSystemBase.cpp#18,45 [4] http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/dom/filesystem/OSFileSystem.cpp#112,115

I meant to say 'when PFileSysteRequestConstructor message is received on the parent side' not 'sent'

Comment on attachment 8845326 [details] [diff] [review] kill.patch Review of attachment 8845326 [details] [diff] [review]: ----------------------------------------------------------------- r- since this doesn't fix the issue. Also I don't think a surgical fix is the way to go here. Why don't we actually delete the whole code for device storage here, at least at the IPC layer?

> r- since this doesn't fix the issue. Also I don't think a surgical fix is > the way to go here. Why don't we actually delete the whole code for device > storage here, at least at the IPC layer? All this code is gone since 2 days ago. This fix was meant to be for m-a in case we don't want to uplift bug 1299500.

If we are talking about FileSystem IPC calls or read/write operations on OS files, this bug is already fixed because DeviceStorage API is gone and now, with the Entries API, we allow just the reading of files. This means that, content process can have access to any file on the OS but in readonly. This readonly access is granted because <input type="file"> happens on the child process. If we want to improve the Sandbox security, we could combine FilePicker and following File APIs: What about if we introduce a parent-only call to assign local paths to a tab context, so that, when the FilePicker is opened and a dir/file is chosen, we store with the local path and the tab context. Any following file-related IPC calls, will check if the tab context has access to that local path.

(In reply to Andrea Marchesini [:baku] from comment #20) > > r- since this doesn't fix the issue. Also I don't think a surgical fix is > > the way to go here. Why don't we actually delete the whole code for device > > storage here, at least at the IPC layer? > > All this code is gone since 2 days ago. > This fix was meant to be for m-a in case we don't want to uplift bug 1299500. Yes, I was talking about a branch fix. Sorry if I wasn't clear. I was assuming you weren't even going to land this on m-c as I reviewed those patches as well. (In reply to Andrea Marchesini [:baku] from comment #21) > If we want to improve the Sandbox security, we could combine FilePicker and > following File APIs: What about if we introduce a parent-only call to assign > local paths to a tab context, so that, when the FilePicker is opened and a > dir/file is chosen, we store with the local path and the tab context. Any > following file-related IPC calls, will check if the tab context has access > to that local path. That sounds like a great idea to me. (We should only grant access to the specific file chosen unless if the directory picker was used, not the entire directory.)

Can you please CC me on bug 1344957 as well? Our bugzilla security is working a bit too well! ;-)

Just to make it clear, this was addressed in build 55 with the fix for Bug 1299500 - Get rid of DeviceStorage API.

Comment on attachment 8847111 [details] [diff] [review] patch Review of attachment 8847111 [details] [diff] [review]: ----------------------------------------------------------------- The backport of this patch should be handled with care. For one thing, the patch basically describes the security issue. The code changes also make it moderately risky... I can't think of a better way to do this unfortunately. r=me with the below addressed. ::: dom/base/nsContentUtils.cpp @@ +7890,5 @@ > continue; > } > > + if (aParent) { > + bool isDir; Nit: init to false, please. ::: dom/filesystem/FileSystemRequestParent.cpp @@ +114,5 @@ > + NS_IMETHOD > + Run() override > + { > + if (NS_IsMainThread()) { > + NullifyContentParentRAII raii(mContentParent); Nit: MakeScopeExit() is a cleaner way to do things like this. ::: dom/filesystem/FileSystemSecurity.cpp @@ +19,5 @@ > + > +/* static */ already_AddRefed<FileSystemSecurity> > +FileSystemSecurity::Get() > +{ > + MOZ_ASSERT(NS_IsMainThread()); All of the methods here require *release* assertions ensuring they run in the parent process. ::: dom/filesystem/FileSystemSecurity.h @@ +25,5 @@ > + static already_AddRefed<FileSystemSecurity> > + GetOrCreate(); > + > + void > + Store(ContentParentId aId, const nsAString& aDirectoryPath); Store() is a pretty bad name for this function. :-) How about GrantAccessToContentProcess()? @@ +31,5 @@ > + void > + Forget(ContentParentId aId); > + > + bool > + PathAllowed(ContentParentId aId, const nsAString& aPath); How about calling this ContentProcessHasAccessTo()? ::: dom/ipc/ContentParent.cpp @@ +1680,5 @@ > ProcessHangMonitor::RemoveProcess(mHangMonitorActor); > mHangMonitorActor = nullptr; > } > > + RefPtr<FileSystemSecurity> fss = FileSystemSecurity::GetOrCreate(); How can we not have a FileSystemSecurity object here? I think you should instead assert that Get() returns non-null.

> How can we not have a FileSystemSecurity object here? Well, maybe the contentProcess was not sharing any file. Get() would return null.

[Security approval request comment] How easily could an exploit be constructed based on the patch? Not easy at all. the child process must be completely hacked and a custom IPC message must be sent from child to parent in order to retrieve a File object. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The patch is about introducing a security model for sharing granted files cross processes. It's generic enough. Which older supported branches are affected by this flaw? We should backport it, but I want to wait a couple of days before asking this: I want to see if there are regressions in nightly: if the file is not allowed, the child process is killed. If not all supported branches, which bug introduced the flaw? Entries API + DeviceStorage API. How likely is this patch to cause regressions; how much testing does it need? I think I covered (and tested) all the entry-points for the fileSystem APIs. If I forgot something, the child process crashes and we can quickly fix.

sec-approval+ for trunk. We'll want branch patches made and nominated as well.

Adding NI for myself to take a look at the new patch as requested in sandboxing standup today.

(In reply to Haik Aftandilian [:haik] from comment #30) > Adding NI for myself to take a look at the new patch as requested in > sandboxing standup today. If you have cyclces to do anothee review pass, I'd certainly appreciate it. I think this patch is complex enough that it could definitely use another pair of eyes looking over it. :-)

We're hoping to ship a 52.0.2 dot release some time next week, is this still something we should consider including, or would it be better to leave it until 53?

Comment on attachment 8847488 [details] [diff] [review] patch Review of attachment 8847488 [details] [diff] [review]: ----------------------------------------------------------------- The way we handle paths right now would still allow a compromised content process to access arbitrary paths using the ../ trick. See below. ::: dom/base/nsContentUtils.cpp @@ +7889,5 @@ > + if (aParent) { > + bool isDir = false; > + if (NS_SUCCEEDED(file->IsDirectory(&isDir)) && isDir) { > + nsAutoString path; > + if (NS_WARN_IF(NS_FAILED(file->GetPath(path)))) { I don't understand exactly how nsContentUtils::TransferableToIPCTransferable is used, but it looks like the data is coming from a drag event or file dialog which comes from the parent so it's safe to grant access to this file or directory. Is that right? ::: dom/filesystem/FileSystemSecurity.cpp @@ +56,5 @@ > +} > + > +void > +FileSystemSecurity::GrantAccessToContentProcess(ContentParentId aId, > + const nsAString& aDirectoryPath) We should make sure aDirectoryPath doesn't include symlinks. If it ended up pointing at a link, then the target directory could change. nsIFile has a Normalize() method that should do the trick, but we need to look at it really closely. @@ +94,5 @@ > + return false; > + } > + > + for (uint32_t i = 0, len = paths->Length(); i < len; ++i) { > + if (FileSystemUtils::IsDescendantPath(paths->ElementAt(i), aPath)) { FileSystemUtils::IsDescendantPath doesn't appear to be suitable for security checks like this. For example, I think it would consider "/home/foo/../bar" as a descendant path of "/home/foo". So we'd have to be sure that the paths we're checking are already sanitized and resolved (in terms of links). nsIFile::Normalize() should also help with this, but again we need to look it at carefully. ::: dom/filesystem/FileSystemUtils.cpp @@ +20,5 @@ > } // anonymous namespace > > /* static */ bool > +FileSystemUtils::IsDescendantPath(const nsAString& aPath, > + const nsAString& aDescendantPath) As mentioned above, this will only work reliably if the paths have been sanitized to not include .. and links.

(In reply to Julien Cristau [:jcristau] from comment #33) > We're hoping to ship a 52.0.2 dot release some time next week, is this still > something we should consider including, or would it be better to leave it > until 53? Seems like this needs Aurora/Beta/ESR52 approval requests in general. And release if desired for 52.0.2 dot release consideration.

> The way we handle paths right now would still allow a compromised content > process to access arbitrary paths using the ../ trick. See below. The normalization happens in the content process. If the content process sends a path with '..', it must be killed immediately. > or directory. Is that right? Right. > We should make sure aDirectoryPath doesn't include symlinks. If it ended up > pointing at a link, then the target directory could change. We support symlinks in Entries API. We must support them. > FileSystemUtils::IsDescendantPath doesn't appear to be suitable for security > checks like this. For example, I think it would consider "/home/foo/../bar" > as a descendant path of "/home/foo". So we'd have to be sure that the paths I disagree: a filePicker always returns a normalized path. We use it comparing what we receive in the IPC message. If the IPC message contains '..', clearly something wrong is happening and the child process must be killed.

(In reply to Andrea Marchesini [:baku] from comment #36) > > The way we handle paths right now would still allow a compromised content > > process to access arbitrary paths using the ../ trick. See below. > > The normalization happens in the content process. If the content process > sends a path with '..', it must be killed immediately. Agree. Is that what we're doing now? > > or directory. Is that right? > > Right. > > > We should make sure aDirectoryPath doesn't include symlinks. If it ended up > > pointing at a link, then the target directory could change. > > We support symlinks in Entries API. We must support them. OK. Sorry, I meant to say that the resolved symlink path should always be used when checking if dir A is a subdirectory of dir B. And we should just resolve the path once. > > FileSystemUtils::IsDescendantPath doesn't appear to be suitable for security > > checks like this. For example, I think it would consider "/home/foo/../bar" > > as a descendant path of "/home/foo". So we'd have to be sure that the paths > > I disagree: a filePicker always returns a normalized path. We use it > comparing what we receive in the IPC message. > If the IPC message contains '..', clearly something wrong is happening and > the child process must be killed. We want to protect against the case when the content process is compromised and is sending bogus paths in an attempt to be able to write to locations on the filesystem we don't want to allow. A compromised content process could send a path that includes "..".

> > The normalization happens in the content process. If the content process > > sends a path with '..', it must be killed immediately. > > Agree. Is that what we're doing now? Yes, it is. FilePicker always returns a normalized path. Any try to send a non-normalized path ends up with a KillHard().

Comment on attachment 8847488 [details] [diff] [review] patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1332003 [User impact if declined]: In theory, the content process could send a IPC message in order to have read-only access to any existing file. [Is this code covered by automated tests?]: yes. [Has the fix been verified in Nightly?]: not yet. [Needs manual test from QE? If yes, steps to reproduce]: none [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: This patch introduces a new component (FileSystemSecurity) that keeps track of which path has been shared with the content process via FilePicker and/or DataTransfer (Drag&Drop). Any following IPC call, related to Entries API, must match one of these shared paths. The only risk I can imagine is that I have forgotten an entry point for the Entries API, but the patch is well tested and fully green on try. So, no risks. [String changes made/needed]: none

(In reply to Andrea Marchesini [:baku] from comment #38) > > > The normalization happens in the content process. If the content process > > > sends a path with '..', it must be killed immediately. > > > > Agree. Is that what we're doing now? > > Yes, it is. FilePicker always returns a normalized path. Any try to send a > non-normalized path ends up with a KillHard(). Discussed this further with :baku and I've filed Bug 1349276 to be used to address the ".." issue. Essentially, we need sanitize the paths coming from content before they're passed to FileSystemUtils::IsDescendantPath().

Comment on attachment 8847488 [details] [diff] [review] patch Fix a sec-high. Aurora54+ & Beta53+.

has problems to apply to aurora (and seems has review - too) warning: conflicts while merging dom/filesystem/FileSystemRequestParent.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/filesystem/GetDirectoryListingTask.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/filesystem/GetDirectoryListingTask.h! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/filesystem/GetFileOrDirectoryTask.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/filesystem/GetFileOrDirectoryTask.h! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/filesystem/GetFilesTask.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/filesystem/GetFilesTask.h! (edit, then use 'hg resolve --mark') warning: conflicts while merging ipc/glue/BackgroundParentImpl.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging ipc/glue/BackgroundParentImpl.h! (edit, then use 'hg resolve --mark')

We need to uplift bug 1299500.

ok waiting then on the uplift approval from the depended bug

depended bug landed on aurora so i could land this also https://hg.mozilla.org/releases/mozilla-aurora/rev/076416de288687037559442ec1dbe2987752da66 further uplift to beta is blocked because bug 1299500 need also be uplifted to beta and so need approval for beta too

I approved the uplifts to beta from 1299500 so once that lands this should be OK for beta uplift.

Setting qe-verify- based on Andrea's assessment on manual testing needs (see Comment 39) and the fact that this fix has automated coverage.

Comment on attachment 8847488 [details] [diff] [review] patch restrict which files content processes can access, esr52+ Looks like the commit message says "patches" to mean "paths" :)