Bugzilla

Randell Jesup [:jesup] (needinfo me)

Comment 2

•

7 years ago

INFO: Last good revision: 44ae8462d6ab (2014-03-12)
INFO: First bad revision: 46041cc216fd (2014-03-13)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44ae8462d6ab&tochange=46041cc216fd

Bug 948265 maybe?
https://hg.mozilla.org/mozilla-central/rev/7997cb5af49c

Has Regression Range: --- → yes

status-firefox53: --- → wontfix

status-firefox54: --- → affected

status-firefox55: --- → affected

status-firefox-esr45: --- → wontfix

status-firefox-esr52: --- → affected

tracking-firefox54: --- → ?

tracking-firefox55: --- → ?

tracking-firefox-esr52: --- → ?

Flags: needinfo?(jwatt)

Version: Trunk → 30 Branch

Updated

•

7 years ago

Keywords: csectype-bounds, sec-high

Gerry Chang [:gchang]

Comment 3

•

7 years ago

Track 54+/55+ as sec-high.

tracking-firefox54: ? → +

tracking-firefox55: ? → +

Assignee

Comment 4

•

7 years ago

Attached file simplified testcase — Details

Assignee: nobody → jwatt

Attachment #8868038 - Attachment is obsolete: true

Flags: needinfo?(jwatt)

Assignee

Comment 5

•

7 years ago

Attached patch part 1 - Prevent painting of non-displayable SVG elements (obsolete) — Details — Splinter Review

Either of these checks is sufficient to prevent this crash, but both seem like they should be added.

Attachment #8868981 - Flags: review?(dholbert)

Assignee

Comment 6

•

7 years ago

Attached patch part 2 - Add crashtest for crash painting non-displayable SVG element as background image (obsolete) — Details — Splinter Review

Attachment #8868982 - Flags: review?(dholbert)

Assignee

Comment 7

•

7 years ago

Attached patch part 2 - Add crashtest for crash painting non-displayable SVG element as background image — Details — Splinter Review

Attachment #8868982 - Attachment is obsolete: true

Attachment #8868982 - Flags: review?(dholbert)

Attachment #8868983 - Flags: review?(dholbert)

Daniel Holbert [:dholbert]

Updated

•

7 years ago

Group: core-security → layout-core-security

Comment 8

•

7 years ago

Comment on attachment 8868981 [details] [diff] [review]
part 1 - Prevent painting of non-displayable SVG elements

