Closed
Bug 1386787
Opened 7 years ago
Closed 7 years ago
IPC: heap-buffer-overflow [@ JSStructuredCloneReader::read]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
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)
10.01 KB,
text/plain
|
Details | |
117.70 KB,
text/plain
|
Details | |
5.27 KB,
patch
|
kanru
:
review+
gchang
:
approval-mozilla-beta+
gchang
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
Looks like an issue related to structured clone deserialization. [Faulty] pickle field {size_t} of value: 56 changed to: 55
Component: IPC → JavaScript Engine
Reporter | ||
Updated•7 years ago
|
Component: JavaScript Engine → IPC
Summary: IPC: heap-buffer-overflow [@ peek] → IPC: heap-buffer-overflow [@ JSStructuredCloneReader::read]
Updated•7 years ago
|
Component: IPC → JavaScript Engine
Reporter | ||
Comment 3•7 years ago
|
||
// 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
Reporter | ||
Updated•7 years ago
|
Component: IPC → JavaScript Engine
Comment 4•7 years ago
|
||
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.
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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.
Comment 7•7 years ago
|
||
(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.
Updated•7 years ago
|
Group: core-security → javascript-core-security
Assignee | ||
Comment 8•7 years ago
|
||
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)
Assignee | ||
Comment 9•7 years ago
|
||
But generally, I hadn't realized structured clone was an attack surface. I see more unguarded reads.
Assignee | ||
Comment 10•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment 11•7 years ago
|
||
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+
Comment 12•7 years ago
|
||
Content really shouldn't be able to crash chrome, if anything malicious content should get the content process killed :-)
Assignee | ||
Comment 13•7 years ago
|
||
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?
Comment 14•7 years ago
|
||
(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.
Comment 15•7 years ago
|
||
Thanks, I didn't realize that was already the case.
Comment 16•7 years ago
|
||
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.
status-firefox55:
--- → wontfix
status-firefox56:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox56:
--- → +
tracking-firefox57:
--- → +
tracking-firefox-esr52:
--- → 56+
Whiteboard: [checkin on 8/28]
Updated•7 years ago
|
Attachment #8895636 -
Flags: sec-approval? → sec-approval+
Comment 17•7 years ago
|
||
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]
Assignee | ||
Comment 18•7 years ago
|
||
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?
Comment 19•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cd9aa3938195
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Comment 20•7 years ago
|
||
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+
Comment 21•7 years ago
|
||
Hi Christoph, Can you help check if the issue was fixed in the latest nightly?
Flags: needinfo?(cdiehl)
Comment 22•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/234a1abc97dd
Reporter | ||
Comment 23•7 years ago
|
||
Yes, this seems to be fixed. It was a frequent crasher and it did not yet pop up again.
Flags: needinfo?(cdiehl)
Comment 24•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/b8417112486d
Updated•7 years ago
|
Whiteboard: [adv-main56+][adv-esr52.4+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main56+][adv-esr52.4+] → [adv-main56+][adv-esr52.4+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•