Bugzilla

Updated

•

8 years ago

Group: core-security → layout-core-security

Assignee

Comment 2

•

8 years ago

I'll look into this for now.

I notice that we're hitting an assertion in StyleAnimationValue::ExtractComputedValue where we get back a null unit for the stroke-dasharray property (strokeDasharray: 'none' in the test).

I wonder if that's going to leave us with a values array that violates some assumptions in BuildSegmentsFromValueEntries.

For my own reference, the stack in comment 0 points the finger at this line (which has shifted in current trunk m-c):

  https://hg.mozilla.org/mozilla-central/file/8d85de1a5c24/dom/animation/KeyframeUtils.cpp#l959

Looking at that line, I'm not entirely sure why we assume it's safe to access aEntries[i + 2]. I'll look into it a little more in a moment.

Assignee: nobody → bbirtles

Status: NEW → ASSIGNED

Flags: needinfo?(bbirtles)

Assignee

Comment 3

•

8 years ago

(In reply to Brian Birtles (:birtles) from comment #2)
> Looking at that line, I'm not entirely sure why we assume it's safe to
> access aEntries[i + 2]. I'll look into it a little more in a moment.

So it looks like it's based on the assertion that our first element has offset 0 and our last element has offset 1. We know that this point that aEntries[i + 1].mOffset == 0.0f so, based on the assertion about the last element, we must have at *least* one more frame at i + 2 (and if it is the last frame, it will have offset 1.0).

So perhaps we're getting into a situation where we have a keyframe at offset 0, but none at offset 1, hence we read past the end of the array. That probably wouldn't have happened when we checked the result of StyleAnimationValue::ExtractComputedValue earlier in the process, but now that we store nsCSSValues and convert to StyleAnimationValues at a later point I guess it does.

I know I ran some checks that even if StyleAnimationValue::ExtractComputedValue returned false we wouldn't do anything bad, but perhaps I forgot to check for the case of 0.0 being filled-in but 1.0 being left off.

That's all just a guess though, I've yet to step through and check. If it's correct, this probably affects aurora too (but not beta, I think).

Assignee

Comment 4

•

8 years ago

Oh, it looks like the problem is actually with:

  o59.animate([{dominantBaseline: 'use-script !important',all: 'initial'},{dominantBaseline: 'mathematical',all: 'unset'}],1);

For 'all', we compute the value for 'initial' ok, but when we get to the second keyframe and encounter 'unset', StyleAnimationValue::ComputeValues returns false so we don't add keyframes at offset 1.0 for those values.

So we have a situation where:

* We *try* to detect cases like this in advance in RequiresAdditiveAnimation but that explicitly does not catch all cases since it is called in situations where we don't have a context element so we can't check if all values can be computed sensibly or not. In this case, we check the 'all:unset' can be parsed correctly but don't check if it can be computed into suitable StyleAnimationValues and hence we don't detect this case in advance.

* We sort the KeyframeValueEntry objects by property and then offset

* In this case, although we have one keyframe with an offset of 1.0 (for dominant-baseline), it doesn't sort last since we sort first by property name so we fail to satisfy the condition that the last keyframe has offset 1.0.

So I guess we need to make BuildSegmentsFromValueEntries more robust to drop properties if we don't have a 0.0 or 1.0 keyframe.

(For what it's worth, I'm not sure if 'all:unset' is supposed to be use-able in keyframes but I guess it is.)

Al Billings [:abillings - ex-MoCo]

Assignee

Comment 5

•

8 years ago

It turns out that it's not as simple as all: unset always causing ComputeValues to fail. Rather, after dropping the assertion generated by strokeDasharray: 'none', we hit can get ComputeValues to fail (and generate a sequence of properties where we don't have a value at offset 1.0 for some properties) using the following reduced test case:

  <div id=div></div>
  <script>
  function start() {
    document.documentElement.animate(
      [ { strokeDasharray: 'none' },
        { strokeDasharray: 'none' } ], 1);
    div.animate([{ all: 'initial' },{ all: 'unset' }], 1);
  }
  </script>
  <body onload="start()"></body>

