Add runtime check before downcast in BreakSink::SetCapitalization

RESOLVED FIXED in Firefox 36

Status

()

--
enhancement
RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug, {sec-want})

Trunk
mozilla38
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 wontfix, firefox36+ fixed, firefox37+ fixed, firefox38 fixed, firefox-esr3136+ fixed, b2g-v1.4 wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main36-][adv-esr31.5-][b2g-adv-main2.2-])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Given that we've had a few exploitable crashes at the same place in
BreakSink::SetCapitalization now - I think it's justified to add
a test before the downcast in that method as a precaution.

http://hg.mozilla.org/mozilla-central/annotate/053d14064932/layout/generic/nsTextFrame.cpp#l964

We should wrap the code there like so:

if (mTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_TRANSFORMED) {
 ...
}

(same as in Finish() below it)

This is the only downcast in nsTextFrame.cpp that is done unchecked.

I'm hoping it will make future bugs like bug 1092363 etc non-exploitable.
The runtime cost should be negligible.
(Assignee)

Comment 1

4 years ago
Created attachment 8522968 [details] [diff] [review]
fix

I haven't pushed this to Try yet.  I'll wait for bug 1092363 to land first.
Yeah, this is essentially the same as the "wallpaper" I suggested a little while ago in bug 1081550. Given that we've seen several cases where we fail to (re)construct the right kind of textrun, I think it's worth having this.
(Assignee)

Comment 3

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Yeah, this is essentially the same as the "wallpaper" I suggested a little
> while ago in bug 1081550.

Sorry, I completely forgot about that. :-(
(Assignee)

Comment 4

4 years ago
Comment on attachment 8522968 [details] [diff] [review]
fix

Since we ran out of time to fix bug 1092363 in v35, I think we should
take this as a wallpaper.
Attachment #8522968 - Flags: review?(jfkthame)
Attachment #8522968 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 5

4 years ago
Comment on attachment 8522968 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: bug 1092363, although Cameron might land a wallpaper to avoid it, but we've had similar crash bugs historically so I think it's justified to have simple check here to avoid exploitable crashes in the future.
[User impact if declined]: potential exploitable crashes
[Describe test coverage new/current, TBPL]: none so far
[Risks and why]: low risk, it's a check to avoid doing call on a downcast pointer that will definitely crash unless this bit is set
[String/UUID change made/needed]: none

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

Probably not that hard once you find a crash, but note that there are
no known bugs that causes a crash here besides bug 1092363.

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

The patch reveals that it has to do with 'text-transform'.

Which older supported branches are affected by this flaw?

All, but this is just a preventive measure.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch also applies to Aurora (36) and Beta (35) as is.

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

Low risk.  No specific testing required.
Attachment #8522968 - Flags: sec-approval?
Attachment #8522968 - Flags: approval-mozilla-beta?
Attachment #8522968 - Flags: approval-mozilla-aurora?
Do you know if esr is affected?
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox-esr31: --- → ?
This has missed the window for Firefox 35. I'll give it sec-approval+ to go in on January 26, which is two weeks into the next cycle. That said, as a "sec-want" rated issue, it doesn't need sec-approval to go into trunk, that only applies to sec-high or sec-critical issues. Is this rated incorrectly for its impact?
Whiteboard: [checkin on 1/26]
status-firefox-esr31: ? → affected
Attachment #8522968 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 9

4 years ago
(In reply to Al Billings [:abillings] from comment #8)
> Is this rated incorrectly for its impact?

Bug 1092363 is rated sec-high, and this patch reveals some details, so I figured I should
ask for approval to be on safe side.
Ah, good thinking!
We're going to build 35 RC today and if this is good to wait, let's do that and not rush it into a final build.  Leaving the nomination open until the appropriate time in the 36 cycle and marking the esr tracking accordingly.
status-firefox35: affected → wontfix
tracking-firefox-esr31: --- → 36+
tracking-firefox36: --- → +
tracking-firefox37: --- → +
https://hg.mozilla.org/integration/mozilla-inbound/rev/73ac9581083a

Are we going to want this on esr31 as well?
status-firefox38: --- → affected
Flags: needinfo?(mats)
Whiteboard: [checkin on 1/26]
(Assignee)

Comment 13

4 years ago
Yes.
Flags: needinfo?(mats)
https://hg.mozilla.org/mozilla-central/rev/73ac9581083a

In addition to esr31, we should probably nominate it for b2g34 approval as well.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v1.4: --- → wontfix
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
status-firefox38: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8522968 - Flags: approval-mozilla-esr31+
Attachment #8522968 - Flags: approval-mozilla-beta?
Attachment #8522968 - Flags: approval-mozilla-beta+
Attachment #8522968 - Flags: approval-mozilla-aurora?
Attachment #8522968 - Flags: approval-mozilla-aurora+
Comment on attachment 8522968 [details] [diff] [review]
fix

See comment 5.
Attachment #8522968 - Flags: approval-mozilla-b2g34?
Attachment #8522968 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Whiteboard: [adv-main36-][adv-esr31.5-]
Whiteboard: [adv-main36-][adv-esr31.5-] → [adv-main36-][adv-esr31.5-][b2g-adv-main2.2-]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.