Closed Bug 1353312 (CVE-2017-7753) Opened 7 years ago Closed 7 years ago

logical property on pseudo-element that disallows it (e.g., border-inline-* on ::first-line) causes access violation at 0x14141414

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
54 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox52 - wontfix
firefox-esr52 55+ verified
firefox53 + wontfix
firefox54 + wontfix
firefox55 + verified
firefox56 + verified

People

(Reporter: abbGZcvu_bugzilla.mozilla.org, Assigned: heycam)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main55+][adv-esr52.3+])

Attachments

(4 files, 2 obsolete files)

repro.html
755 bytes, text/html
Details
simpler version of reporter's testcase
205 bytes, text/html
Details
BugId reports.zip
16.59 KB, application/x-zip-compressed
Details
patch v1.1 (good for central, beta and esr52)
4.36 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Attached file repro.html — 
Repro:

<html>
  <head>
    <style id=style>
      * {
        float:left;
      }
      ::first-line {
        border-inline-start: 0;
      }
      .boom ::first-line {
        border: 0;
      }
    </style>
    <script>
      onload = function() {
        document.body.className="boom";
        document.body.offsetTop;
        alert("Click OK to crash Firefox");
        document.body.className="x";
        // On x64, this causes an AV, but not on x86:
        document.body.offsetTop;
        // On x86, this code is reached. Refreshing the page appears to 
        // trigger the AV the next time the page shows the alert.
        location.reload();
      };
    </script>
  </head>
  <body>
    <x>x</x>
  </body>
</html>

