1354308 - Crash in IPCError-browser | This path is not allowed.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-b13a14f8-e63a-4e4c-a724-265e12170406.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	ntdll.dll 	NtWaitForMultipleObjects 	
1 	kernelbase.dll 	WaitForMultipleObjectsEx 	
2 	user32.dll 	MsgWaitForMultipleObjectsEx 	
3 	xul.dll 	mozilla::widget::WinUtils::WaitForMessage(unsigned long) 	widget/windows/WinUtils.cpp:787
4 	xul.dll 	nsAppShell::ProcessNextNativeEvent(bool) 	widget/windows/nsAppShell.cpp:382
5 	xul.dll 	nsBaseAppShell::DoProcessNextNativeEvent(bool) 	widget/nsBaseAppShell.cpp:138
6 	xul.dll 	nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) 	widget/nsBaseAppShell.cpp:289
7 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1213
8 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:390
9 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:124
10 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
11 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
12 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
13 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
14 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:262
15 	xul.dll 	XRE_RunAppShell() 	toolkit/xre/nsEmbedFunctions.cpp:924
16 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:269
17 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
18 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
19 	xul.dll 	XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/nsEmbedFunctions.cpp:756
20 	xul.dll 	mozilla::BootstrapImpl::XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/Bootstrap.cpp:65
21 	firefox.exe 	content_process_main(mozilla::Bootstrap*, int, char** const) 	ipc/contentproc/plugin-container.cpp:115
22 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
23 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
24 	kernel32.dll 	BaseThreadInitThunk 	
25 	ntdll.dll 	__RtlUserThreadStart 	
26 	ntdll.dll 	_RtlUserThreadStart

crash reports with this signature started showing up with a low frequency since the start of the month - so far only on windows. the pattern suggests that this is due to a patch landing on nightly recently and which subsequently got uplifted to aurora and beta. this first showed up on 53.0b8, so this is the changelog to the beta build before:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_53_0b7_RELEASE&tochange=FIREFOX_53_0b8_RELEASE

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Fairly low frequency crash and we're a week away from release, so I am going to wontfix this for 53.

Comment 2

[Nathan Froyd [:froydnj]]

Gonna tenatively say, based on the crash message, that this is coming from uplifting bug 1349276, which appears in the regression range from comment 0.  The crash message is from CheckPermissionRunnable::Run, and that function calls FileSystemSecurity::ContentProcessHasAccessTo, which the aforementioned bug modifies.

Andrea, can you look into this?  Can we add any diagnostics to make the crash easier to debug?

Comment 3

[Andrea Marchesini [:baku]]

Attachment: killWithReason.patch

In bug 1349276, I introduced a security system so that only known paths can be opened by Entries API. Here we have a crash. It can happen that I have missed something, or there is a bug, or there is actually something bad happening.

I don't know if we can land this because we are actually printing the paths.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8860475 [details] [diff] [review]
killWithReason.patch

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

Isn't a path like "/allowed/directory/a/../b" perfectly legal?

The code looks fine, but I am not sure about the privacy implications of sticking the rejected path in the reason string, since I think those reasons are publically visible.  Asking for clarification from Benjamin on that front.

::: dom/filesystem/FileSystemSecurity.cpp
@@ +113,5 @@
> +{
> +  nsAutoCString reason;
> +  reason.Assign("Path denied: ");
> +  reason.Append(NS_ConvertUTF16toUTF8(aPath));
> +  reason.Append(" of: ");

" of: "?  I'm not sure what how this is connecting the denied path with mPaths.

Comment 5

[Nathan Froyd [:froydnj]]

It'd be nice to get this fixed for 54.

Comment 6

[Andrea Marchesini [:baku]]

> Isn't a path like "/allowed/directory/a/../b" perfectly legal?

It's not. '..' cannot be contained in the path.

Comment 7

[Andrea Marchesini [:baku]]

Actually, the problem can be that we do:

  if (FindInReadable(NS_LITERAL_STRING(".."), aPath)) {
    return false;
  }


so that if a file is called "foo..bar" we kill the child.
I'll submit a patch to fix this issue.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Andrea Marchesini [:baku] from comment #6)
> > Isn't a path like "/allowed/directory/a/../b" perfectly legal?
> 
> It's not. '..' cannot be contained in the path.

This is what the spec says, but is it what everybody who implements the spec does?

Comment 9

[Benjamin Smedberg]

Please do not put a rejected path in the existing crash reason string or any other place that is publicly visible.
If you'd like to add a new annotation which is treated as private data only visible to the same subset of socorro users who have minidump access, that is fine.

Comment 10

[Benjamin Smedberg]

Comment on attachment 8860475 [details] [diff] [review]
killWithReason.patch

Given my current understanding of KillHard behavior, r-

Comment 11

[Marcia Knous [:marcia]]

Tracking 54/55+ for this crash regression.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: killWithReason.patch

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8861447 [details] [diff] [review]
killWithReason.patch

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

::: dom/filesystem/FileSystemSecurity.cpp
@@ +94,4 @@
>      return false;
>    }
> +#elif defined(XP_UNIX)
> +  if (FindInReadable(NS_LITERAL_STRING("../"), aPath)) {

Surely /subdir../file is a reasonable (if strange) path, so this check can't be correct.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: killWithReason.patch

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8861469 [details] [diff] [review]
killWithReason.patch

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

Looks OK, but needs some tests to verify the correct behavior, including a /../-containing path, if there's not such a test already.

Comment 16

[Andrea Marchesini [:baku]]

> Looks OK, but needs some tests to verify the correct behavior, including a
> /../-containing path, if there's not such a test already.

I cannot simulate that with a test. I can extend the test to support "subdir../file", but I cannot simulate a request that kills the child process.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Andrea Marchesini [:baku] from comment #16)
> > Looks OK, but needs some tests to verify the correct behavior, including a
> > /../-containing path, if there's not such a test already.
> 
> I cannot simulate that with a test. I can extend the test to support
> "subdir../file", but I cannot simulate a request that kills the child
> process.

Ah, good point.  OK then, at least test subdir.. and ..subdir if possible.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: killWithReason.patch

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8861768 [details] [diff] [review]
killWithReason.patch

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

Thank you!  Let's see if this solves the problem, or if people really are creating /../ entries...

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06d30abb358
Entries API must support patches containing '..', r=froydnj

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing test_basic.html, test_formSubmission.html and test_no_dnd.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea39575ae0cf1ae5a0958c1bc3de9ac9e3a37e1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9323cfe9cd8e11683bc738b8f5ee58ea4d0fe311&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=94531183&repo=mozilla-inbound

08:21:37     INFO - TEST-START | dom/filesystem/compat/tests/test_basic.html
08:21:38     INFO - GECKO(1132) | Unable to read VR Path Registry from C:\Users\cltbld\AppData\Local\openvr\openvrpaths.vrpath
08:21:38     INFO - GECKO(1132) | JavaScript error: http://mochi.test:8888/tests/dom/filesystem/compat/tests/script_entries.js, line 44: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIFile.create]
08:26:37     INFO - TEST-INFO | started process screenshot
08:26:37     INFO - TEST-INFO | screenshot: exit 0
08:26:37     INFO - TEST-UNEXPECTED-FAIL | dom/filesystem/compat/tests/test_basic.html | Test timed out. 
08:26:37     INFO -     reportError@SimpleTest/TestRunner.js:121:7
08:26:37     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5
08:26:37     INFO -     TestRunner.runTests@SimpleTest/TestRunner.js:380:5
08:26:37     INFO -     RunSet.runtests@SimpleTest/setup.js:194:3
08:26:37     INFO -     RunSet.runall@SimpleTest/setup.js:173:5
08:26:37     INFO -     hookupTests@SimpleTest/setup.js:266:5
08:26:37     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
08:26:37     INFO -     hookup@SimpleTest/setup.js:246:5
08:26:37     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cusers%5Ccltbld%5Cappdata%5Clocal%5Ctemp&cleanupCrashes=true:11:1

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd13c88ccf89
Entries API must support patches containing '..', r=froydnj

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd13c88ccf89

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8861768 [details] [diff] [review]
killWithReason.patch

[Approval Request Comment]
[Feature/Bug causing the regression]:  Bug 1344415
[User impact if declined]: If the filesystem contains a file with '..', when it is shared with the Entries API, the content process is killed.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: not needed, but would be nice to have.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: There is just a better check of '..' in the filename.
[String changes made/needed]: none

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8861768 [details] [diff] [review]
killWithReason.patch

Fix a crash and include tests. Beta54+. Should be in 54 beta 4.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e9e0a7ff2aad

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8861768 [details] [diff] [review]
killWithReason.patch

I see ~20 instances of this on ESR52.2, the fix has worked on Beta54 branch, let's uplift to ESR52.2

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c8ba3f911eb1