Closed Bug 1099110 Opened 9 years ago Closed 9 years ago

Add runtime check before downcast in BreakSink::SetCapitalization


(Core :: Layout: Text and Fonts, enhancement)

Not set



Tracking Status
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 --- fixed
firefox-esr31 36+ 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


(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)


(Blocks 1 open bug)


(Keywords: sec-want, Whiteboard: [adv-main36-][adv-esr31.5-][b2g-adv-main2.2-])


(1 file)

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.

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.
Attached patch fixSplinter Review
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.
(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. :-(
Comment on attachment 8522968 [details] [diff] [review]

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+
Comment on attachment 8522968 [details] [diff] [review]

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?
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]
Attachment #8522968 - Flags: sec-approval? → sec-approval+
(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.

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

In addition to esr31, we should probably nominate it for b2g34 approval as well.
Closed: 9 years ago
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]

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-]
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.