1746543 - Use-after-free crash in [@ mozilla::net::ProxyAutoConfig::ResolveAddress]

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/3b05bf1a-4841-434f-ad36-b86ba0211216

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::net::ProxyAutoConfig::ResolveAddress netwerk/base/ProxyAutoConfig.cpp:465
1 xul.dll mozilla::net::PACResolveToString netwerk/base/ProxyAutoConfig.cpp:521
2 xul.dll mozilla::net::PACDnsResolve netwerk/base/ProxyAutoConfig.cpp:559
3 None @0x00000106b6fe2189 
4 nss3.dll PR_GetCurrentThread nsprpub/pr/src/threads/prcthr.c:151
5 None @0x00007ffeffffffff 
6 xul.dll js::jit::MaybeEnterJit js/src/jit/Jit.cpp:210
7 xul.dll js::Call js/src/vm/Interpreter.cpp:552
8 xul.dll JS_CallFunctionName js/src/vm/CallAndConstruct.cpp:101
9 xul.dll mozilla::net::ProxyAutoConfig::GetProxyForURI netwerk/base/ProxyAutoConfig.cpp:908

Another proxy-related regression with a poison value in a register. This time it is on Windows, in the rcx register. The stack doesn't look the same to me, but maybe it is a similar issue?

Comment 1

[Andrew McCreight [:mccr8]]

Nightly only.

Comment 2

[Andrew McCreight [:mccr8]]

Bug 1495491 is a pre-existing bug for the same signature, but I don't see any crashes on recent versions with that signature, until it started showing up on Nightly, so I think that's a different issue.

Comment 3

[Kershaw Chang [:kershaw]]

I think this should be the same as bug 1746537.
I'll submit a patch and see if that patch can fix both.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Bug 1746543, r=#necko

Comment 5

[Kershaw Chang [:kershaw]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1746537#c3 for the reason of this crash.

Comment 6

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

Set release status flags based on info from the regressing bug 1745385

Comment 7

[Kershaw Chang [:kershaw]]

Comment on attachment 9255893 [details]
Bug 1746543, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This happens during shutdown, so I think it's probably not easy.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: None
• If not all supported branches, which bug introduced the flaw?: Bug 1745385
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This crash only happens on nightly.
• How likely is this patch to cause regressions; how much testing does it need?: Low. This patch doesn't change the behavior.

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I read Bug 1746537 - but I didn't anything to explain why you removed all the null-pointer checks?

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #8)

I read Bug 1746537 - but I didn't anything to explain why you removed all the null-pointer checks?

Since ProxyAutoConfigChild::mPAC is an unique ptr and if this line is removed, ProxyAutoConfigChild::mPAC should never be null. That's why I also remove all the null-pointer checks.

Comment 10

[Kershaw Chang [:kershaw]]

Could you take a look again? I'd like to land this patch soon.
Thanks.

Comment 11

[Andrew McCreight [:mccr8]]

This is Nightly-only so it doesn't actually need sec-approval. I'm not sure why Tom didn't just clear the sec-approval flag.

Comment 12

[Kershaw Chang [:kershaw]]

(In reply to Andrew McCreight [:mccr8] from comment #11)

This is Nightly-only so it doesn't actually need sec-approval. I'm not sure why Tom didn't just clear the sec-approval flag.

Thanks for letting me know. Does this mean I can just land the patch?

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Kershaw Chang [:kershaw] from comment #12)

Thanks for letting me know. Does this mean I can just land the patch?

Yep.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 9255893 [details]
Bug 1746543, r=#necko

sec-approval not needed for Nightly-only issues

Comment 15

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

r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/b35705f921c85e597242f02fc117236d645637ad
https://hg.mozilla.org/mozilla-central/rev/b35705f921c8

Comment 16

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

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Andrew McCreight [:mccr8] from comment #11)

This is Nightly-only so it doesn't actually need sec-approval. I'm not sure why Tom didn't just clear the sec-approval flag.

Thanks; I had not noticed!

Comment 18

[Kershaw Chang [:kershaw]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #16)

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.

done.