Review of attachment 8868981 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/painting/nsImageRenderer.cpp
@@ +194,5 @@
>          nsLayoutUtils::SurfaceFromElement(property->GetReferencedElement());
>        if (!mImageElementSurface.GetSourceSurface()) {
> +        nsIFrame* paintServerFrame = property->GetReferencedFrame();
> +        if (!paintServerFrame ||
> +            // trying to display non-displayable frames can crash

Right now this sounds kind of like a band-aid "oops let's not crash" kind of comment, which makes it a little harder to trust.

Could you pull this out of the if condition and phrase it more clearly to explain the control flow? Something like:

        // If there's no referenced frame, or the referenced frame is
        // non-displayable SVG, then we have nothing valid to paint.
        if (!paintServerFrame || [...]

::: layout/svg/nsSVGIntegrationUtils.cpp
@@ +1275,5 @@
>    }
>  
> +  if (aFrame->IsFrameOfType(nsIFrame::eSVG) &&
> +      !static_cast<nsSVGDisplayableFrame*>(do_QueryFrame(aFrame))) {
> +    return nullptr; // trying to display non-displayable frames can crash

This, too, feels a bit like an "oops how did we get here" kind of comment.  If it is -- and that might be OK -- we should perhaps convert it into a NS_ERROR(...) just before the "return nullptr".

(Because: this seems like something we should catch earlier on, when we decide on aFrame, ideally -- as we do in the nsImageRenderer codepath via your other tweak here.  So it'd be handy to have this be something that fuzzers can detect & file bugs about, so we can catch any other callers into this code that pass in non-displayable frames.)

Attachment #8868981 - Flags: review?(dholbert) → review+

Daniel Holbert [:dholbert]

Comment 9

•

7 years ago

Comment on attachment 8868983 [details] [diff] [review]
part 2 - Add crashtest for crash painting non-displayable SVG element as background image

Review of attachment 8868983 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the crashtest (landing after the fix has made it to all supported builds of course), assuming you've verified that it does crash in the crashtest harness in unpatched builds.

Attachment #8868983 - Flags: review?(dholbert) → review+

Assignee

Comment 10

•

7 years ago

The nsImageRenderer.cpp change is the earliest possible point where we can check whether the element that the content references is valid. That check is not a "just in case" check. It's entirely possible for content to reference nonsensical elements, and this is the logical place to guard against that.

The nsSVGIntegrationUtils.cpp is more of a safety check since there are ways other than -moz-element to enter this code. Adding an NS_ERROR sounds like a good idea. MOZ_ASSERT_UNREACHABLE sounds better to me though. :)

Assignee

Comment 11

•

7 years ago

Attached patch part 1 - Prevent painting of non-displayable SVG elements [r=dholbert] (obsolete) — Details — Splinter Review

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

It would take some work or knowledge of our code to figure out what sort of markup would get us into the error state. The apparent need for designMode is particularly non-obvious.

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

Hopefully not.

Which older supported branches are affected by this flaw?

All of them (everything back to March 2014 - https://hg.mozilla.org/mozilla-central/rev/7997cb5af49c )

If not all supported branches, which bug introduced the flaw?

Bug 948265.

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

Not backported yet, but should be simple enough.

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

Pretty unlikely. We're making an error condition a no-op.

Attachment #8868981 - Attachment is obsolete: true

Attachment #8871200 - Flags: sec-approval?

Attachment #8871200 - Flags: review+

Comment 12

•

7 years ago

sec-approval+ for trunk. 
We'll want this on Beta and ESR52 as well.

tracking-firefox-esr52: ? → 54+

Updated

•

7 years ago

Attachment #8871200 - Flags: sec-approval? → sec-approval+

Comment 13

•

7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3e7c43611ba7c586eacfd00af7dce905f97b92

Please request Beta and ESR52 approval on this at your earliest convenience.

Flags: needinfo?(jwatt)

Flags: in-testsuite?

Assignee

Comment 14

•

7 years ago

Comment on attachment 8871200 [details] [diff] [review]
part 1 - Prevent painting of non-displayable SVG elements [r=dholbert]

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec vuln
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: bug 948265
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none

Flags: needinfo?(jwatt)

Attachment #8871200 - Flags: approval-mozilla-esr52?

Attachment #8871200 - Flags: approval-mozilla-beta?

Comment 15

•

7 years ago

Comment on attachment 8871200 [details] [diff] [review]
part 1 - Prevent painting of non-displayable SVG elements [r=dholbert]

Had to back this out for reftest failures :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b51e0c8b6cce5ccc6c81c3bbbeafaf5dcab8b30

https://treeherder.mozilla.org/logviewer.html#?job_id=103244755&repo=mozilla-inbound

Attachment #8871200 - Flags: approval-mozilla-esr52?

Attachment #8871200 - Flags: approval-mozilla-beta?

Randell Jesup [:jesup] (needinfo me)

Comment 16

•

7 years ago

Marked Firefox 54 and ESR52.2 as won't fix since we've built release candidates or are in the process of doing so.

tracking-firefox-esr52: 54+ → 55+

Comment 17

•

7 years ago

As a sec-high that landed on m-c, is this a candidate for point releases, perhaps as an add-on?

Flags: needinfo?(abillings)

Comment 18

•

7 years ago

We can't ship add-ons for compiled code. The only options here are point release ride-along or shipping with 55/ESR52.3 at this point.

Comment 19

•

7 years ago

I'm not sure why I'm ni? here. This bounced and isn't in 54. Barring a point release, this goes to 55. Release Management will make the call on a point release but unless we think this is a realistic zero day, it probably is just going to wait for 55.

Flags: needinfo?(abillings)

Assignee

Comment 20

•

7 years ago

Drat, I just noticed this bounced when I happened to notice this is mentioned as being assigned to me in the latest tracking email. Investigating.

Flags: needinfo?(jwatt)

Daniel Holbert [:dholbert]

Assignee

Comment 21

•

7 years ago

Attached patch part 1 - Prevent painting of non-displayable SVG elements (obsolete) — Details — Splinter Review

It seems that -moz-element is allowed to reference SVG paint server's directly. This fixes that and now the failing tests pass.

Attachment #8871200 - Attachment is obsolete: true

Flags: needinfo?(jwatt)

Attachment #8879644 - Flags: review?(dholbert)

Comment 22

•

7 years ago

Comment on attachment 8879644 [details] [diff] [review]
part 1 - Prevent painting of non-displayable SVG elements

r=me with comment tweaked to hint at the now-modified condition, as discussed on IRC

Attachment #8879644 - Flags: review?(dholbert) → review+

Assignee

Comment 23

•

7 years ago

Attached patch part 1 - Prevent SVG elements that are neither displayable nor paint servers from painting [r=dholbert] — Details — Splinter Review

Attachment #8879644 - Attachment is obsolete: true

Attachment #8879650 - Flags: review+

Assignee

Comment 24

•

7 years ago

Attached patch part 1 patch for mozilla-beta — Details — Splinter Review

Same as for trunk but with a minor #include conflict resolved. Confirmed that this compiles.

Assignee

Comment 25

•

7 years ago

Attached patch part 1 patch for mozilla-esr52 — Details — Splinter Review

Had the same minor conflict in the #includes section, plus I had to change the name of nsSVGDisplayableFrame to its old class name nsISVGChildFrame to get the patch to compile.

Assignee

Comment 26

•

7 years ago

Oh, and for the esr52 patch the patched code is layout/base/nsCSSRendering.cpp rather than in layout/painting/nsImageRenderer.cpp. It's the same code though.

Assignee

Comment 27

•

7 years ago

Attached patch part 1 patch for mozilla-release (in the off chance we wanted to take this as a ridealong) — Details — Splinter Review

Again, the same minor conflict in the #includes section, and the patched code is is in layout/painting/nsCSSRendering.cpp rather than in layout/painting/nsImageRenderer.cpp as it is on trunk (same code though).

Assignee

Comment 28

•

7 years ago

Comment on attachment 8879650 [details] [diff] [review]
part 1 - Prevent SVG elements that are neither displayable nor paint servers from painting [r=dholbert]

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

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

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

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

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

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

It would take some work or knowledge of our code to figure out what sort of markup would get us into the error state. The apparent need for designMode is particularly non-obvious.

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

Hopefully not.

Which older supported branches are affected by this flaw?

All of them (everything back to March 2014 - https://hg.mozilla.org/mozilla-central/rev/7997cb5af49c )

If not all supported branches, which bug introduced the flaw?

Bug 948265.

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

Yes, backports for beta, esr52 and release all attached.

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

It has passed Try (pushed there since this essentially already made it to trunk). Should be low risk.

Attachment #8879650 - Flags: sec-approval?

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 29

•

7 years ago

Comment on attachment 8879650 [details] [diff] [review]
part 1 - Prevent SVG elements that are neither displayable nor paint servers from painting [r=dholbert]

sec-approval+ for trunk.

Attachment #8879650 - Flags: sec-approval? → sec-approval+

Comment 30

•

7 years ago

https://hg.mozilla.org/mozilla-central/rev/af94bc2592d1

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox56: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla56

Updated

•

7 years ago

Group: layout-core-security → core-security-release

Comment 31

•

7 years ago

Looks like this stuck :). Please request Beta & ESR52 approval on this when you get a chance.

status-firefox54: affected → wontfix

Flags: needinfo?(jwatt)

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Assignee

Comment 32

•

7 years ago

Comment on attachment 8879650 [details] [diff] [review]
part 1 - Prevent SVG elements that are neither displayable nor paint servers from painting [r=dholbert]

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: ESR has the sec issue.
User impact if declined: potential security hole.
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): should be low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: bug 948265.
[User impact if declined]: potential security hole.
[Is this code covered by automated tests?]: the attached test was run manually, but has not yet been landed due to security considerations.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: run the attached test.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: should be low risk.
[Why is the change risky/not risky?]: We're making an error condition a no-op.
[String changes made/needed]: none

Flags: needinfo?(jwatt)

Attachment #8879650 - Flags: approval-mozilla-esr52?

Attachment #8879650 - Flags: approval-mozilla-beta?

Comment 33

•

7 years ago

Comment on attachment 8879650 [details] [diff] [review]
part 1 - Prevent SVG elements that are neither displayable nor paint servers from painting [r=dholbert]

Sec-high, Beta55+, ESR52.3+

Attachment #8879650 - Flags: approval-mozilla-esr52?

Attachment #8879650 - Flags: approval-mozilla-esr52+

Attachment #8879650 - Flags: approval-mozilla-beta?

Attachment #8879650 - Flags: approval-mozilla-beta+

Comment 34

•

7 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/c1683cb4df45

status-firefox55: affected → fixed

Updated

•

7 years ago

Whiteboard: [adv-main55+]

Assignee

Comment 35

•

7 years ago

For ESR, should I land this just on tip, or also on FIREFOX_ESR_52_0_X_RELBRANCH?

Comment 36

•

7 years ago

You want to land on the default branch. Avoid anything marked as a RELBRANCH since those are only for out-of-band dot releases. That said, the ESR52 patch needs rebasing it appears :)

Flags: needinfo?(jwatt)

Comment 37

•

7 years ago

uplift

Jonathan rebased and landed this on ESR52.
https://hg.mozilla.org/releases/mozilla-esr52/rev/5c26df489768

status-firefox-esr52: affected → fixed

Flags: needinfo?(jwatt)

Updated

•

7 years ago

Whiteboard: [adv-main55+] → [adv-main55+][adv-esr52.3+]

Updated

•

7 years ago

Alias: CVE-2017-7786

Updated

•

7 years ago

Flags: sec-bounty?

Matt Wobensmith [:mwobensmith][:matt:]

Updated

•

7 years ago

Flags: sec-bounty? → sec-bounty+

Updated

•

7 years ago

Flags: qe-verify+

Whiteboard: [adv-main55+][adv-esr52.3+] → [adv-main55+][adv-esr52.3+][post-critsmash-triage]

Andrei Vaida [:avaida]

Comment 38

•

7 years ago

I was able to reproduce the crash on an affected asan build (55.0a1, 20170516101906) using the test case from Comment 4 on Ubuntu 16.04 x64.

The crash is no longer occurring on Ubuntu 16.04 x64 using the following builds:

      - asan 56.0a1 (20170801004347)
      - asan 55.0b14 (20170731114018)
      - asan 52.2.1esr (20170731121041)

Status: RESOLVED → VERIFIED

status-firefox55: fixed → verified

status-firefox56: fixed → verified

status-firefox-esr52: fixed → verified