1565848 - Crash in [@ mozilla::net::HttpChannelChild::OnStartRequest], if installed path name includes Unicode Character 'STRING TERMINATOR' (U+009C)

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

This bug is for crash report bp-6295a50d-656d-404f-ba09-76cc90190713.

Top 10 frames of crashing thread:

0 xul.dll mozilla::net::HttpChannelChild::OnStartRequest netwerk/protocol/http/HttpChannelChild.cpp:564
1 xul.dll mozilla::net::StartRequestEvent::Run netwerk/protocol/http/HttpChannelChild.cpp:427
2 xul.dll mozilla::net::ChannelEventQueue::RunOrEnqueue netwerk/ipc/ChannelEventQueue.h:210
3 xul.dll class mozilla::ipc::IPCResult mozilla::net::HttpChannelChild::RecvOnStartRequest netwerk/protocol/http/HttpChannelChild.cpp:489
4 xul.dll mozilla::net::PHttpChannelChild::OnMessageReceived ipc/ipdl/PHttpChannelChild.cpp:859
5 xul.dll mozilla::dom::PContentChild::OnMessageReceived ipc/ipdl/PContentChild.cpp:7197
6 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2082
7 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:295
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1225
9 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486

Reproducible: always

Steps To reproduce:

1. Install Nightly to folder. The folder path includes includes Unicode Character 'STRING TERMINATOR' (U+009C)
2. Launch Nightly
3. Try to load any web page

Actual Results:
Tab crashes

Expected Results:
No crash

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bd5ecd72f76c9a0d1cbfb423553db211ad39b437&tochange=c627801eeecc8bf6100c723b435138b2f47179e8

Regressed by: Bug 1552160

Comment 2

[Alice0775 White]

Nightly70.0a1(20190713175429) crashes.
Firefox Developer Edition 69.0b4 crashes.
Firefox Beta 69.0b4 does not crash.

Comment 3

[Alice0775 White]

Attachment: about:support

Application Binary D:\fuku\MyDownload\aaaaaaa\firefox.exe
↑
U+009C

Comment 4

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

This is a fairly low volume crash but since it's reproducible maybe we have a chance to fix this before shipping 69.

Comment 5

[Bob Owen (:bobowen)]

I've reproduced this, so I'm looking into it now.
Although how the sandbox code update could have broken things here is beyond me at the moment.

Comment 6

[Bob Owen (:bobowen)]

OK, I've worked out what is going on here.

The code just before the assertion at [1] causes nss to be initialised (as part of the component manager initialisation I think) via [2].
As part of that initialisation (many layers down) it calls [3] as part of loading softokn3.dll.
PR_GetLibraryFilePathname is used to try and get the path of nss3.dll, so softokn3.dll can be loaded from the same dir using the call at [4].
However, it uses CP_ACP (the system default Windows ANSI code page) to convert to and from wchar, which can cause character loss.
So, the path returned is incorrect in this case. (In fact even if PR_GetLibraryFilePathname worked loader_LoadLibInReferenceDir eventually calls pr_LoadLibraryByPathname that also uses CP_ACP, so it would fail there anyway.)

PORT_LoadLibraryFromOrigin then falls back to attempting the load with just the bare file name (softokn3.dll) at [5].
Before the latest update to the chromium sandbox this used to work because the application dir is part of the default library loading locations.
This is now no longer in the list when the sandbox is lowered and so it is now failing as well.

I think the best course of action is to fix the attempt to explicitly load from the same location as nss3.dll (by using CP_UTF8 not CP_ACP), because this is obviously the intention and what happens when the path doesn't contain characters that are currently lost in conversion.

[1] https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/netwerk/protocol/http/HttpChannelChild.cpp#562
[2] https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/security/nss/lib/nss/nssinit.c#948
[3] https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/security/nss/lib/util/secload.c#147
[4] https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/security/nss/lib/util/secload.c#151
[5] https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/security/nss/lib/util/secload.c#174

Comment 7

[Bob Owen (:bobowen)]

Attachment: Bug 1565848: Change NSPR to use CP_UTF8 when handling Windows library paths. r=glandium!

Comment 8

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&duplicate_jobs=visible&selectedJob=257941902&revision=39f954858f717007a965c26c752ca1160b8ec20c

Comment 9

[Bob Owen (:bobowen)]

Attachment: Bug 1565848: Revert latest change to MITIGATION_DLL_SEARCH_ORDER. r=handyman!

This is until any regressions can be fixed, see bug 1568850.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ba557fab26e
Revert latest change to MITIGATION_DLL_SEARCH_ORDER. r=aklotz

Comment 11

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7ba557fab26e

Comment 12

[Bob Owen (:bobowen)]

Hi Alice, would you confirm that the latest Nightly fixes this for you, please.

Comment 13

[Alice0775 White]

Build ID 20190725215157
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

I verified that the latest Nightly70.a1 is no longer crashed. Thanks

Comment 14

[Bob Owen (:bobowen)]

Comment on attachment 9080636 [details]
Bug 1565848: Revert latest change to MITIGATION_DLL_SEARCH_ORDER. r=handyman!

Beta/Release Uplift Approval Request

• User impact if declined: For people with non-ANSI characters in the Firefox path, DLLs may fail to load after the sandbox is lowered, causing functional issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This reverts the behaviour of the sandbox mitigation MITIGATION_DLL_SEARCH_ORDER to it's previous behaviour.
• String changes made/needed: None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9080636 [details]
Bug 1565848: Revert latest change to MITIGATION_DLL_SEARCH_ORDER. r=handyman!

Reverts us back to previous known-good behavior to avoid a crash. Approved for 69.0b9.

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 17

[Anca Soncutean, Desktop QA]

I've reproduced this crash with Fx 70.0a1 (20190713175429) on Windows 10. The issue is not reproducible with Fx 69.0b9 (20190730004747).