826995 - Fix CSS columns-related security issues (or disable the feature)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

-moz-column support is 

1) a constant source of security bugs,

2) impossible to test for security issues beyond "does this crash in an obviously exploitable way" (due to bug 588237 and bug 724978), which spills into making other parts of layout similarly difficult to test, and

3) essentially unmaintained (as can be seen from the state of those bugs), even though new features are being added on top (see bug 698783).

This was almost an acceptable state of affairs back when I was the only one regularly finding these bugs.  Now it's not.

Columns should go behind a pref until:

1) someone steps up to fix bug 588237 and bug 724978,

2) the resulting builds have gone through enough rounds of fuzz-and-fix that it at least takes me some effort to make it crash or violate key invariants, and

3) the feature gets a fresh security review.

Based on comments in bug 724978, it sounds like having the feature temporarily disabled might actually it less scary to fix the code...

Comment 1

[Masatoshi Kimura [:emk]]

When the ETA of reenabling? Indefinitely?

Comment 2

[Masatoshi Kimura [:emk]]

Some non-test codes are using -moz-column right now.
https://mxr.mozilla.org/mozilla-central/search?string=moz-column

Comment 3

[Jean-Yves Perrier [:teoli]]

Added dev-doc-needed as, if we disable it, we will need to document it.

Comment 4

[Scott Johnson (:jwir3)]

I think I support this idea, but I think we should come up with a specific set of criteria to exit the pref-controlled behavior. Specifically, as Jesse mentioned, we need a set of bugs fixed and a security review performed. 

I'm a but skeptical, though, of the fuzz-and-fix requirement (#2), since it's pretty nebulous (e.g. How many is "enough" rounds of fuzz-and-fix?)

Comment 5

[Jesse Ruderman]

> How many is "enough" rounds of fuzz-and-fix?

When we go a week without hitting any crashes or assertions* involving -moz-column (or due to regressions from related patches).

*Ignoring assertions about widths and heights, and other isolated assertions that are not relevant to safety.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Web pages actually rely on columns support.

Given we need to do something drastic here, I think the drastic thing should be to pull someone off something else to fix the columns bugs.

Comment 7

[Daniel Veditz [:dveditz]]

I hope we fix the bugs rather than disable the feature, but if we were to disable it we're unlikely to uplift such a change to aurora or beta so I'll mark those tracking-minus.

Comment 8

[Jet Villegas (inactive)]

Chatted with dbaron about this one. He's got a plan to address these crashers. Stay tuned...

Comment 9

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

Any updates here? It has been a while.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Al Billings [:abillings] from comment #9)
> Any updates here? It has been a while.

scott is trying to:

 * get bug 810726 landed (involves weakening incorrect assertion slightly)

 * get the branch patch on bug 724978 landed on trunk, which should have happened originally (involves dealing with a crash in one of the testcases) (this means not doing the full fix involving bug 743402)

Once those land, we'll see where we are with the fuzzers.

Comment 11

