Closed Bug 1565848 Opened 6 months ago Closed 6 months ago

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

Categories

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

70 Branch
Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- verified
firefox70 --- verified

People

(Reporter: alice0775, Assigned: bobowen)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords)

Crash Data

Attachments

(2 files, 1 obsolete file)

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

Component: Networking: HTTP → Security: Process Sandboxing
Keywords: regression
Regressed by: 1552160

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

Attached file about:support

Application Binary D:\fuku\MyDownload\aaaœaaaa\firefox.exe

U+009C

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

Flags: needinfo?(bobowencode)

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.

Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(bobowencode)
Priority: -- → P1

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

Blocks: 1568850

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

Attachment #9080296 - Attachment is obsolete: true
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7ba557fab26e
Revert latest change to MITIGATION_DLL_SEARCH_ORDER. r=aklotz
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

Flags: needinfo?(alice0775)

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

Status: RESOLVED → VERIFIED
Flags: needinfo?(alice0775)

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
Attachment #9080636 - Flags: approval-mozilla-beta?

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.

Attachment #9080636 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.