Closed Bug 1433609 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 60+ fixed
firefox59 --- wontfix
firefox60 + fixed
firefox61 + fixed

People

(Reporter: posidron, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-bounds, sec-high, Whiteboard: [necko-triaged][adv-main60+][adv-esr52.8+])

Attachments

(4 files, 1 obsolete file)

Attached file 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
Attached file session.txt
Group: core-security → network-core-security
Valentin, this seems to be rather critical, can you look for a quick/simple fix here?
Assignee: nobody → valentin.gosu
Priority: -- → P1
Summary: IPC: global-buffer-overflow crash [@SegmentIs] → IPC: global-buffer-overflow crash [@nsStandardURL::SegmentIs]
Whiteboard: [necko-triaged]
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.
Attachment #8949961 - Flags: review?(honzab.moz)
(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 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)
Attachment #8949961 - Flags: review?(honzab.moz) → review+
(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.
MozReview-Commit-ID: BMQfPzPhDhc
Attachment #8949961 - Attachment is obsolete: true
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?
Flags: needinfo?(dveditz)
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.
(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:pass@example.com/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)
Hmmm, is verifying that `component.mPos + component.mLen < mSpec.Length()` not sufficient? (Plus checks for integer overflow!)
(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.
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?
(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.
(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.
Flags: needinfo?(dveditz)
See Also: → 1445472
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.
Attachment #8950908 - Flags: sec-approval?
sec-approval+ for trunk. We'll want patched nominated for affected branches as well, to land after this clears trunk.
Attachment #8950908 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/d0da2be2c950
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please nominate this for Beta & ESR52 approval when you're comfortable doing so.
Flags: needinfo?(valentin.gosu)
MozReview-Commit-ID: BMQfPzPhDhc
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.
Flags: needinfo?(valentin.gosu)
Attachment #8950908 - Flags: approval-mozilla-esr52?
Attachment #8950908 - Flags: approval-mozilla-beta?
Comment on attachment 8950908 [details] [diff] [review]
Ensure that deserialized URL is correct

IPC security fix, approved for 60.0b7
Attachment #8950908 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: network-core-security → core-security-release
(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-.
Flags: qe-verify-
Attachment #8950908 - Flags: approval-mozilla-esr52?
Comment on attachment 8961413 [details] [diff] [review]
[esr52] Ensure that deserialized URL is correct

Fix a sec-high. Approved for ESR 52.8.
Attachment #8961413 - Flags: approval-mozilla-esr52+
Whiteboard: [necko-triaged] → [necko-triaged][adv-main60+][adv-esr52.8+]
Group: core-security-release
Depends on: 1517542
You need to log in before you can comment on or make changes to this bug.