Closed Bug 1517542 Opened 2 years ago Closed 2 years ago

IPC: wild-ptr crash PNecko::Msg_PredPredict [@mozilla::net::NeckoParent::RecvPredPredict]

Categories

(Core :: Networking, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 65+ fixed
firefox64 --- wontfix
firefox65 + fixed
firefox66 + fixed

People

(Reporter: posidron, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-bounds, sec-moderate, Whiteboard: [necko-triaged][post-cristsmash-triage][adv-main65+][adv-esr60.5+])

Attachments

(3 files)

We had an issue with our harness and the mutated / original message dump was not saved during this find hence there ain't not much information to provide.

Revision: 20181227-894e36ecd784

Log Excerpt:

[Faulty] (     32336) Process: 2 | Size:        256 | message.32336.82     | Channel::ChannelImpl::Send => PNecko::Msg_PredPredict
[Faulty] (     32336) FUZZING (6 bytes): PNecko::Msg_PredPredict
Group: network-core-security, core-security
Attached file log_stderr.txt
See Also: → 1392739
Group: core-security
Valentin, according the first attachment this seems like a malformed URL begin received.  are we still missing some boundary check here?  unfortunately, I don't see the URL logged anywhere..
Assignee: nobody → valentin.gosu
Priority: -- → P1
Whiteboard: [necko-triaged]
(In reply to Honza Bambas (:mayhemer) from comment #2)
> Valentin, according the first attachment this seems like a malformed URL
> begin received.  are we still missing some boundary check here? 

Most likely. I think the safest way forward would be just reparsing the URL from the spec, but that would be quite bad performance-wise.

> unfortunately, I don't see the URL logged anywhere..
(In reply to Christoph Diehl [:posidron] from comment #0)
> We had an issue with our harness and the mutated / original message dump was
> not saved during this find hence there ain't not much information to provide.

Hopefully we will hit this again, and find out where the issue is.
Valentin: if mRef.mPos was 0, would this trigger it? I don't think anything prohibits that.
Flags: needinfo?(valentin.gosu)
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #4)
> Valentin: if mRef.mPos was 0, would this trigger it? I don't think anything
> prohibits that.

I think you're right, that would definitely trigger it. I'll post a patch ASAP.
Flags: needinfo?(valentin.gosu)

https://hg.mozilla.org/integration/autoland/rev/16e495b2c1b5

If I'm following the blame correctly, this is from bug 1433609 and ESR60 is therefore not affected?

Flags: needinfo?(valentin.gosu)
Keywords: checkin-needed

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

https://hg.mozilla.org/integration/autoland/rev/16e495b2c1b5

If I'm following the blame correctly, this is from bug 1433609 and ESR60 is therefore not affected?

I'd say it's more of a follow-up to bug 1433609, which I think is in esr60 too. :)
I think it should apply cleanly, but let me know if doesn't.

Flags: needinfo?(valentin.gosu)
Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Please nominate this for Beta/ESR60 approval when you get a chance.

Flags: needinfo?(valentin.gosu)

Comment on attachment 9034792 [details]
Bug 1517542 - Fail URL deserialization if query or ref start at position 0 r=mayhemer!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Possible out-of-bounds memory access if the content process sends corrupted messages.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

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): Just adds a check to make sure we never dereference out of bounds indexes in the string.

String changes made/needed:

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration:

User impact if declined:

Fix Landed on Version: 66

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just adds a check to make sure we never dereference out of bounds indexes in the string.

String or UUID changes made by this patch:

Flags: needinfo?(valentin.gosu)
Attachment #9034792 - Flags: approval-mozilla-esr60?
Attachment #9034792 - Flags: approval-mozilla-beta?

Comment on attachment 9034792 [details]
Bug 1517542 - Fail URL deserialization if query or ref start at position 0 r=mayhemer!

[Triage Comment]
Follow-up fix from previous work which fixes a possible out-of-bounds memory access if the content process sends corrupted messages. Approved for 65.0b10 and 60.5.0esr.

Attachment #9034792 - Flags: approval-mozilla-esr60?
Attachment #9034792 - Flags: approval-mozilla-esr60+
Attachment #9034792 - Flags: approval-mozilla-beta?
Attachment #9034792 - Flags: approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [necko-triaged] → [necko-triaged][post-cristsmash-triage]
Whiteboard: [necko-triaged][post-cristsmash-triage] → [necko-triaged][post-cristsmash-triage][adv-main65+][adv-esr60.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.