[bhavana bajaj [:bajaj]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10)
> (In reply to Al Billings [:abillings] from comment #9)
> > Any updates here? It has been a while.
> 
> scott is trying to:
> 
>  * get bug 810726 landed (involves weakening incorrect assertion slightly)

Looks like this has landed on aurora(Fx21).
> 
>  * get the branch patch on bug 724978 landed on trunk, which should have
> happened originally (involves dealing with a crash in one of the testcases)
> (this means not doing the full fix involving bug 743402)
Not ready to be uplifted on Fx21 yet, see a few unresolved dependencies.
> 
> Once those land, we'll see where we are with the fuzzers.

Fx21(now aurora) is going to become beta in ~2weeks, so are we still on target to resolve this before the merge keeping in mind rounds of fuzz-and-fix that may need to happen here ?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to bhavana bajaj [:bajaj] from comment #11)
> >  * get the branch patch on bug 724978 landed on trunk, which should have
> > happened originally (involves dealing with a crash in one of the testcases)
> > (this means not doing the full fix involving bug 743402)
> Not ready to be uplifted on Fx21 yet, see a few unresolved dependencies.

Those dependencies are not regressions and should not block uplift.  Though I'm also not sure we want to uplift everything.

Comment 13

[Jesse Ruderman]

I'm waiting to see how many assertions and crashes are left after Scott Johnson and David Baron fix bug 600100.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Jesse Ruderman from comment #13)
> I'm waiting to see how many assertions and crashes are left after Scott
> Johnson and David Baron fix bug 600100.

Looks like this was fixed. Any good news?

Comment 15

[Mats Palmgren (inactive)]

I've triaged the remaining Severity:critical bugs with the word "column" in the
Subject and I found no outstanding security bugs.  The remaining crash bugs
are bug 762332 (a debug-only fatal assertion) and bug 856235 which looks like
a flex-box bug triggered by columns.  Other than that, there are a few hangs --
I'm guessing we simply need to put an upper limit on the number of columns we
create during reflow (bug 730559).

I think the state of FF22 is likewise good.  So we only need to decide what
to do for FF21.  I don't think the problem in FF21 is bad enough to warrant
a risky change to disable columns support.  I'd rather see a few of the recent
crash fixes merged.

Comment 16

[Kohei Yoshino [:kohei]]

The summary and status of this bug is confusing...

Comment 17

[bhavana bajaj [:bajaj]]

(In reply to Mats Palmgren [:mats] from comment #15)
> I've triaged the remaining Severity:critical bugs with the word "column" in
> the
> Subject and I found no outstanding security bugs.  The remaining crash bugs
> are bug 762332 (a debug-only fatal assertion) and bug 856235 which looks like
> a flex-box bug triggered by columns.  Other than that, there are a few hangs
> --
> I'm guessing we simply need to put an upper limit on the number of columns we
> create during reflow (bug 730559).
> 
> I think the state of FF22 is likewise good.  So we only need to decide what
> to do for FF21.  I don't think the problem in FF21 is bad enough to warrant
> a risky change to disable columns support.  I'd rather see a few of the
> recent
> crash fixes merged.

Is this a wontfix for Fx21 given we do not want a risky change to disable columns support or are we planning on uplifting recent crash fixes here to fix the issue.Unclear on what those bugs are as there is nothing in dependency list here , can you please point me to them ?


fyi,we will be going to build with Fx21 beta 4 on Tuesday(Apr 23) morning & any low risk speculative uplifts must happen by then.Thanks !

Comment 18

[Mats Palmgren (inactive)]

I think bug 600100 and bug 724978 are the main ones.  I have a bunch of recent
crash fixes that were columns-related too, but most of them were mitigated by
frame-poisoning / null-pointer crashes so they can ride the trains.
Two others were fixed for FF21, bug 850931 and bug 812893.

I think at this point uplifting 600100 and bug 724978 probably isn't worth it
given the regression risks involved.  Scott knows more about their status
though so I'll defer to him.

Comment 19

[bhavana bajaj [:bajaj]]

(In reply to Mats Palmgren [:mats] from comment #18)
> I think bug 600100 and bug 724978 are the main ones.  I have a bunch of
> recent
> crash fixes that were columns-related too, but most of them were mitigated by
> frame-poisoning / null-pointer crashes so they can ride the trains.
> Two others were fixed for FF21, bug 850931 and bug 812893.
> 
> I think at this point uplifting 600100 and bug 724978 probably isn't worth it
> given the regression risks involved.

Given the risk analysis here and since we have already gone to build with beta & this bug will be fixed in Fx22, It seems worthwhile to maintain the status quo for Fx21, hence I am wontfixing this.

Please let me know if there is any disagreement on this and change the status flag accordingly .

>Scott knows more about their status
> though so I'll defer to him.

Scott, please email me or needsinfo me if anything else may happen to resolve the issue .

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't meet ESR branch landing criteria, untracking and wontfixing.

Comment 21

[Mats Palmgren (inactive)]

Resolving as fixed in FF22.
It appears we have consensus on wontfix for FF21 and older.