It doesn't reproduce with other properties that I've tried, only strokeDasharray. It doesn't reproduce if the two animations are applied to the same element.

It does reproduce if I replace 'unset' with 'inherit'.

I guess we go to inherit the 'none' value but, in ComputeValuesFromStyleRule, we will get false back StyleAnimationValue::ExtractComputedValue. I'll have to check tomorrow why we recover successfully if we just do:

   document.documentElement.animate(
    [ { strokeDasharray: 'none' },
      { strokeDasharray: '5, 5' } ], 1);

But not if we inherit 'none' through a shorthand.

Updated

•

8 years ago

Keywords: sec-high

Assignee

Comment 6

•

8 years ago

So it looks like part of the issue with stroke-dasharray is that as of [1] we represent stroke-dasharray: none" with single-value nsCSSValueList. However, when we go to inherit that we don't expect 'none' values in the list.

That's a simple fix but I'm concerned there will be other cases similar to this and we need a more general solution.

As far as I can see, we can do two things:

1) Make BuildSegmentsFromValueEntries robust enough to entry lists where not all properties have 0.0 and 1.0 keyframes. This is easy enough but I don't think we can write an automated test for this. I mean, we can write one using the stroke-dasharray but once we fix that the test will no longer work. Likewise for any other property:value combinations that trigger ComputeValues to fail.

2) Make StyleAnimationValue::ComputeValues fill in all values with 'none' values or something suitable even in the failure case. That seems difficult, however, since there are a bunch of reasons we might fail in that method and for all of them we'd have to still make sure we go through all the component properties and set up 'none' values. It's also not clear what we should do if the passed-in property is disabled.

