1675540 - Crash in [@ mozilla::net::nsStandardURL::Release]

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

Crash report: https://crash-stats.mozilla.org/report/index/afc51442-e422-4d43-bf7a-a65890201105

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(aSeg.mLen < 0 || (aSeg.mPos + aSeg.mLen <= aSpec.Length() && aSeg.mPos + aSeg.mLen >= aSeg.mPos))

Top 10 frames of crashing thread:

0 xul.dll mozilla::net::nsStandardURL::Release netwerk/base/nsStandardURL.cpp:1261
1 xul.dll mozilla::net::NullHttpChannel::Release netwerk/protocol/http/NullHttpChannel.cpp:14
2 xul.dll static mozilla::dom::DeferredFinalizerImpl<nsISupports>::DeferredFinalize dom/bindings/BindingUtils.h:2727
3 xul.dll mozilla::IncrementalFinalizeRunnable::ReleaseNow xpcom/base/CycleCollectedJSRuntime.cpp:1546
4 xul.dll mozilla::IncrementalFinalizeRunnable::Run xpcom/base/CycleCollectedJSRuntime.cpp:1582
5 xul.dll IdleRunnableWrapper::Run xpcom/threads/nsThreadUtils.cpp:348
6 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:514
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:327

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Combination of weird pointer crashes and SanityCheck() failures; this points at some sort of UAF.

Comment 2

[Jens Stutte [:jstutte]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #1)

Combination of weird pointer crashes and SanityCheck() failures; this points at some sort of UAF.

Not sure what is meant here with "weird pointer crashes". I see for example here a simple deallocation of nsStandardURL on the stack which triggers these sanity checks which cause one of these two MOZ_RELEASE_ASSERTs.

Valentin, are we sure those sanity checks are causing more good than harm? If we think they are really needed: Are we sure they are in the right place here in the deconstructor?

Comment 3

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

(In reply to Jens Stutte [:jstutte] from comment #2)

Agreed, it's just the sanity check failing.

Valentin, are we sure those sanity checks are causing more good than harm? If we think they are really needed: Are we sure they are in the right place here in the deconstructor?

We have another check here but that can't catch every way the indexes can go wrong.
Ideally we would put a sanity check after every change, but that may needlessly impact performance.

Comment 4

[Jens Stutte [:jstutte]]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #3)

Ideally we would put a sanity check after every change, but that may needlessly impact performance.

I fear that is the only way if you want to make them work. We might want to put them behind the nightly and early beta flag?

Comment 5

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

About half of the reports I've looked at also have LoadInfo::Release() on the stack, so I suspect there's a correlation with bug 1544127.
As Jens noticed with bug 1544127 comment 51, most of the crashes here also seem to be related with CycleCollection.
I wasn't able to find any better reasons. It could be that this is a UAF, and we assert because that memory has been overridden with bytes that trigger the assertion, or could point to an actual bug - it's difficult to say. I'm currently looking at the code for some other leads.

Comment 6

[Jens Stutte [:jstutte]]

(wrong bug, see bug 1544127)

Comment 7

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

Attachment: Bug 1675540 - Perform URL sanity checks when changing it r=#necko

Comment 8

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

Comment on attachment 9202059 [details]
Bug 1675540 - Perform URL sanity checks when changing it r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This is only a diagnostic patch. Bug isn't immediately evident from the code, but it does indicate we believe to be a bug somewhere.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Easy to uplift.
• How likely is this patch to cause regressions; how much testing does it need?: Low chance for regressions.

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9202059 [details]
Bug 1675540 - Perform URL sanity checks when changing it r=#necko

Approved to land. I do not think we want to uplift this, I think we should land it in Nightly and if needed, let it ride into Beta.

Comment 10

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

https://hg.mozilla.org/integration/autoland/rev/ae3e88b6b1beb53bacb95716f6a033a32c3c8423
https://hg.mozilla.org/mozilla-central/rev/ae3e88b6b1be

Comment 11

[Jens Stutte [:jstutte]]

I assume, being just a diagnostic patch means we should leave this open for now?

Comment 12

[Valentin Gosu [:valentin] {{ PTO 3 July - 3 August }}]

The new crash reports will likely have a different signature, so it's probably OK to close this.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 14

[Julien Cristau [:jcristau]]

Do we need a new bug for the remaining SanityCheck failures on 87+? E.g. bp-d8ec6330-74b5-4680-8cb7-7ebd80210325.

Comment 15

[Julien Cristau [:jcristau]]

The remaining crashes are tracked in 1700895.