1099110 - Add runtime check before downcast in BreakSink::SetCapitalization

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

I haven't pushed this to Try yet.  I'll wait for bug 1092363 to land first.

Comment 2

[Jonathan Kew [:jfkthame]]

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.

Comment 3

[Mats Palmgren (inactive)]

(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 4

[Mats Palmgren (inactive)]

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.

Comment 5

[Mats Palmgren (inactive)]

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.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Do you know if esr is affected?

Comment 7

[Mats Palmgren (inactive)]

Looks like it:
http://mxr.mozilla.org/mozilla-esr31/source/layout/generic/nsTextFrame.cpp#983

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 9

[Mats Palmgren (inactive)]

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

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Ah, good thinking!

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ac9581083a

Are we going to want this on esr31 as well?

Comment 13

[Mats Palmgren (inactive)]

Yes.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73ac9581083a

In addition to esr31, we should probably nominate it for b2g34 approval as well.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8522968 [details] [diff] [review]
fix

See comment 5.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b3b366d4d6c
https://hg.mozilla.org/releases/mozilla-beta/rev/12972395700a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/116ac16029ec

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0b3b366d4d6c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/decabdd4ebcd

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/decabdd4ebcd