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

Status: RESOLVED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #1675540 +++

bp-d8ec6330-74b5-4680-8cb7-7ebd80210325

Comment 1

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

My Necko colleagues suggested that we might convert the release asserts to telemetry events so that we might find out how the offsets are wrong, given that the dumps do not show the memory containing the URL or offsets.
I did so and after that I looked into writing a test - which led me to the deserialization code. Looking at the crash reports, most of them are when releasing principals. That means a principal deserialization is what generated the wrong URL, and it explains why the sanity checks only fail being called from nsStandardURL::Release.

I'll implement a more thorough deserialization check, and land that.

Comment 2

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

Attachment: Bug 1700895 - Check if deserialized URL is valid r=#necko

We return an error code if the serialization isn't valid, and also clear the
URL segments so we don't trigger the sanity checks when the URL is released.

Comment 3

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

Comment on attachment 9224689 [details]
Bug 1700895 - Check if deserialized URL is valid r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: An exploit also requires the ability to change serialized data or IPC messages.
  The good news is that this is not a UAF in the URL code.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• 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
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions.

Comment 4

[Tom Ritter [:tjr]]

Comment on attachment 9224689 [details]
Bug 1700895 - Check if deserialized URL is valid r=#necko

Approved to land and uplift.

Comment 5

[Julien Cristau [:jcristau]]

Check if deserialized URL is valid r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/411686723027a23bfac7d3612956131a6dc08bef

Comment 6

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

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

Comment 7

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/71a613ae7b42

Comment 8

[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 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/36bca8ad92bd