I'm going to go ahead and do (1) even if we can't write an automated test (or at least not one that we can check-in -- I'll write one locally). That might be ok since we hope within about 6 months to implement additive animation which should provide the fallback behavior to use in this case.

[1] https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d

Assignee

Comment 7

•

8 years ago

Looking into this further (and trying to write a test case), I'm starting to suspect bug 1269175 for this (https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d). I'm hitting assertions where the nsStyleCoord has a null unit which makes me suspect we're not managing the memory of those dasharray arrays correctly.

Assignee

Comment 8

•

8 years ago

(In reply to Brian Birtles (:birtles) from comment #7)
> Looking into this further (and trying to write a test case), I'm starting to
> suspect bug 1269175 for this
> (https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d)

Nope, that's not it. I can reproduce the assertion on the parent of that changeset.

Assignee

Comment 9

•

8 years ago

More likely it's the fact that nsRuleNode::ComputeSVGData doesn't recognize that we can have none values in the list so it leaves the coord initialized to none. However, as of bug 524539 we represent none values as nsCSSValueList objects where the first value is none.[1]

[1] https://hg.mozilla.org/mozilla-central/rev/1c1825d6960d

So we can either try to emulate that in mStrokeDasharray, or probably better, recognize that pattern (a list with a single element whose value is none), and leave the array empty.

It's probably not necessary to fix this bug, however.

Assignee

Comment 10

•

8 years ago

Attached patch Handle entries arrays which don't have values at 0.0/1.0 offset (obsolete) — Details — Splinter Review

Assignee

Comment 11

•

8 years ago

Attached patch Test changes to handling of entries array (not to be checked-in) (obsolete) — Details — Splinter Review

Assignee

Comment 12

•

8 years ago

Attached patch Recognize none values of stroke-dasharray represented as a single-value nsCSSValueList when computing SVG data — Details — Splinter Review

This can probably be landed in a separate public bug but I'm just putting it here now since I don't have anywhere else to put it

Assignee

Comment 13

•

8 years ago

Comment on attachment 8759545 [details] [diff] [review]
Handle entries arrays which don't have values at 0.0/1.0 offset

Hi Cameron, what do you think of this? This is not as trivial as I hoped. I've written tests (attachment 8759546 [details] [diff] [review]) but they're not in a state they could be checked-in. I'm not even sure how we could make them landable short of adding some fairly invasive test API. I'll have a think about it though.

Comment 6 explains a bit of background. Basically, I'm trying to add code to safely handle when ComputeValues fails.

Attachment #8759545 - Flags: feedback?(cam)

Assignee

Comment 14

•

8 years ago

(In reply to Brian Birtles (:birtles) from comment #13)
> Hi Cameron, what do you think of this? This is not as trivial as I hoped.
> I've written tests (attachment 8759546 [details] [diff] [review]) but
> they're not in a state they could be checked-in. I'm not even sure how we
> could make them landable short of adding some fairly invasive test API. I'll
> have a think about it though.

Actually, I reckon if I look a bit harder I can probably find another example of a shorthand value that parses but doesn't compute. It might even be as simple as using variables. Then we could write tests for this.

Assignee

Comment 15

•

8 years ago

(In reply to Brian Birtles (:birtles) from comment #14)
> Actually, I reckon if I look a bit harder I can probably find another
> example of a shorthand value that parses but doesn't compute. It might even
> be as simple as using variables. Then we could write tests for this.

So far I haven't been able to succeed in doing this. Even if I use variables and stuff them with all sorts of invalid values, we don't fail to compute the value because we'll just ignore the invalid token stream value and return a style context with the default values filled-in. At this stage I can't think of any way to reliably reproduce this situation once we fix the stroke-dasharray bug.

Assignee

Comment 16

•

8 years ago

Comment on attachment 8759545 [details] [diff] [review]
Handle entries arrays which don't have values at 0.0/1.0 offset

I can't think of anything better to do at this stage. I don't like landing this without automated tests we can land with it (or shortly after it) but it still seems better to land something locally tested that is known to fix a security bug, than do nothing.

Perhaps we could add a chrome-only API that lets us toggle a kind of test mode where we can do something like attachment 8759546 [details] [diff] [review]? i.e. recognize a certain value and fail when we see it?

Attachment #8759545 - Flags: feedback?(cam) → review?(cam)

Comment 17

•

8 years ago

Comment on attachment 8759545 [details] [diff] [review]
Handle entries arrays which don't have values at 0.0/1.0 offset

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

::: dom/animation/KeyframeUtils.cpp
@@ +977,5 @@
>  
>    size_t i = 0, n = aEntries.Length();
>  
>    while (i + 1 < n) {
> +    // Skip properties that don't have an entry with offset 0.

We should have a comment somewhere that says we have to handle properties with missing t=0/1 values in this way due to the possibility of ComputeValues failing earlier.

@@ +989,5 @@
> +    }
> +
> +    // Drop properties that don't end with an entry with offset 1.
> +    if (aEntries[i].mProperty != aEntries[i + 1].mProperty &&
> +        aEntries[i].mOffset != 1.0f) {

Because the loop goes up to the second last element, I think we'll miss the case where we are missing an mOffset == 1.0f entry at the very end of the aEntries array.

@@ +1006,5 @@
>        // We need to generate an initial zero-length segment.
>        MOZ_ASSERT(aEntries[i].mProperty == aEntries[i + 1].mProperty);
>        j = i + 1;
> +      while (aEntries[j + 1].mOffset == 0.0f &&
> +             aEntries[j + 1].mProperty == aEntries[j].mProperty) {

Let's ensure we have a test for this case, entries like:

  (property = X, offset = 0.0)
  (property = Y, offset = 0.0)
  (property = Y, offset = 1.0)

@@ +1011,5 @@
>          ++j;
>        }
>      } else if (aEntries[i].mOffset == 1.0f) {
> +      if (aEntries[i + 1].mOffset == 1.0f &&
> +          aEntries[i + 1].mProperty == aEntries[i].mProperty) {

And for this:

  (property = X, offset = 0.0)
  (property = X, offset = 1.0)
  (property = Y, offset = 1.0)

@@ +1021,5 @@
>            ++j;
>          }
>        } else {
>          // New property.
> +        animationProperty = nullptr;

With this and the other |animationProperty = nullptr| assignment earlier, will it always be null once we're about to move on to a new property?  If so (and I think it is) please assert just before we assign the new appended AnimationProperty to it below.

I wonder if we should also be asserting that the mProperty of the entries are never eCSSProperty_UNKNOWN, since we use that as a dummy value just before the loop to detect the first time to append a new AnimationProperty.

@@ -1003,5 @@
>          }
>        } else {
>          // New property.
> -        MOZ_ASSERT(aEntries[i + 1].mOffset == 0.0f);
> -        MOZ_ASSERT(aEntries[i].mProperty != aEntries[i + 1].mProperty);

I think it's safe to leave this second assertion.  If the current entry has mOffset == 1.0f, and the following entry has mOffset != 1.0f, then it must be for a different property (even if we consider missing offsets due to failed ComputedValues calls).

Attachment #8759545 - Flags: review?(cam)

Comment 18

•

8 years ago

(In reply to Brian Birtles (:birtles) from comment #16)
> Perhaps we could add a chrome-only API that lets us toggle a kind of test
> mode where we can do something like attachment 8759546 [details] [diff] [review]
> [review]? i.e. recognize a certain value and fail when we see it?

I'm fine with a [ChromeOnly] thing that can cause ComputeValues to fail.

From IRC discussion, can we have it be a [ChromeOnly] dictionary member on one of the animate() arguments, which makes a value at a specific index fail to compute?  I think that would be better than hard coding in a special failing value like 77%.

Assignee

Comment 19

•

8 years ago

(In reply to Cameron McCormack (:heycam) from comment #18)
> (In reply to Brian Birtles (:birtles) from comment #16)
> > Perhaps we could add a chrome-only API that lets us toggle a kind of test
> > mode where we can do something like attachment 8759546 [details] [diff] [review]
> > [review]? i.e. recognize a certain value and fail when we see it?
> 
> I'm fine with a [ChromeOnly] thing that can cause ComputeValues to fail.
> 
> From IRC discussion, can we have it be a [ChromeOnly] dictionary member on
> one of the animate() arguments, which makes a value at a specific index fail
> to compute?  I think that would be better than hard coding in a special
> failing value like 77%.

I'm having a hard time doing this without introducing an extra bool member on Keyframe (since the point where we read this value off the dictionary is quite separate from the point where we go to call ComputeValues on the Keyframe object we generate as a result). It seems unfortunate to swell the size of all Keyframes for the sake if this check. Perhaps I can exploit some of the other members of the nsCSSTokenStream class to indicate a value we should reject.

Comment 20

•

8 years ago

Hmm, can you just use a valid CSS value with invalid variable references that are guaranteed to be invalid at computed value time, like "var(--invalid)" without that variable existing?  Or does that not cause ComputeValues to fail.  If you need to store something additional on nsCSSTokenStream there's free space after mLevel.

Assignee

Comment 21

•

8 years ago

(In reply to Cameron McCormack (:heycam) from comment #20)
> Hmm, can you just use a valid CSS value with invalid variable references
> that are guaranteed to be invalid at computed value time, like
> "var(--invalid)" without that variable existing?  Or does that not cause
> ComputeValues to fail.  If you need to store something additional on
> nsCSSTokenStream there's free space after mLevel.

Yeah, I'm pretty sure invalid variables don't do the trick -- we just skip over them.

However, we only really need to test failures on shorthands, I think. Or at least, that should be sufficient. And we currently represent shorthands as token streams where we just leave all the fields at their default values (e.g. eCSSProperty_UNKNOWN) and stick the string in the mTokenStream member. We pull out the mTokenStream string and pass that along when we actually go to compute the value.

So I think we could repurpose any of those other members to indicate "shorthand value that we should fail on". As long as we carefully document it and encapsulate that behavior in KeyframeUtils I think it's ok. We already check the value of the mProperty fields etc. to distinguish between token streams that represent:

* Shorthands
* Longhands that are valid token streams (e.g. have variable references)
* Longhands that failed to parse but which we stuffed in a token stream so we can return the original values from getKeyframes()

So we'd just be extending that logic to differentiate between two types of shorthand values.

Comment 22

•

8 years ago

Sounds good.

Assignee

Comment 23

•

8 years ago

(In reply to Cameron McCormack (:heycam) from comment #17)
> ::: dom/animation/KeyframeUtils.cpp
> @@ +977,5 @@
> >  
> >    size_t i = 0, n = aEntries.Length();
> >  
> >    while (i + 1 < n) {
> > +    // Skip properties that don't have an entry with offset 0.
> 
> We should have a comment somewhere that says we have to handle properties
> with missing t=0/1 values in this way due to the possibility of
> ComputeValues failing earlier.

Added.

> @@ +989,5 @@
> > +    }
> > +
> > +    // Drop properties that don't end with an entry with offset 1.
> > +    if (aEntries[i].mProperty != aEntries[i + 1].mProperty &&
> > +        aEntries[i].mOffset != 1.0f) {
> 
> Because the loop goes up to the second last element, I think we'll miss the
> case where we are missing an mOffset == 1.0f entry at the very end of the
> aEntries array.

Fixed and added test.

> @@ +1006,5 @@
> >        // We need to generate an initial zero-length segment.
> >        MOZ_ASSERT(aEntries[i].mProperty == aEntries[i + 1].mProperty);
> >        j = i + 1;
> > +      while (aEntries[j + 1].mOffset == 0.0f &&
> > +             aEntries[j + 1].mProperty == aEntries[j].mProperty) {
> 
> Let's ensure we have a test for this case, entries like:
> 
>   (property = X, offset = 0.0)
>   (property = Y, offset = 0.0)
>   (property = Y, offset = 1.0)

That should be covered by the following (newly-added) case:

    frames:   [ { margin: '5px', right: '10px' },
                { margin: '5px', right: '20px',
                  simulateComputeValuesFailure: true } ],

> @@ +1011,5 @@
> >          ++j;
> >        }
> >      } else if (aEntries[i].mOffset == 1.0f) {
> > +      if (aEntries[i + 1].mOffset == 1.0f &&
> > +          aEntries[i + 1].mProperty == aEntries[i].mProperty) {
> 
> And for this:
> 
>   (property = X, offset = 0.0)
>   (property = X, offset = 1.0)
>   (property = Y, offset = 1.0)

That should be covered by the (existing) case:

    frames:   [ { margin: '77%', left: '10px',
                  simulateComputeValuesFailure: true },
                { margin: '5px', left: '20px' } ],

> @@ +1021,5 @@
> >            ++j;
> >          }
> >        } else {
> >          // New property.
> > +        animationProperty = nullptr;
> 
> With this and the other |animationProperty = nullptr| assignment earlier,
> will it always be null once we're about to move on to a new property?  If so
> (and I think it is) please assert just before we assign the new appended
> AnimationProperty to it below.

Added.

> I wonder if we should also be asserting that the mProperty of the entries
> are never eCSSProperty_UNKNOWN, since we use that as a dummy value just
> before the loop to detect the first time to append a new AnimationProperty.

Added.

> @@ -1003,5 @@
> >          }
> >        } else {
> >          // New property.
> > -        MOZ_ASSERT(aEntries[i + 1].mOffset == 0.0f);
> > -        MOZ_ASSERT(aEntries[i].mProperty != aEntries[i + 1].mProperty);
> 
> I think it's safe to leave this second assertion.  If the current entry has
> mOffset == 1.0f, and the following entry has mOffset != 1.0f, then it must
> be for a different property (even if we consider missing offsets due to
> failed ComputedValues calls).

You're right, I've added it back. I originally dropped it before changing the condition before it.

Patches forthcoming once I split them up correctly.

Assignee

Comment 24

•

8 years ago

Attached patch part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset (obsolete) — Details — Splinter Review

MozReview-Commit-ID: GCCF4taOC6z

Attachment #8760599 - Flags: review?(cam)

Assignee

Updated

•

8 years ago

Attachment #8759545 - Attachment is obsolete: true

Assignee

Comment 25

•

8 years ago

Attached patch part 2 - Add tests for entries array handling when ComputeValues fails (obsolete) — Details — Splinter Review

MozReview-Commit-ID: DIQyt7f91an

Attachment #8760604 - Flags: review?(cam)

Assignee

Updated

•

8 years ago

Attachment #8759546 - Attachment is obsolete: true

Comment 26

•

8 years ago

Comment on attachment 8760599 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

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

::: dom/animation/KeyframeUtils.cpp
@@ +1004,5 @@
> +    // Drop properties that don't end with an entry with offset 1.
> +    if ((aEntries[i].mProperty != aEntries[i + 1].mProperty &&
> +         aEntries[i].mOffset != 1.0f) ||
> +        // Including when this is the last segment in the list
> +        (i == n - 2 && aEntries[i + 1].mOffset != 1.0f)) {

Is this right?  What about an array that has these three entries:

  [0] mProperty = P1, mOffset = 0.0f
  [1] mProperty = P1, mOffset = 1.0f
  [2] mProperty = P2, mOffset = 0.0f

The first time around the loop, i = 0 and we create a new AnimationProperty and add the first segment to it.  The next time around, i = 1 and this condition will be true, and we'll remove that sole segment from aResult.

Assignee

Comment 27

•

8 years ago

(In reply to Cameron McCormack (:heycam) from comment #26)
> Comment on attachment 8760599 [details] [diff] [review]
> part 1 - Handle entries arrays where we don't have a property value at the
> 0.0/1.0 offset
> 
> Review of attachment 8760599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/KeyframeUtils.cpp
> @@ +1004,5 @@
> > +    // Drop properties that don't end with an entry with offset 1.
> > +    if ((aEntries[i].mProperty != aEntries[i + 1].mProperty &&
> > +         aEntries[i].mOffset != 1.0f) ||
> > +        // Including when this is the last segment in the list
> > +        (i == n - 2 && aEntries[i + 1].mOffset != 1.0f)) {
> 
> Is this right?  What about an array that has these three entries:
> 
>   [0] mProperty = P1, mOffset = 0.0f
>   [1] mProperty = P1, mOffset = 1.0f
>   [2] mProperty = P2, mOffset = 0.0f
> 
> The first time around the loop, i = 0 and we create a new AnimationProperty
> and add the first segment to it.  The next time around, i = 1 and this
> condition will be true, and we'll remove that sole segment from aResult.

Ah, I guess we can't actually reproduce that case, hence why none of the tests fail. The closest we can reproduce is:

>   [0] mProperty = P1, mOffset = 0.0f
>   [1] mProperty = P1, mOffset = 1.0f
>   [2] mProperty = P2, mOffset = 0.0f
>   [3] mProperty = P3, mOffset = 0.0f
>   [4] mProperty = P4, mOffset = 0.0f
>   [5] mProperty = P5, mOffset = 0.0f

I guess we need a separate branch to deal with the last segment case.

Comment 28

•

8 years ago

Comment on attachment 8760604 [details] [diff] [review]
part 2 - Add tests for entries array handling when ComputeValues fails

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

Please add some brief comments in nsCSSValue.h mentioning the different kinds of special values we can store in nsCSSTokenStreams.  r=me with that and if you add a test for the case that I mention in the part 1 review.  You'll need to get a DOM peer to review the .webidl change.

Attachment #8760604 - Flags: review?(cam) → review+

Assignee

Comment 29

•

8 years ago

Attached patch part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset — Details — Splinter Review

MozReview-Commit-ID: GCCF4taOC6z

Attachment #8760614 - Flags: review?(cam)

Assignee

Updated

•

8 years ago

Attachment #8760599 - Attachment is obsolete: true

Attachment #8760599 - Flags: review?(cam)

Assignee

Comment 30

•

8 years ago

Attached patch part 2 - Add tests for entries array handling when ComputeValues fails (obsolete) — Details — Splinter Review

Olli, would you mind checking the webidl changes here? Thanks

Attachment #8760615 - Flags: review?(bugs)

Assignee

Updated

•

8 years ago

Attachment #8760604 - Attachment is obsolete: true

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 31

•

8 years ago

Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

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

Looks good, thanks.

Attachment #8760614 - Flags: review?(cam) → review+

Comment 32

•

8 years ago

Comment on attachment 8760615 [details] [diff] [review]
part 2 - Add tests for entries array handling when ComputeValues fails

r+ for .webidl

Attachment #8760615 - Flags: review?(bugs) → review+

Al Billings [:abillings - ex-MoCo]

Assignee

Comment 33

•

8 years ago

Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
See below--somewhat easily.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
Not a bulls-eye, but pretty close. There's a comment that specifically mentions that this patch makes us do the right thing when StyleAnimationValue::ComputeValues fails. So basically it tells you that if you can find a way to get ComputeValues to fail (non-trivial, but possible) then you could reproduce the problem. Shall I drop the comment?

> Which older supported branches are affected by this flaw?
Beta and Aurora (i.e. 48 and 49)

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

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?
This patch appears to apply cleanly to aurora and beta.

> How likely is this patch to cause regressions; how much testing does it need?
It's not likely to cause functional regressions--it simply makes us do something sensible in a particular edge case that normally should not happen. Part 2 (which I plan to land later, perhaps after we've opened up this bug) includes tests for quite a large combination of inputs, however it's possible that there is some combination we've overlooked. If that's the case, is's likely to be a further edge case rather than a new regression. I haven't run these tests on try but have verified that the tests that exercise this code under dom/animation/test/, testing/web-platform/tests/web-animations, and layout/style/test/test_{animation|transition}* pass locally on Windows with this patch applied.

Attachment #8760614 - Flags: sec-approval?

Comment 34

•

8 years ago

Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

sec-approval+ for trunk since it only affects trunk, beta, and aurora.

Please make and nominate 48 and 49 patches too.

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

Al Billings [:abillings - ex-MoCo]

Updated

•

8 years ago

status-firefox47: --- → unaffected

status-firefox48: --- → affected

status-firefox50: --- → affected

status-firefox-esr45: --- → unaffected

tracking-firefox48: --- → +

tracking-firefox49: --- → +

tracking-firefox50: --- → +

Assignee

Comment 35

•

8 years ago

Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

Approval Request Comment
[Feature/regressing bug #]: Bug 1245748
[User impact if declined]: Potentially exploitable security bug
[Describe test coverage new/current, TreeHerder]: Test written (attachment 8760615 [details] [diff] [review]) but won't be landed until the patch has landed on affected branches.
[Risks and why]: I haven't run the patch through try because I didn't want to expose the vulnerability. I'll land this on trunk momentarily and so we'll find out pretty soon if there are any major problems. Otherwise, see comment 33.
[String/UUID change made/needed]: None.

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

Attachment #8760614 - Flags: approval-mozilla-aurora?

Assignee

Comment 36

•

8 years ago

Part 1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/451483416eed6a8be15a6e2253321c9e99b45b04
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=451483416eed6a8be15a6e2253321c9e99b45b04

Keywords: leave-open

Comment 37

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/451483416eed

status-firefox50: affected → fixed

Target Milestone: --- → mozilla50

Assignee

Comment 38

•

8 years ago

(In reply to Cameron McCormack (:heycam) from comment #28)
> Please add some brief comments in nsCSSValue.h mentioning the different
> kinds of special values we can store in nsCSSTokenStreams.

I went to add this comment but I'm not sure if it's necessary since, for the most part, the special values are encapsulted within KeyframeUtils.

Here's the comment I drafted:

// A string value used primarily to represent variable references.
//
// Animation code, specifically the KeyframeUtils class, also uses this
// type as a container for various string values including:
//
// * Shorthand property values
// * Shorthand sentinel values used for testing failure conditions
// * Invalid longhand property values
//
// For the most part, the above values are not passed to functions that
// manipulate nsCSSValue objects in a generic fashion. Instead KeyframeUtils
// extracts the string from the nsCSSValueTokenStream and passes that around
// instead. The single exception is nsCSSValue::AppendToString which we use
// to serialize the string contained in the nsCSSValueTokenStream by ensuring
// the mShorthandPropertyID is set to eCSSProperty_UNKNOWN.

What do you think?

Comment 39

•

8 years ago

Thanks, I think this sounds good.

Assignee

Comment 40

•

8 years ago

Attached patch part 2 - Add tests for entries array handling when ComputeValues fails — Details — Splinter Review

Updated comment. I'm not planning to land this until part 1 has been applied to beta and aurora branches, however.

Assignee

Updated

•

8 years ago

Attachment #8760615 - Attachment is obsolete: true

Daniel Holbert [:dholbert]

Assignee

Comment 41

•

8 years ago

Liz, I'd like to land this on beta/aurora fairly quickly since the fix that landed on trunk gives some hints about how to exploit this. What do you think? (approval request in comment 35)

Flags: needinfo?(lhenry)

Updated

•

8 years ago

Flags: in-testsuite?

Liz Henry (:lizzard) (relman/hg->git project)

Assignee

Comment 42

•

8 years ago

Closing since it's only tests ad backports that are yet to land (I didn't realize until know that we closed bugs when only tests had yet to land).

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

Keywords: leave-open

Resolution: --- → FIXED

Comment 43

•

8 years ago

Comment on attachment 8760614 [details] [diff] [review]
part 1 - Handle entries arrays where we don't have a property value at the 0.0/1.0 offset

sec-high, looks low risk, ok for aurora and beta uplift.

Flags: needinfo?(lhenry)

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

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

Attachment #8760614 - Flags: approval-mozilla-aurora?

Attachment #8760614 - Flags: approval-mozilla-aurora+

Comment 44

•

8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/ad1604d8bee0

status-firefox49: affected → fixed

Comment 45

•

8 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/5a99e549fb2d

status-firefox48: affected → fixed

Updated

•

8 years ago

Blocks: 1245748

Flags: sec-bounty?

Keywords: regression

Updated

•

8 years ago

Flags: sec-bounty? → sec-bounty+

Updated

•

8 years ago

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

Assignee

Comment 47

•

8 years ago

I believe the fix for this bug has now shipped on all affected branches so it should be safe to open.

Daniel Holbert [:dholbert]

Assignee

Comment 48

•

8 years ago

Pushed test case to inbound because I believe the fix has shipped on all affected branches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb47df5ed6a947150a7c786eab419137fde296c

Flags: in-testsuite? → in-testsuite+

Updated

•

8 years ago

Blocks: 1278463

Comment 49

•

8 years ago

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

Assignee

Comment 50

•

8 years ago

Daniel, I think it's safe to make this bug public now since the fix has shipped on all affected branches.

Flags: needinfo?(dveditz)

Updated

•

8 years ago

Group: core-security-release

Assignee

Updated

•

8 years ago

Flags: needinfo?(dveditz)