1433609 - IPC: global-buffer-overflow crash [@nsStandardURL::SegmentIs]

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack.txt

INFO: This is an IPC crash found by the fuzzer faulty - there is no test-case available which leads to an immediate crash for reproduction.

The attached session.txt contains a trace of IPC messages which were sent and received during a session of visiting  https://html5test.com

*** Possible reproduction scenario:

pip install git+https://github.com/mozillasecurity/fuzzfetch
fuzzfetch -a --fuzzing -n firefox -o /tmp

export FAULTY_PROBABILITY=50000
export FAULTY_LARGE_VALUES=1
export FAULTY_PARENT=1
export FAULTY_ENABLE_LOGGING=1
export FAULTY_PICKLE=1
export MOZ_IPC_MESSAGE_LOG=1


*** Messages which correlate with the stack:

[time: 1517011604468329][6751->6821] [PBrowserParent] Sending  PBrowser::Msg_LoadRemoteScript
[time: 1517011604470821][6751->6821] [PBrowserParent] Sending  PBrowser::Msg_AsyncMessage
[time: 1517011604470925][6751->6821] [PBrowserParent] Sending  PBrowser::Msg_RenderLayers
[time: 1517011604470954][6751->6821] [PBrowserParent] Sending  PBrowser::Msg_SetDocShellIsActive
[time: 1517011604476341][6821<-6751] [PBrowserChild] Received  PBrowser::Msg_LoadRemoteScript
[time: 1517011604476489][6751->6821] [PBrowserParent] Sending  PBrowser::Msg_UpdateDimensions
[time: 1517011604476804][6821<-6751] [PBrowserChild] Received  PBrowser::Msg_LoadRemoteScript
[time: 1517011604477044][6751->6860] [PBrowserParent] Sending  PBrowser::Msg_SynthMouseMoveEvent
[time: 1517011604477224][6860<-6751] [PBrowserChild] Received  PBrowser::Msg_SynthMouseMoveEvent
[time: 1517011604477922][6860->6751] [PContentChild] Sending  PContent::Msg_AccumulateChildHistograms
[time: 1517011604477966][6821<-6751] [PBrowserChild] Received  PBrowser::Msg_LoadRemoteScript
[Faulty] pickle field {UInt32} of value: 0 changed to: 1


*** Source

    bool
    nsStandardURL::SegmentIs(const URLSegment &seg1, const char *val, const URLSegment &seg2, bool ignoreCase)
    {
        if (seg1.mLen != seg2.mLen)
            return false;
        if (seg1.mLen == -1 || (!val && mSpec.IsEmpty()))
            return true; // both are empty
        if (!val)
            return false;
        if (ignoreCase)
            return !PL_strncasecmp(mSpec.get() + seg1.mPos, val + seg2.mPos, seg1.mLen);
        else
*           return !strncmp(mSpec.get() + seg1.mPos, val + seg2.mPos, seg1.mLen);
    }

https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/netwerk/base/nsStandardURL.cpp#1007

Comment 1

[Christoph Diehl [:posidron]]

Attachment: session.txt

Comment 2

[Honza Bambas (:mayhemer)]

Valentin, this seems to be rather critical, can you look for a quick/simple fix here?

Comment 3

[Valentin Gosu [:valentin]]

Attachment: Ensure that deserialized URL is correct

I added some sanity checks. We might want to consider adding a few more, for example ensuring that all the segments' indexes are in the proper order.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #3)
> Created attachment 8949961 [details] [diff] [review]
> Ensure that deserialized URL is correct
> 
> I added some sanity checks. We might want to consider adding a few more, for
> example ensuring that all the segments' indexes are in the proper order.

probably yes.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8949961 [details] [diff] [review]
Ensure that deserialized URL is correct

Review of attachment 8949961 [details] [diff] [review]:
-----------------------------------------------------------------

based on assumption (while reading the stack track - a hashtable compare during get keyed with a just IPC-received URI) that the broken instance is the deserialized one.  if it's based on some more concrete fact, please point me to it.

anyway, this is a good patch, I believe.