Applying and removing the `nsRuleNode` for class "boom" appears to cause the code to attempt to apply the `nsConditionalResetStyleData` for the `nsRuleNode` with a "border-inline-start" property. This data is corrupt in that the `nsConditionalResetStyleData::Entry` instance at index 9 of the `mEntries` property (which deals with CSS border properties) has an `mStyleStruct` member that has the value 0x14141414 (x86) or 0x14141414`14141414 (x64). This causes an AV when the code calls `nsStyleBorder::CalcDifference` with this values as its `aNewData` argument, as that code assumes it is given a pointer to a valid `nsStyleVariables` instance and attempts to access members of this object.

I've attempted to find out where this value comes from by looking at various functions in the call stack at the time of the AV, but it appears to have been created before the JavaScript that causes the AV is executed.

I've also set breakpoints on various functions that create various objects implicated in this bug, before running the repro, but my understanding of Firefox CSS internals is limited, and I was unable to find out where this value comes from and why.

I hope somebody with better understanding of how the code is supposed to work can figure out if this is some kind of "magic value" that is not being special cased correctly, or if it's a type confusion where data from a different kind of object is incorrectly interpreted as containing a pointer where it has the value seen in the crash, or something else entirely.
Group: core-security → dom-core-security
Heycam, is this something you could look at? Thanks.

I'm not familiar with any of our poison values being 0x14141414, FWIW.
Flags: needinfo?(cam)
In a debug build I hit a fatal assert first:

   187    void SetStyleData(nsStyleStructID aSID, void* aStyleStruct) {
-> 188      MOZ_ASSERT(!(mConditionalBits & GetBitForSID(aSID)),
   189                 "rule node should not have unconditional and conditional style "
   190                 "data for a given struct");

aSID is eStyleStruct_Border.  mConditionalBits only contains one bit, and it's the one for Border.

In particular, mEntries[aSID] is non-null at this point.  And then we write our new pointer in there.  That seems fishy.

Why did the mStyleData.GetStyleBorder(aContext, aComputeData) call in nsRuleNode::GetStyleBorder not find this struct?
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
First, thanks for the bug report, SkyLined!

In the call stack where that assertion fires (on the first offsetTop call),
we're under a CalcStyleDifference call while restyling the ::first-line frame
for the <x> element.

Before restyling, the <x> element's Line frame's rule node looks like this:

  [0x7fed774c6510]
    mRule = *::first-line { border-inline-start: 0px none;} /* file:///tmp/repro.html:7 */
    mStyleData.mResetData = {
      mConditionalBits = eStyleStruct_Border
      mEntries[eStyleStruct_Border] = {
        mConditions = eHaveWritingMode (with WritingMode(0))
        mStyleStruct = (Border) 0x7fed774c9018 = { ... initial values ... }
      }
    }

So we computed a Border struct, and the RuleNodeCacheConditions indicated that
we could cache it on the rule node with a WritingMode dependency (due to the
use of the logical property). 

Then, we come to restyle the frame.  The new rule node looks like this:

  [0x7fed774ee8d0]
    mRule = .boom *::first-line { border: 0px none;} /* file:///tmp/repro.html:10 */
    mStyleData.mResetData = {
      mConditionalBits = 0
      mEntries[eStyleStruct_Border] =
        (Border) 0x7fed6fbe9c10 = { ... initial values ... }
    }
    mParent = 0x7fed774c6510

So we have another cached Border struct, but this time it's cached without 
dependencies, since there was no logical property mapped into the nsRuleData.

One interesting thing is that our computed RuleDetail, for both of these two
cached Border structs, is None, because none of the Border properties are
allowed in ::first-line rules.  So while we the computed RuleNodeCacheConditions
for the first Border struct had a writing-mode dependency, recorded when we
copied border-inline-start's longhands into the nsRuleData in MapRuleInfoInto,
we later call UnsetPropertiesWithoutFlags, clearing all of them out.

A second interesting thing is that the new Border struct, 0x7fed6fbe9c10, came
from the root rule node.  The loop in WalkRuleNode that looks up the rule tree
to find a start struct ignores structs that are cached with dependencies.  (I
never got around to the bug 804975 followup, bug 1176794, which would allow
caching of conditional structs up the rule tree.)  So in a way it is correct
that we look past the parent rule node, with its conditionally cached struct,
since we can't use it as a start struct.

But because we had a RuleDetail of None, we call PropagateDependentBit to
cache this initial struct all the way from our rule node up to the root.  This
is where the assertion fires, when we try to overwrite the conditionally cached
struct in the parent rule node with the unconditional struct we grabbed from
the root rule node.


My thinking in bug 804975 when I made nsConditionalResetStyleData store a
single unconditional struct or a list of conditional structs, but not both,
was that it wasn't possible for a given list of CSS rules to have a different
set of dependencies (e.g. for one element, computing the struct depends on
font-size, but for another element, computing the struct with the same rule
node has no dependencies).  But here, PropagateDepenentBit uses cached structs
to save time looking up the tree.  If later we want to resolve another Border
struct for a sibling rule node of the |.boom *::first-line| one, it would
be useful to find the cached unconditional Border struct on the parent, so that
we can avoid looking all the way up the tree again.

There are a few things we could do.

1. We could make nsConditionalResetStyleData support caching structs with and
without conditions.

2. We could skip the setting of the dependent bit and the call to SetStyleData
on the rule node that has the conditionally cached struct, in
PropagateDependentBit.  I'm not sure if that would break any existing
assumptions of the dependent bit always being set contiguously up the tree.  If
we take this option, we should also make
nsConditionalResetStyleData::SetStyleData safer when those assertions fire.

Besides that, we should probably also fix the unexpected case of using
conditions to store a struct whose RuleDetail ends up being None, which can
really only happen in this case where we have restricted rules like
::first-line.
(In reply to Cameron McCormack (:heycam) from comment #3)
> 1. We could make nsConditionalResetStyleData support caching structs with and
> without conditions.

I'm not convinced this is a good solution, now.  If we have a conditional struct cached on a rule node at some point, and then later we decide to store an unconditional struct there too (in PropagateDependentBit), and we upgrade the struct pointer to an nsConditionalResetStyleData::Entry list, subsequent GetStyleContext() calls will go through that list to find the first that matches.  And maybe we'll get the unconditional struct back.  I think it's the case that both of these two cached structs will have the same data, since the only reason we are storing a conditional struct and an unconditional struct is due to the RuleDetail None bug.  But it seems strange to start returning a different struct after the PropagateDependentBit call.

> 2. We could skip the setting of the dependent bit and the call to
> SetStyleData on the rule node that has the conditionally cached struct,
> in PropagateDependentBit.  I'm not sure if that would break any existing
> assumptions of the dependent bit always being set contiguously up the tree.

This, too, is not great.  If PropagateDependentBit is called first, and then later we request a struct (one which would have had conditions if we'd requested it earlier), then we'll just return the unconditional struct that PropagateDependentBit stored on there.  Again, these two structs should have the same value, but it's a bit odd that where the struct comes from (and whether it's conditional) depends on this ordering.

So I'm thinking we should just do these parts:

> If we take this option, we should also make
> nsConditionalResetStyleData::SetStyleData safer when those assertions fire.
> 
> Besides that, we should probably also fix the unexpected case of using
> conditions to store a struct whose RuleDetail ends up being None, which can
> really only happen in this case where we have restricted rules like
> ::first-line.
Attached patch patch (obsolete) — — Splinter Review 
I haven't run this patch through try yet, but I've verified it resolves the issue with the test case.  Also, if I remove the WalkRuleTree change, and disable the assertions in nsConditionalResetStyleData::SetStyle, we don't crash.  (Though we assert later that we leaked some objects from the pres arena.)
Attachment #8855251 - Flags: review?(dbaron)
Summary: Use of border-inline-start can cause access violation at 0x14141414... → Conditionally-cacheable declaration on pseudo-element that disallows it (e.g., border-* on ::first-line) causes access violation at 0x14141414
So I guess I don't understand why this can happen only with eRuleNone.  What prevents this from happening if there's some other cacheable-but-supported-on-::first-line declaration as well, in the same struct?

(I'm still looking for an example of such a combination.)
Flags: needinfo?(cam)
maybe:

#foo::first-letter {
  float: left;
  transform-origin: 2em 2em;
}
(and maybe the 2 declarations need to be in different rules, in a specific order, with the transform-origin one also applying to something else?)
The example in comment 7 doesn't work because nsStyleDisplay is never cached on ::first-letter.  The only other possible example I found was the interaction of opacity (supported) and box-shadow (unsupported, and can have font-size dependency) on ::placeholder, using nsStyleEffects.
Oh, but I guess I missed that it has to be a writing-mode dependency because that's the only one that's set during rule walking.
I suppose that also means that this is a regression from bug 1251797.
(In reply to David Baron :dbaron: ⌚️UTC+8 from comment #11)
> Oh, but I guess I missed that it has to be a writing-mode dependency because
> that's the only one that's set during rule walking.

Ah, good observation.

(In reply to David Baron :dbaron: ⌚️UTC+8 from comment #6)
> So I guess I don't understand why this can happen only with eRuleNone.  What
> prevents this from happening if there's some other
> cacheable-but-supported-on-::first-line declaration as well, in the same
> struct?

That would mean that we don't call PropagateDependentBit in WalkRuleTree, since that only happens if the (recomputed) RuleDetail is eRuleNone.  There is another call to PropagateDependentBit, in COMPUTE_END_RESET, but that one only sets the bit and structs up to aHighestNode.  I am struggling to find a way to make aHighestNode get selected such that this PropagateDependentBit call in COMPUTE_END_RESET will walk up to a rule node where we've previously cached a struct with conditions, since this rule node itself we're calling PropagateDependentBit on would have to have its (pre-recomputed) RuleDetail be eRuleNone for aHighestNode to be anything other than itself.
Flags: needinfo?(cam)
Summary: Conditionally-cacheable declaration on pseudo-element that disallows it (e.g., border-* on ::first-line) causes access violation at 0x14141414 → logical property on pseudo-element that disallows it (e.g., border-inline-* on ::first-line) causes access violation at 0x14141414
Comment on attachment 8855251 [details] [diff] [review]
patch

OK, r=dbaron.  I looked a little more, and agree this all looks good.

The one other thing that bugs me a little about this code is that when startStruct is non-null, WalkRuleTree can set highestNode to rootNode.  I don't think it matters since in that case we're guaranteed to hit the if (detail == eRuleNone && startStruct) and return early.  But it's a little odd that highestNode is wrong in that case.

(Maybe the if (!highestNode) highestNode = rootNode; should be after that early return?)
Attachment #8855251 - Flags: review?(dbaron) → review+
Comment on attachment 8855251 [details] [diff] [review]
patch

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

Not easily, since it requires some knowledge of how to set styles that opt in to specific style system optimizations, which isn't apparent from the code.

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

The comment illustrates that pseudo-elements with restricted properties are involved in the issue, but the remaining aspects of the issue (the use of logical properties, the specific rules that are needed) aren't mentioned.

Which older supported branches are affected by this flaw?

48+ (so, ESR52 and Release/Beta/Aurora/Nightly)

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

bug 1251797 (which arguably exposed a latent, but untriggerable, flaw in the initial bug 804975 work)

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

I haven't yet, but the code these patches touch isn't changed often, so the patch might well apply cleanly to branches.

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

Regressions seem unlikely; the changes to nsRuleNode.cpp just add some safety to cases we already were asserting about (and put us back into a more stable state), and the real fix in nsRuleNode.h is self contained enough for me to be happy with a normal try run (which I haven't done), and manual testing of the case we're worried about (which I have).
Attachment #8855251 - Flags: sec-approval?
Component: DOM: CSS Object Model → CSS Parsing and Computation
One other thought is that maybe the NS_ASSERTION should be a MOZ_ASSERT instead.
This needs a security rating before it can have sec-approval. You only need to request sec-approval for sec-high and sec-critical rated issues (and all security bugs need a rating to go in unless they only affect trunk).

Assuming it is a sec-high or sec-critical issue, it has missed the window for Firefox 53, which we are building for release in a few days. If it needs sec-approval, it isn't going to be allowed to checkin until May 2, two weeks into the next cycle.
Blocks: 1251797
Group: dom-core-security → layout-core-security
Keywords: regression
Where does the 0x14141414 value come from? It's not in the testcases.

On Mac I crash with a null deref on both testcases, but the marker value is in rbx and rsi (the 64-bit version).

On a 32-bit windows build I crash at 0x5f0f10af consistently (the eip value) with EXCEPTION_STACK_BUFFER_OVERRUN. I see the 0x14141414 values in ecx and esi. The stack points at the UntrackImage(aContext) call at the top of nsStyleBorder::Destroy() which doesn't seem like something that would cause a stack buffer overrun.
> On Mac I crash with a null deref on both testcases, but the marker value is
> in rbx and rsi (the 64-bit version).

That was on Firefox 52.0.2. When I try aurora (54) the 0x1414... value is consistently in rl4, rl5, and rdi (not rsi). It's crashing in a slightly different spot while destroying the nsStyleBorder.

Skylined: what Firefox version and platform were you testing when you saw the violation using the 0x14141414... address?
Flags: needinfo?(abbGZcvu_bugzilla.mozilla.org)
Keywords: sec-high
We could still take this for Monday's RC build, if you think it is important to land for 53. 
dbaron, what do you think? Do you feel confident enough here for this to land at the last minute in beta or would you rather wait?

If it can wait for 54, we can go with the plan Al suggests in comment 17. I'll check back over the weekend.
Flags: needinfo?(dbaron)
I'm flying back from Infiltrate today, so unfortunately I won't be able to look into this until Monday morning, CEST.
I'd be ok either way.  I think it's pretty low risk, but I'm not a fan of very-last-minute changes like that.  The other option would be to take only one half or the other of the patch (i.e., the clearing conditions, or the belt-and-braces bits), which I think would reduce risk a bit.
Flags: needinfo?(dbaron)
Track 54+/55+ as sec-high.
tracking-firefox54: ? → +
tracking-firefox55: ? → +
Tested with:
- Windows 10 Pro fully up-to-date, both x86 and x64 version running in a Hyper-V VM. 
- Firefox Developer Edition 54.0.0.6303 and .6308, x86 and x64.

I've spent quite some time trying to find out where the 0x14141414... value comes from, but was unable to find the source. However, I did find that it is stored in the `mStyleStruct` member of an `nsConditionalResetStyleData::Entry` instance, as explained in the original report.

On x64 `nsStyleContext::CalcStyleDifferenceInternal` (https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp#989) goes through a list of such `Entry` instances and calls the appropriate `nsStyleXXXX::CalcDifference` for each one, passing in the `mStyleStruct` value as an argument. In case of the `Entry` for borders, that value is corrupt, causing `nsStyleBorder::CalcDifference` (https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#441) to trigger an AV when it attempts to use it as a pointer (accessing `aNewData.*` will trigger the AV).

On x86, either the above code does not get executed, or it does not trigger an AV for some reason. I do not think the later is possible, so I assume the code must flow through an alternative code-path. However, I do not know why that may be and it makes no sense to me to have different code-paths on x86 and x64. This itself may be interesting to find out. The repro contains a comment that explains where during execution of Javascript the AV happens on x64: when layout is triggered by accessing the `offsetTop` property of an element. On x86, that does not trigger the AV and more Javascript can be executed. A page-refresh does trigger a similar crash when the style structures are getting destroyed, as the code attempts to free up the memory used by a `nsStyleBorder` instance it believes exists because of the same 0x14141414 pointer in `mStyleStruct`.

I do run with page-heap enabled, but AFAIK this should not affect the crash as Firefox does not use the Windows heap manager.
Flags: needinfo?(abbGZcvu_bugzilla.mozilla.org)
Attached file BugId reports.zip — 
Attached two BugId reports for x86 and x64. These contain version information on the application tested, a description of the crash, stacks, registers and disassembly in case you wanted more info than I provided in my previous comment. If anything is still missing, let me know.
The 0x14141414 value comes from nsStyleBorder::nsStyleBorder initializing
its mBorderRadius member, which is a nsStyleCorners struct:
http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/layout/style/nsStyleCoord.h#402
It just so happens that the unit nsStyleUnit::eStyleUnit_Coord is 0x14
so the mUnits array there gets filled with that value initially.
That memory location is then misinterpreted as a pointer.
Comment on attachment 8855251 [details] [diff] [review]
patch

Let's skip 53 then (comment 22) and get it into a release with a bit of beta testing. sec-approval=dveditz
Attachment #8855251 - Flags: sec-approval? → sec-approval+
(In reply to Mats Palmgren (:mats) from comment #26)
> The 0x14141414 value comes from nsStyleBorder::nsStyleBorder initializing
> its mBorderRadius member, which is a nsStyleCorners struct:
> http://searchfox.org/mozilla-central/rev/
> eace920a0372051a11d8d275bd9b8f14f3024ecd/layout/style/nsStyleCoord.h#402
> It just so happens that the unit nsStyleUnit::eStyleUnit_Coord is 0x14
> so the mUnits array there gets filled with that value initially.
> That memory location is then misinterpreted as a pointer.

Thanks for clarifying. Does this mean an attacker might influence the pointer by adding CSS border radius properties with various units to the repro? I tried a few such options myself, but had no luck getting anything other than 0x14141414...
You could try using percentage values on the border-radius properties:
https://drafts.csswg.org/css-backgrounds-3/#the-border-radius
(or combining % and px on different sides, and calc() too if we support that)

Just specifying a value might inhibit the "rule sharing" that is discussed
above though...  Perhaps setting it to a value in both rules, in a way that
would make the final value the same could work?  (see dbaron's testcase)
This landed last week:

https://hg.mozilla.org/mozilla-central/rev/08a6b726c361
https://hg.mozilla.org/releases/mozilla-beta/rev/6ed18ad42fc2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: affected → fixed
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: layout-core-security → core-security-release
Whiteboard: [fixed by bug 1356601]
I've been meaning to look into a PoC exploit that proves exploitability more thoroughly, but I am working on too many other projects that take priority (i.o.w. I get paid to do those). If you consider exploitability sufficiently obvious for a bug bounty, I'd rather spent my time on other things.
(In reply to SkyLined from comment #31)
> I've been meaning to look into a PoC exploit that proves exploitability more
> thoroughly, but I am working on too many other projects that take priority
> (i.o.w. I get paid to do those). If you consider exploitability sufficiently
> obvious for a bug bounty, I'd rather spent my time on other things.

I don't work on the bounty committee, but we often pay bounties without PoCs. I believe you do need to email security@mozilla.org about this bug (per https://www.mozilla.org/en-US/security/client-bug-bounty/ ) if you haven't already. If you have, try again, as the sec-bounty? flag should be set by somebody who works on the bounty committee and that doesn't seem to have happened yet.
Flags: sec-bounty?
Thanks, Daniel. I was not aware I had to ask for a bounty: I assumed that would happen automatically to confirmed security bugs.
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify+
Whiteboard: [fixed by bug 1356601] → [fixed by bug 1356601][post-critsmash-triage]
Whiteboard: [fixed by bug 1356601][post-critsmash-triage] → [fixed by bug 1356601][post-critsmash-triage][adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7753
It seems that this crash is still reproducible on all fixed builds - latest Nightly 55.a01 (2017-06-07), 54.0 RC (20170605204906) and esr 52.2.0 (20170607123825). I've used both testcases from comment 0 (FF crashes as soon as I open the testcase) and comment 9 (FF crashes but only after a page refresh). Tested on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 LTS.

Can you please take a look at this, Cameron?
Flags: needinfo?(cam)
SkyLined, can you verify that this is either fixed or that the problem still reproduces in builds?
Flags: needinfo?(abbGZcvu_bugzilla.mozilla.org)
The testcase from comment 0 attached crashes in a local Nightly for me:
190	    MOZ_ASSERT(!(mConditionalBits & GetBitForSID(aSID)),
191	               "rule node should not have unconditional and conditional style "
192	               "data for a given struct");
gdb) p aSID
$1 = eStyleStruct_Border
(gdb) p/x mConditionalBits
$3 = 0x40000
(gdb) p/x GetBitForSID(aSID)
$5 = 0x40000

It's trivially repro-able in rr

I'll note my code doesn't have the patch here.  And I don't see it in the history for nsRuleNode.h.  Comment 30 says it was landed on central and beta, but that links to:
Bug 1356601 - Don't force computation of a Variables struct when animations are involved. r=dbaron 
Which is not this patch, and not the one given s-a (though ryan marked "[fixed by bug 1356601]" after comment 30.

I'm looking at a (debug) rr run to see where those bits came from
aSid comes from "STYLE_STRUCT_RESET(Border, nullptr)" in nsStyleStructList.h:124 (nsRuleNode::GetStyleBorder<true>)

mConditionalBits comes from nsConditionalResetStyleData::SetStyleData() with aSid=eStyleStruct_Border, called from nsRuleNode::ComputeBorderData() (COMPUTE_END_RESET(Border, border))

should be easy to repro and debug under linux with rr
Al, which builds are your referring to? It still reproduces in 53.0.3 for me, which is the latest release build if I understand correctly.
A current Nightly or Beta build.
Per comment 27, this is expected to be unfixed in 53, I believe.  But I agree that the things linked to in comment 30 and the uplift to 52 don't match the patch that was supposed to land for this bug.  :(
A nightly build or a beta Firefox build.  The contention is that maybe we didn't fix this after all.
I think comment 37 is pretty clear on the fact that this is not fixed on nightly.
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #43)
> I think comment 37 is pretty clear on the fact that this is not fixed on
> nightly.

Yeah, I missed that. Looks like we're releasing 54 with this.
Ugh, I guess I got myself confused with these two bugs I was working on around the same time.  Sorry.
Flags: needinfo?(cam)
Comment on attachment 8855251 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1251797 (which arguably exposed a latent, but untriggerable, flaw in the initial bug 804975 work)
[User impact if declined]: it would be possible for a page to crash the tab, or interpret some memory that is somewhat under page control as a pointer
[Is this code covered by automated tests?]: there is a test but not included in the patch
[Has the fix been verified in Nightly?]: not landed yet
[Needs manual test from QE? If yes, steps to reproduce]: would be good just to verify it's actually fixed!
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: unlikely
[Why is the change risky/not risky?]: the changes to nsRuleNode.cpp just add some safety to cases we already were asserting about (and put us back into a more stable state), and the real fix in nsRuleNode.h is self contained enough for me to be happy with normal test coverage, and manual testing of the case we're worried about
[String changes made/needed]: none


[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: -
User impact if declined: as above
Fix Landed on Version: not landed yet
Risk to taking this patch (and alternatives if risky): I think the patch is low risk, for the reasons above.  Alternative would be to just take half the patch as dbaron mentions in comment 22.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(abbGZcvu_bugzilla.mozilla.org)
Attachment #8855251 - Flags: approval-mozilla-esr52?
Attachment #8855251 - Flags: approval-mozilla-beta?
Comment on attachment 8855251 [details] [diff] [review]
patch

This never landed, and we're about to release a new version, so this should really get sec-approval again, IMO.

(See comment 15 for the answers to the questions.)

ni? on heycam to make sure he sees this.
Flags: needinfo?(cam)
Attachment #8855251 - Flags: sec-approval+ → sec-approval?
Would there be any reason to consider this for 54 point releases?  Or should we wontfix it for 54?
Won't fix it for 54.

I'll give sec-approval+ for landing on 6/26, two weeks into the next cycle. We'll want it nominated with branch patches (Beta and ESR52) at that time too.
Whiteboard: [fixed by bug 1356601][post-critsmash-triage][adv-main54+][adv-esr52.2+] → [fixed by bug 1356601][post-critsmash-triage][checkin on 6/26]
Comment on attachment 8855251 [details] [diff] [review]
patch

(Never mind, I see the branch nominations.)
Attachment #8855251 - Flags: sec-approval? → sec-approval+
Flags: needinfo?(cam)
Comment on attachment 8855251 [details] [diff] [review]
patch

Sec-high, Beta55+
Attachment #8855251 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This needs a rebased patch.
Flags: needinfo?(cam)
Whiteboard: [fixed by bug 1356601][post-critsmash-triage][checkin on 6/26] → [post-critsmash-triage]
Tested locally that we assert without this patch, and no longer assert or crash with it, on beta.
Flags: needinfo?(cam)
Trees are closed right now.  Ryan, could you or a sheriff check these in to inbound and beta once they're open?
Keywords: checkin-needed
Hey Cameron, can you take a look at the inbound one i got 

Hunk #1 FAILED at 2549
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsRuleNode.cpp.rej
patching file layout/style/nsRuleNode.h
Hunk #1 succeeded at 190 with fuzz 2 (offset 5 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1353312.patch
Flags: needinfo?(cam)
status-firefox56: --- → affected
tracking-firefox56: --- → ?
Target Milestone: mozilla55 → ---
Attachment #8855251 - Attachment is obsolete: true
Attachment #8855251 - Flags: approval-mozilla-esr52?
Comment on attachment 8881657 [details] [diff] [review]
old backport for beta (don't land)

Just moving the ESR52 request over to the patch that landed. I've confirmed already that it grafts cleanly. Will use the Beta approval already granted in comment 51.
Attachment #8881657 - Flags: approval-mozilla-esr52?
also had to back this out from m-c for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=110326970&repo=mozilla-inbound 

Assertion is [task 2017-06-28T14:11:56.878099Z] 14:11:56     INFO - [Child 1091] ###!!! ASSERTION: we should already be cacheable without dependencies if we didn't have a pseudo restriction: 'ruleData.mConditions.CacheableWithoutDependencies() || pseudoRestriction', file /home/worker/workspace/build/src/layout/style/nsRuleNode.cpp, line 2689
Flags: needinfo?(cam)
Attachment #8881657 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Tracking for 56 since it looks like this still needs to land on m-c and beta.
tracking-firefox56: ? → +
OFF TOPIC. Is there some way I can get notified by email only of certain mayor changes to this bug, such as when this fix is released, rather than every time somebody sets a flag? I'm sure that's interesting to the developers, but as a reported I care less about the process and more about the result :).
(In reply to SkyLined from comment #61)
> OFF TOPIC. Is there some way I can get notified by email only of certain
> mayor changes to this bug, such as when this fix is released, rather than
> every time somebody sets a flag? I'm sure that's interesting to the
> developers, but as a reported I care less about the process and more about
> the result :).

Unfortunately not (most of this flag changing is not interesting to developers either...). Your best bet is just to stop receiving mail altogether for this specific bug, and manually check it occasionally. At this point in the bug's lifecycle, there's probably not going to be too much that is interesting.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #60)
> Tracking for 56 since it looks like this still needs to land on m-c and beta.

i talked to cameron on irc (since i noticed this bug on the esr uplift list :) and he is looking into this
Looking into one of the three test failures, 1356601-1.html, we're hitting the new assertion because we are resolve style for rules inside a ::first-line, and AnimValuesStyleRule is calling SetUncacheable() as it decides not to map its values (since animations don't apply to pseudos).  So we have another case where we can validly end up with a RuleNodeCacheConditions that indicates something other than "cacheable without conditions" when we had no rule data at all.

The style context we're resolving (under nsStyleSet::ResolveStyleByAddingRules, for the animation) is a child of the one with the ::first-line pseudo, which is why we don't think we have a pseudo restriction.
Flags: needinfo?(cam)
Should probably get review for the updated patch.

The difference from the first version is that in this case of no rule data, if we get the RuleNodeCacheConditions indicates "cacheable with conditions", we convert that to "cacheable without conditions", but we leave "uncacheable" as is.

Tested locally that the three tests from the earlier inbound landing are fixed by this.
Attachment #8883511 - Flags: review?(dbaron)
Comment on attachment 8883511 [details] [diff] [review]
patch v1.1 (good for central, beta and esr52)

r=dbaron, but please fix your diff settings to show 8 lines of context.  (It was a little annoying to diff this patch against the previous one I reviewed, since that one *did* have 8 lines of context.)
Attachment #8883511 - Flags: review?(dbaron) → review+
Al, do I need new approvals for this updated patch?  I don't think the change would make me answer any of the approval request forms any differently.
Flags: needinfo?(abillings)
(In reply to Cameron McCormack (:heycam) from comment #67)
> Al, do I need new approvals for this updated patch?  I don't think the
> change would make me answer any of the approval request forms any
> differently.

No, this is fine. Existing sec-approval can stand.
Flags: needinfo?(abillings)
Attachment #8881657 - Attachment description: backport for beta → old backport for beta (don't land)
Attachment #8881657 - Attachment is obsolete: true
Comment on attachment 8883511 [details] [diff] [review]
patch v1.1 (good for central, beta and esr52)

The patch applies to and works on beta and esr52.
Attachment #8883511 - Attachment description: patch v1.1 → patch v1.1 (good for central, beta and esr52)
Tomcat, could you check this in for me on central/beta/esr52?  Thanks!
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/c5507f94e8fe
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
> Is there some way I can get notified by email only of certain mayor changes to this bug

For just this bug, no.

For bugs in general, yes.  See https://bugzilla.mozilla.org/userprefs.cgi?tab=email for the editable preference matrix for which emails you get depending on your relationship to the bug.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main55+][adv-esr52.3+]
Reproduced the crash on an affected build (54.0.1, 20170628075643, 90f18f9c15f7) [1] using the test case from Comment 0 on Windows 10 x64.

I can confirm the test case is no logner crashing on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12 using:
    - 52.2esr (20170723091719, 3856be3d222f)
    - 55.0b10 (20170717063821, e26b1f5d635e)
    - 56.0a1 (2017-07-23, 20170723030206)

[1] bp-32cb6d01-287b-4bdb-be6a-364521170724
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: fixed → verified
status-firefox-esr52: fixed → verified
Flags: qe-verify+
Group: core-security-release
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: