Closed Bug 1354308 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect, critical)

53 Branch
All
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: philipp, Assigned: baku)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 3 obsolete files)

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
Fairly low frequency crash and we're a week away from release, so I am going to wontfix this for 53.
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?
Component: General → Security: Process Sandboxing
Flags: needinfo?(amarchesini)
Attached patch killWithReason.patch (obsolete) — Splinter Review
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.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8860475 - Flags: review?(nfroyd)
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.
Attachment #8860475 - Flags: review?(nfroyd) → review?(benjamin)
It'd be nice to get this fixed for 54.
> Isn't a path like "/allowed/directory/a/../b" perfectly legal?

It's not. '..' cannot be contained in the path.
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.
(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?
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 on attachment 8860475 [details] [diff] [review]
killWithReason.patch

Given my current understanding of KillHard behavior, r-
Attachment #8860475 - Flags: review?(benjamin) → review-
Tracking 54/55+ for this crash regression.
Attached patch killWithReason.patch (obsolete) — Splinter Review
Attachment #8860475 - Attachment is obsolete: true
Attachment #8861447 - Flags: review?(nfroyd)
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.
Attachment #8861447 - Flags: review?(nfroyd) → review-
Attached patch killWithReason.patch (obsolete) — Splinter Review
Attachment #8861447 - Attachment is obsolete: true
Attachment #8861469 - Flags: review?(nfroyd)
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.
Attachment #8861469 - Flags: review?(nfroyd) → review-
> 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.
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
Attachment #8861469 - Attachment is obsolete: true
Attachment #8861768 - Flags: review?(nfroyd)
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...
Attachment #8861768 - Flags: review?(nfroyd) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06d30abb358
Entries API must support patches containing '..', r=froydnj
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
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd13c88ccf89
Entries API must support patches containing '..', r=froydnj
https://hg.mozilla.org/mozilla-central/rev/cd13c88ccf89
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Attachment #8861768 - Flags: approval-mozilla-esr52?
Attachment #8861768 - Flags: approval-mozilla-beta?
Attachment #8861768 - Flags: approval-mozilla-aurora?
Comment on attachment 8861768 [details] [diff] [review]
killWithReason.patch

Fix a crash and include tests. Beta54+. Should be in 54 beta 4.
Attachment #8861768 - Flags: approval-mozilla-beta?
Attachment #8861768 - Flags: approval-mozilla-beta+
Attachment #8861768 - Flags: approval-mozilla-aurora?
Attachment #8861768 - Flags: approval-mozilla-aurora-
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
Attachment #8861768 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.