Closed Bug 1517542 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Networking, defect, P1)

Product:
Core
Component:
Networking
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
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)

log_ffp_asan_26181.log.32166.txt
(deleted), text/plain
Details
log_stderr.txt
(deleted), text/plain
Details
Bug 1517542 - Fail URL deserialization if query or ref start at position 0 r=mayhemer!
(deleted), text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr60+
Details
Attached file log_ffp_asan_26181.log.32166.txt (deleted) —
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 (deleted) —
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?

status-firefox64: --- → wontfix
status-firefox65: --- → affected
status-firefox-esr60: --- → ?
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)

https://hg.mozilla.org/mozilla-central/rev/16e495b2c1b5

Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66: affected → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: