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

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Christoph Diehl [:posidron]]

Attachment: log_ffp_asan_26181.log.32166.txt

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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: log_stderr.txt

Comment 2

[Honza Bambas (:mayhemer)]

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..

Comment 3

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Valentin: if mRef.mPos was 0, would this trigger it? I don't think anything prohibits that.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1517542 - Fail URL deserialization if query or ref start at position 0 r=mayhemer!

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 8

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Indeed, thanks!

Comment 10

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

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Valentin Gosu [:valentin] (he/him)]

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:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e80c71637a63

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b4aa9fdd1e58