Closed Bug 1386787 Opened 7 years ago Closed 7 years ago

IPC: heap-buffer-overflow [@ JSStructuredCloneReader::read]

Categories

(Core :: JavaScript Engine, defect)

Unspecified
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 56+ fixed
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: posidron, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-bounds, sec-high, Whiteboard: [adv-main56+][adv-esr52.4+][post-critsmash-triage])

Attachments

(3 files)

Attached file callstack.txt
The following crash occurs consistently on mozilla-central. Last tested revision is 20170802-52285ea5e54c.

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
Attached file session.txt
Looks like an issue related to structured clone deserialization.
[Faulty] pickle field {size_t} of value: 56 changed to: 55
Component: IPC → JavaScript Engine
Component: JavaScript Engine → IPC
Summary: IPC: heap-buffer-overflow [@ peek] → IPC: heap-buffer-overflow [@ JSStructuredCloneReader::read]
Component: IPC → JavaScript Engine
// Perform the whole recursive reading procedure.
bool
JSStructuredCloneReader::read(MutableHandleValue vp)
{
[...]
        uint32_t tag, data;
        if (!in.getPair(&tag, &data))
            return false;
[...]
Component: JavaScript Engine → IPC
Component: IPC → JavaScript Engine
I think the problem is here:

bool
SCInput::get(uint64_t* p)
{
    if (point.done())
        return reportTruncated();
    *p = NativeEndian::swapFromLittleEndian(point.peek());

It looks like point.done() just checks if we have any bytes left, but peek() reads out more than one byte.
Keywords: sec-high
Is comment 4 right, Steve? I'm surprised that we wouldn't have noticed something like that before but maybe it only shows up with a malicious content process. (If this is a problem here, it looks like it is a problem in a number of other places that assume that point.done() implies you can safely point.peek().)
Flags: needinfo?(sphink)
I would be very surprised if it's not safe to peek() if there is still data in the buffer. We assert that there is enough room for type T for peak<T>(). Only malicious content process can send this kind of buffer. In this case maybe we should turn checks into MOZ_RELEASE_ASSERT.
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #6)
> Only malicious content process can send this kind of buffer.

Yes, what this fuzzer does is mutate IPC messages, to simulate a malicious content process. As seen in comment 2, it reduced the size field by 1.

> In this case maybe we should turn checks into MOZ_RELEASE_ASSERT.

That sounds reasonable to me. I'm not sure what the best way to crash is from the perspective of the fuzzer being able to tell what is going on.
Group: core-security → javascript-core-security
Hm. Yeah, I guess MOZ_RELEASE_ASSERT would be fine, though if perf allowed it would be kind of nice to do the right thing here (reportTruncated()). Like, say, replace point.done() with point.canPeek() or point.hasAtLeast(sizeof(uint64_t)) (neither of which exists yet.) If we're ok with a release assert, then it would be no slower.
Flags: needinfo?(sphink)
But generally, I hadn't realized structured clone was an attack surface. I see more unguarded reads.
I *think* something like this ought to be ok? I haven't tested it locally, but I pushed to try with

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5efe612ac3a47a7b65d6e269160abc699c69b59
Attachment #8895636 - Flags: review?(kchen)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8895636 [details] [diff] [review]
Throw for short structured clone reads

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

LGTM. I agree that if perf allows it would be nice to report truncated instead of crash the chrome process when content sends malicious content.
Attachment #8895636 - Flags: review?(kchen) → review+
Content really shouldn't be able to crash chrome, if anything malicious content should get the content process killed :-)
Comment on attachment 8895636 [details] [diff] [review]
Throw for short structured clone reads

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't think this patch particularly helps, other than to spark the idea that you can feed mangled data to structured clone.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I tried to be circumspect in the comment.

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?

I would guess this would backport pretty cleanly, though things may have shifted around a bit.

How likely is this patch to cause regressions; how much testing does it need?

It's pretty benign. If it doesn't break everything, I wouldn't expect it to break anything. And if you ignore the stuff my try push was based on, the try run looked good.
Attachment #8895636 - Flags: sec-approval?
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #12)
> Content really shouldn't be able to crash chrome, if anything malicious
> content should get the content process killed :-)

That would be ideal, but we currently default to killing both processes if content does something weird. In fact, we used to kill only the content process in that case. The main reason is that almost 100% of the time when this happens it is due to a bug in Firefox, not malicious code, and we get better crash information that way, that lets us fix these crashes.

If some skilled hacker manages to put together an exploit that achieves arbitrary code execution in the content process, and all they can do is crash Firefox, I think that's okay. There's probably lots of ways for content to crash the chrome process, maybe by requesting a download of a really large file.
Thanks, I didn't realize that was already the case.
This gets sec-approval to be checked in on 8/28, three weeks into the current cycle (and not before).
We'll want a beta and ESR52 patch made and nominated to go in after this lands on trunk.
Whiteboard: [checkin on 8/28]
Attachment #8895636 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd9aa3938195762e74c1595895b462a8b0a5fe87

Please request Beta & ESR52 approval on this when you get a chance. I've confirmed that it grafts cleanly to both branches.
Flags: needinfo?(sphink)
Whiteboard: [checkin on 8/28]
Comment on attachment 8895636 [details] [diff] [review]
Throw for short structured clone reads

[Feature/Bug causing the regression]: structured cloning. The bug has been there since it landed.
[User impact if declined]: a compromised content process can do some limited out of bounds reads in the chrome process. Causing a crash is probably pretty easy. I haven't seen a way to get more than a very small information leak out of it.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes, landed earlier today
[Needs manual test from QE? If yes, steps to reproduce]: if QE wanted to try to reproduce these sort of fuzz bugs, I guess they'd use the instructions in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it's a slight modification of bounds checking that is already present, to make it a little more strict (and accurate)
[String changes made/needed]: none
Flags: needinfo?(sphink)
Attachment #8895636 - Flags: approval-mozilla-esr52?
Attachment #8895636 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/cd9aa3938195
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: javascript-core-security → core-security-release
Comment on attachment 8895636 [details] [diff] [review]
Throw for short structured clone reads

Fix a sec-high. Beta56+ & ESR52+.
Attachment #8895636 - Flags: approval-mozilla-esr52?
Attachment #8895636 - Flags: approval-mozilla-esr52+
Attachment #8895636 - Flags: approval-mozilla-beta?
Attachment #8895636 - Flags: approval-mozilla-beta+
Hi Christoph,
Can you help check if the issue was fixed in the latest nightly?
Flags: needinfo?(cdiehl)
Yes, this seems to be fixed. It was a frequent crasher and it did not yet pop up again.
Flags: needinfo?(cdiehl)
Whiteboard: [adv-main56+][adv-esr52.4+]
Flags: qe-verify-
Whiteboard: [adv-main56+][adv-esr52.4+] → [adv-main56+][adv-esr52.4+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.