::: netwerk/base/nsStandardURL.cpp
@@ +3698,5 @@
> +
> +    // Some sanity checks
> +    NS_ENSURE_TRUE(mScheme.mPos == 0, false);
> +    NS_ENSURE_TRUE(mScheme.mLen > 0, false);
> +    NS_ENSURE_TRUE(static_cast<int32_t>(mSpec.Length()) >= mScheme.mLen + 3, false);

mScheme.mLen + 3 could overflow, and I don't understand this check (comment please)

Comment 6

[Valentin Gosu [:valentin]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 8949961 [details] [diff] [review]
> Ensure that deserialized URL is correct
> 
> Review of attachment 8949961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> based on assumption (while reading the stack track - a hashtable compare
> during get keyed with a just IPC-received URI) that the broken instance is
> the deserialized one.  if it's based on some more concrete fact, please
> point me to it.

Not sure I understand what you mean here. The reason for this crash is that the fuzzer randomly changes some of bytes in IPC messages.
Do you mean that we should add a checksum for all the URL segments?

> ::: netwerk/base/nsStandardURL.cpp
> @@ +3698,5 @@
> > +
> > +    // Some sanity checks
> > +    NS_ENSURE_TRUE(mScheme.mPos == 0, false);
> > +    NS_ENSURE_TRUE(mScheme.mLen > 0, false);
> > +    NS_ENSURE_TRUE(static_cast<int32_t>(mSpec.Length()) >= mScheme.mLen + 3, false);
> 
> mScheme.mLen + 3 could overflow, and I don't understand this check (comment
> please)

Good point. I'll fix it.
This checks that the URL is at least scheme + "://" (3 characters) I'll put in a comment.

Comment 7

[Valentin Gosu [:valentin]]

Attachment: Ensure that deserialized URL is correct

MozReview-Commit-ID: BMQfPzPhDhc

Comment 8

[Valentin Gosu [:valentin]]

Daniel, before landing/requesting sec-approval, I have a few questions about the threat model here.
I assume we are trying to defend against a compromised content process, that has the ability to send serialized nsStandardURLs to the parent. The serialization contains a string and indexes, for the already parsed URL segments. I have added some sanity checks, but that wouldn't stop a bad content process from sending "incorrect" indexes, that still pass the checks, but eventually lead to an out-of-bounds. The only solution would be to parse the URL again, but that would be inefficient. We could maybe add a LRU cache once nsIURI is immutable.
Thoughts?

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Answering for Daniel.

Yes, the threat model here is a compromised content process, trying to escape the sandbox. IPC endpoints should be resilient to maliciously crafted messages, in the same way the content process should be resilient to maliciously crafted HTML and JS :-)

I'm not familiar with how nsStandardURLs are serialized so I don't have a specific suggestion of how to handle this efficiently.

Comment 10

[Valentin Gosu [:valentin]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #9)
> Answering for Daniel.
> 
> Yes, the threat model here is a compromised content process, trying to
> escape the sandbox. IPC endpoints should be resilient to maliciously crafted
> messages, in the same way the content process should be resilient to
> maliciously crafted HTML and JS :-)
> 
> I'm not familiar with how nsStandardURLs are serialized so I don't have a
> specific suggestion of how to handle this efficiently.

nsStandardURLs are largely serialized as follows: { mSpec: "http://user:[e-mail]/path?query#ref", mScheme: { mPos = 0, mLen = 4}, mUsername = { mPos = 7, mLen = 4}, mPassword = { mPos = 12, mLen = 4 } ... etc}
The segments are used for getters and setters. GetScheme just returns a Substring(mspec, mScheme.mPos, mScheme.mLen)

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Hmmm, is verifying that `component.mPos + component.mLen < mSpec.Length()` not sufficient? (Plus checks for integer overflow!)

Comment 12

[Valentin Gosu [:valentin]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #11)
> Hmmm, is verifying that `component.mPos + component.mLen < mSpec.Length()`
> not sufficient? (Plus checks for integer overflow!)

It's enough for ensuring that we don't crash immediately, but it could lead to bad URLs. If mHost = { mPos = 0, mLen = 4 }  the segment passes the sanity checks, but that would lead to a host of "http" instead of "example.com". Moreover, changing the host would change the scheme instead, but even worse, it wouldn't update all the segments before the host (scheme, username, password, authority) possibly making their indexes exceed the bounds of the mSpec string.

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Things like host="http" presumably aren't a big deal -- you can always just send mSpec=http://http or similar to get that outcome, is that right?

The mutation case is messy, is that fully addressed by immutable nsIURI?

Comment 14

[Valentin Gosu [:valentin]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #13)
> Things like host="http" presumably aren't a big deal -- you can always just
> send mSpec=http://http or similar to get that outcome, is that right?

True.

> The mutation case is messy, is that fully addressed by immutable nsIURI?

Actually no :( The serialized URL is immutable, but there is a Mutator API that clones the URL and can call setters on the clone before finalizing it. So... there's still a possibility of out-of-bounds.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > Comment on attachment 8949961 [details] [diff] [review]
> > Ensure that deserialized URL is correct
> > 
> > Review of attachment 8949961 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > based on assumption (while reading the stack track - a hashtable compare
> > during get keyed with a just IPC-received URI) that the broken instance is
> > the deserialized one.  if it's based on some more concrete fact, please
> > point me to it.
> 
> Not sure I understand what you mean here. The reason for this crash is that
> the fuzzer randomly changes some of bytes in IPC messages.
> Do you mean that we should add a checksum for all the URL segments?

Following the latest comments I can see now it was not an assumption.  So, disregard my comment please.


If a content process is compromised so that the indexes can be mangled with then I believe the whole spec can be manipulated.  Sanity checking against out-of-bounds crashing (and maybe that indexes don't overlap) is IMO enough.

Comment 16

[Valentin Gosu [:valentin]]

Comment on attachment 8950908 [details] [diff] [review]
Ensure that deserialized URL is correct

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
An exploit would require that the attacker already controls the content process. 
Even with this patch, a carefully constructed IPC message could still cause us to access invalid memory. However, landing this patch allows us to fuzz URL deserialization more intensely.
As a followup, I plan to change the Deserialization code to parse the URL again. This would probably cause a performance hit, but it would eliminate this issue completely.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, but careful analysis of what the code does would reveal what the problem is.

Which older supported branches are affected by this flaw?
All.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Easy to backport. Most of this code hasn't changed.

How likely is this patch to cause regressions; how much testing does it need?
The risk is low. Running on try didn't reveal any issues.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We'll want patched nominated for affected branches as well, to land after this clears trunk.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0da2be2c9508f48b8e4804bd4a4ca3b37b56d4f

Comment 19

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

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta & ESR52 approval when you're comfortable doing so.

Comment 21

[Valentin Gosu [:valentin]]

Attachment: [esr52] Ensure that deserialized URL is correct

MozReview-Commit-ID: BMQfPzPhDhc

Comment 22

[Valentin Gosu [:valentin]]

Comment on attachment 8950908 [details] [diff] [review]
Ensure that deserialized URL is correct

Approval Request Comment
[Feature/Bug causing the regression]:
Has always existed, since initial e10s implementation.

[User impact if declined]:
Attack vector for a compromised content process to the parent process.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]:
No.
 
[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
Low risk.

[Why is the change risky/not risky?]:
This change only affects e10s deserialization. The patch adds some sanity checks verifying that the segments it receives have indexes that are within bounds. The regression risk is if a valid URL is now rejected by this change (which seems unlikely)

[String changes made/needed]:
None.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8950908 [details] [diff] [review]
Ensure that deserialized URL is correct

IPC security fix, approved for 60.0b7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Cornel Ionce [:noni]]

(In reply to Valentin Gosu [:valentin] from comment #22)
> [Has the fix been verified in Nightly?]:
> Yes. 
> [Needs manual test from QE? If yes, steps to reproduce]:
> No.

Per Valentin's assessment on manual testign needs, marking as qe-.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8961413 [details] [diff] [review]
[esr52] Ensure that deserialized URL is correct

Fix a sec-high. Approved for ESR 52.8.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/7127ccf8f88c