963086 - heap-use-after-free (read) at mozilla::PodCopy

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Aki Helin]

Attachment: ff-uaf-podcopy.svg

ASan spots a use after free when the attached SVG is opened in recent Firefox builds.

==6373==ERROR: AddressSanitizer: heap-use-after-free on address 0x7f5af510c800 at pc 0x7f5b1a249232 bp 0x7fff80ad6100 sp 0x7fff80ad60f8
READ of size 2448 at 0x7f5af510c800 thread T0
    #0 0x7f5b1a249231 in void mozilla::PodCopy<int>(int*, int const*, unsigned long) /home/aki/src/mozilla-aurora/objdir-ff-asan/gfx/2d/../../dist/include/mozilla/PodOperations.h:110
    #1 0x7f5b1a24dc24 in mozilla::gfx::GetDataSurfaceInRect(mozilla::gfx::SourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::ConvolveMatrixEdgeMode) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:458
    #2 0x7f5b1a250187 in mozilla::gfx::FilterNodeSoftware::GetInputDataSourceSurface(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::FilterNodeSoftware::FormatHint, mozilla::gfx::ConvolveMatrixEdgeMode, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:721
    #3 0x7f5b1a252cd9 in mozilla::gfx::FilterNodeTransformSoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:978
    #4 0x7f5b1a24cac0 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #5 0x7f5b1a24fcc5 in mozilla::gfx::FilterNodeSoftware::GetInputDataSourceSurface(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::FilterNodeSoftware::FormatHint, mozilla::gfx::ConvolveMatrixEdgeMode, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:691
    #6 0x7f5b1a2599ae in mozilla::gfx::FilterNodeComponentTransferSoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:1570
    #7 0x7f5b1a24cac0 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #8 0x7f5b1a24fcc5 in mozilla::gfx::FilterNodeSoftware::GetInputDataSourceSurface(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::FilterNodeSoftware::FormatHint, mozilla::gfx::ConvolveMatrixEdgeMode, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:691
    #9 0x7f5b1a2606ac in mozilla::gfx::FilterNodeBlurXYSoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:2707
    #10 0x7f5b1a24cac0 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
[...]

Filing as a potential security issue based on bug type.

Comment 1

[Daniel Veditz [:dveditz]]

Aki: your build paths say "mozilla-aurora" -- is this a recent aurora build or does this also crash on trunk? We'll test, of course, but if it's helps you in the future we have tinderbox builds of ASAN mozilla-central available at https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/ which might save you some build time (we also have mozilla-aurora and mozilla-beta asan builds available).

Comment 2

[Daniel Holbert [:dholbert]]

FWIW, I can confirm that the attached testcase (saved locally) aborts with...
==8778==ERROR: AddressSanitizer: heap-use-after-free on address 0x7fea1ca94800 at pc 0x7fea5db1ca86 bp 0x7fffc06505e0 sp 0x7fffc06505d8
READ of size 2432 at 0x7fea1ca94800 thread T0
...in the latest 64-bit linux ASAN build from dveditz's link. (though it doesn't show any symbol information in the backtrace)

Version info:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1391621367/firefox-30.0a1.en-US.linux-x86_64-asan.txt
{
20140205092927
https://hg.mozilla.org/mozilla-central/rev/d2d0899bb3bd
}

I'll do a local ASAN build and dig in a bit more with a debugger later today.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: full backtrace from ASAN

Comment 4

[Daniel Holbert [:dholbert]]

So we end up entering mozilla::gfx::CopyRect with aSrcRect =
{
    x = 0, 
    y = 2147483392, 
    width = 608, 
    height = 1024
}

and when we get to this code...
> 210   uint8_t* sourceData = aSrc->GetData();
[snip]
> 215   sourceData += DataOffset(aSrc, aSrcRect.TopLeft());
http://mxr.mozilla.org/mozilla-central/source/gfx/2d/FilterNodeSoftware.cpp#215

...the DataOffset call returns -622592, so we end up indexing a good deal *before* the beginning of  aSrc->GetData(), and that's probably what gets us into trouble.

Comment 5

[Aki Helin]

Attachment: ff-bofr-podcopy.svg

Comment 6

[Aki Helin]

dveditz: This happens in both for me. Thanks for the tinderbox link though - I didn't remember you also had ASan mozilla-central builds.

dholbert: ASan reports a heap buffer overflow with the new repro file. It likely got confused because the offset was well over red zone size originally.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (reduced)

Here's a more reduced version of "ff-bofr-podcopy.svg". This triggers a heap-buffer-overflow ASAN error in my own ASAN build.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: hackaround

Here's a hackaround, which prevents ASAN aborts in all of the attached testcases.

This is pretty close to where we create the problematic rect, and this is where we attempt to sanity-check it (using "IsEmpty()"), which fails to *sufficiently* sanity-check it.

IsEmpty() checks whether "width" and "height" are > 0, but they don't check whether XMost is larger than x, or YMost is larger than y.  (Those checks should be equivalent to width > 0 and height > 0, but they're not equivalent when we have integer overflow.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8375093 [details] [diff] [review]
hackaround

>+      if (inputFilterOutput.YMost() < 0) {

er, I meant to check if YMost is < inputFilterOutput.y there, not < 0 (just forgot to qref that in before posting).

Doesn't really matter, though, as both versions work, and this isn't an actual final patch.

Comment 10

[Daniel Holbert [:dholbert]]

This FilterNodeSoftware code was added in bug 924102, so this was likely to be a regression from that bug.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: hackaround v2

I think the last hackaround wasn't actually what we wanted.  The fact that the rect's YMost() wraps around is interesting but not the direct problem.

The more proximal issue is that we're allowing DataOffset() to produce negative values, from multiplying a huge "y" value by Stride().

Here's a second hackaround that more directly targets that aspect of the problem; this also fixes all of the attached testcases. However, I'm concerned that DataOffset() might be broken beyond just needing a "negative?" check, as noted in the comment in this patch.  (In particular, if we're multiplying large "y" values and triggering integer overflow, we could end up mapping to an arbitrary *positive* value (just as easily as a negative value), which would also be bad.)

Also, if the negative-check here is correct, then maybe we need it in all the other DataOffset() call-sites...?

mstange, I suspect you're the right person to write an actual patch here, since you understand how all of these layers fit together.  Could you take a look at this?

Comment 12

[Daniel Holbert [:dholbert]]

Assigning to mstange, since I think he's likely the best person to fix this.

Comment 13

[Markus Stange [:mstange]]

Attachment: v1

I think the root problem occurs in GetDataSurfaceInRect, where we do
   IntRect intersect = sourceRect.Intersect(aDestRect);

In the testcase, I get the values

sourceRect: 109, 109, 1083, 1083
aDestRect: 109, 2147483392, 1083, 1536
intersect: 109, 2147483392, 1083, 1536

even though the rects clearly don't intersect. From then on, we try to copy a part from the source surface which is outside of it, and things go bad.

More generally, anywhere we call Intersect or try to do sanity checking with Intersects and Contains, we can't rely on the result if the input rects have overflowing XMost() or YMost(). I'd expect this problem to have come up before, e.g. in layout... how is this usually dealt with?

This patch does several things to address the problem:

 - I'm adding an IntRectOverflows function and bail out early in some methods that deal with rects - especially those that try to Intersect them. First I wanted to add the check only to GetDataSurfaceInRect, but I can't prove that that's sufficient to catch everything, so I've sprinkled the checks over the other methods as well.

 - I'm renaming DataOffset to DataAtOffset to make it easier to use correctly, and to make it clear that the offset should always be inside the surface. And I've added a MOZ_CRASH for when that's not the case.

 - FilterNodeTransformSoftware::SourceRectForOutputRect and FilterNodeTransformSoftware::GetOutputRectInRect were converting a Rect to an IntRect with RoundedToInt() without checking whether the result overflows. I've changed them from RoundedToInt() to .ToIntRect(&intRect) which checks whether the rect's position and dimensions can be represented in both float and int32_t accurately. This check actually is enough to fix the testcase in this bug, but it doesn't address the root problem.

 - CopyRect gets an early exit for when the rect-to-copy is empty. It wouldn't have done any copying in that case before, but it would still have called DataAtOffset, which now has the MOZ_CRASH, and we don't want to hit that.

Comment 14

[Markus Stange [:mstange]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> However, I'm
> concerned that DataOffset() might be broken beyond just needing a
> "negative?" check, as noted in the comment in this patch.  (In particular,
> if we're multiplying large "y" values and triggering integer overflow, we
> could end up mapping to an arbitrary *positive* value (just as easily as a
> negative value), which would also be bad.)

The "point is inside surface" check in my patch should catch all cases here. We ensure during surface and DrawTarget creation that stride * height does not overflow int32_t. This was added in bug 944579.

Comment 15

[Robert Longson [:longsonr]]

regarding

static bool
IntRectOverflows(const IntRect& aRect)
{
  return aRect.XMost() < aRect.x || aRect.YMost() < aRect.y;
}

aRect.XMost() is aRect.x + aRect.width

so an opmimising compiler could turn this into

return aRect.width < 0 || aRect.height < 0

Can we check the offset filter and see if dx > filter space width (or maybe the filter primitive subregion width) then do nothing as the offset goes completely outside the filter and same for dy and height.

If the check goes in the right place then it should catch drop shadows too.

Comment 16

[Markus Stange [:mstange]]

(In reply to Robert Longson from comment #15)
> regarding
> 
> static bool
> IntRectOverflows(const IntRect& aRect)
> {
>   return aRect.XMost() < aRect.x || aRect.YMost() < aRect.y;
> }
> 
> aRect.XMost() is aRect.x + aRect.width
> 
> so an opmimising compiler could turn this into
> 
> return aRect.width < 0 || aRect.height < 0

Oh. Then I'd better use CheckedInt here.

> Can we check the offset filter and see if dx > filter space width (or maybe
> the filter primitive subregion width) then do nothing as the offset goes
> completely outside the filter and same for dy and height.

It's not easy to find the right place for such a check. When we have no overflow, this case is handled by intersecting the needed source rect for the offset (actually transform) FilterNode with the output rect of the input FilterNode, which is usually a crop FilterNode whose output rect is contained in the filter space. If this intersection is empty, we know that the offset filter result won't be painted.

Hmm, thinking about that, I think I didn't identify the real cause of the problem before. The Intersect() call I mention at the beginning of comment 13 shouldn't even be hit because already have noticed by then that we don't need anything from the input surface... I'll check.

> If the check goes in the right place then it should catch drop shadows too.

The check in this patch should catch that.

But, now that I think about it again, we may need similar checks in FilterSupport.cpp when we calculate the output extents, for example.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8375486 [details] [diff] [review]
v1

>+static uint8_t*
>+DataAtOffset(DataSourceSurface* aSurface, IntPoint aPoint)
> {
>-  return aPoint.y * aSurface->Stride() +
>-         aPoint.x * BytesPerPixel(aSurface->GetFormat());
>+  if (!IntRect(IntPoint(), aSurface->GetSize()).Contains(aPoint)) {
>+    MOZ_CRASH("sample position needs to be inside surface!");
>+  }

This is better, but unfortunately not foolproof -- Contains() isn't a robust way to check for overflow, because it fails on edge-cases.

In particular, the "point" version of Contains() appears to fail if aPoint.x or aPoint.y are INT_MAX, which overflows to INT_MIN if we add one to it.  *Any* rect will report itself as containing such a point. This is because Contains() does its check like so:
 x <= aX && aX + 1 <= XMost()
which in this case would translate to x <= INT_MAX && INT_MIN <= XMost(), which is trivially true for any x / XMost().
Reference: http://mxr.mozilla.org/mozilla-central/source/gfx/2d/BaseRect.h#78

>+  return aSurface->GetData() + aPoint.y * aSurface->Stride() +
>+    aPoint.x * BytesPerPixel(aSurface->GetFormat());
>+}

I think we can run into trouble here if aPoint.x or aPoint.y are a sufficiently large fraction of INT_MAX, because then aPoint.x * Stride() will overflow, and/or aPoint.y * BytesPerPixel will overflow.  Could you add checks to handle that? Or, if we have somehow already guaranteed that this won't be a problem, could you add an explicit assertion to document that? (e.g. assert that INT32_MAX / aSurface.Stride() <= aPoint.x, or somesuch)


Also: The offset we're adding to GetData() here is expected to be non-negative, yes? (i.e. we're expecting to look at memory *after* the passed-in pointer, not before)

If so, could you store the offset in a local variable and sanity-check (with an assertion at least) that it's >=0?  If you include that in your bail-out MOZ_CRASH case, it might catch this sort of thing more robustly...  Though I suppose an optimizing compiler could also neuter that check, so maybe we we need to be a bit more careful about that check. (Or we could be less careful, as long as we do a final sanity-check to make sure we're in-bounds before we actually use this data-pointer.) 


>+static bool
>+IntRectOverflows(const IntRect& aRect)
>+{
>+  return aRect.XMost() < aRect.x || aRect.YMost() < aRect.y;
> }

As Robert says in comment 15, compilers can optimize this function to be useless, unfortunately.

I think the more robust way to check this would be e.g.
  "INT32_MAX - aRect.width <= aRect.x"

That doesn't depend on any particular overflow behavior, so compilers can't optimize it away.

>+  uint8_t* sourceData = DataAtOffset(aSrc, aSrcRect.TopLeft());
>   uint32_t sourceStride = aSrc->Stride();
>-  uint8_t* destData = aDest->GetData();
>+  uint8_t* destData = DataAtOffset(aDest, aDestPoint);
>   uint32_t destStride = aDest->Stride();

Might be worth asserting that sourceData >= aSrc and destData >= aDest here?

(If you add the offset >= 0 assertion that I suggested above, it'd be a bit redundant, but possibly still worth it, for an abundance of caution & making your assumptions super-explicit. Up to you, though.)

> static void
> FillRectWithPixel(DataSourceSurface *aSurface, const IntRect &aFillRect, IntPoint aPixelPos)
> {
>-  uint8_t* data = aSurface->GetData();
>-  uint8_t* sourcePixelData = data + DataOffset(aSurface, aPixelPos);
>+  MOZ_ASSERT(IntRect(IntPoint(), aSurface->GetSize()).Contains(aFillRect),
>+             "aFillRect needs to be completely inside the surface");
>+  MOZ_ASSERT(IntRect(IntPoint(), aSurface->GetSize()).Contains(IntRect(aPixelPos, IntSize(1, 1))),
>+             "aPixelPos needs to be inside the surface");

As above, note that Contains() isn't robust, and in fact the version that takes a Rect as an argument (the one called here) is much less robust than the version that takes a point as an argument (the one discussed above). That's because in the rect version, x or y don't have to be *exactly* at INT_MAX; you just need x + width or y + height to overflow, and then they'll behave as if they're contained by every valid rect.

However, if you have an improved IntRectOverflows() method, then you could include that in these assertions to handle this. (or assert about that in a separate assertion, just before these ones). Probably worth doing that.

(Same goes for other Contains()-based assertions in the patch).

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Markus Stange [:mstange] from comment #16)
> (In reply to Robert Longson from comment #15)
> Oh. Then I'd better use CheckedInt here.

(Actually, that's a good point; you can probably use CheckedInt (or several CheckedInt values) to compute the offset in DataAtOffset(), actually. Still worth asserting that the Stride() and BytesPerPixel() multiplications aren't going to overflow, though.)

Comment 19

[Markus Stange [:mstange]]

Attachment: v2

Thanks for the very thorough review, Daniel!

(In reply to Daniel Holbert [:dholbert] from comment #17)
> Comment on attachment 8375486 [details] [diff] [review]
> v1
> 
> >+static uint8_t*
> >+DataAtOffset(DataSourceSurface* aSurface, IntPoint aPoint)
> > {
> >-  return aPoint.y * aSurface->Stride() +
> >-         aPoint.x * BytesPerPixel(aSurface->GetFormat());
> >+  if (!IntRect(IntPoint(), aSurface->GetSize()).Contains(aPoint)) {
> >+    MOZ_CRASH("sample position needs to be inside surface!");
> >+  }
> 
> This is better, but unfortunately not foolproof -- Contains() isn't a robust
> way to check for overflow, because it fails on edge-cases.

I've added a SurfaceContainsPoint helper function that has no overflow problems.

> >+  return aSurface->GetData() + aPoint.y * aSurface->Stride() +
> >+    aPoint.x * BytesPerPixel(aSurface->GetFormat());
> >+}
> 
> I think we can run into trouble here if aPoint.x or aPoint.y are a
> sufficiently large fraction of INT_MAX, because then aPoint.y * Stride()
> will overflow, and/or aPoint.x * BytesPerPixel will overflow.

aPoint.x is < surfaceWidth, and surfaceWidth * BytesPerPixel is <= Stride().
aPoint.y is < surfaceHeight, and the fact that surfaceHeight * Stride() does not overflow is ensured in Factory::CheckSurfaceSize. And if the allocation of the surface succeeded, adding GetData() and something < allocationSize can't overflow either.

>  Could you add
> checks to handle that? Or, if we have somehow already guaranteed that this
> won't be a problem, could you add an explicit assertion to document that?
> (e.g. assert that INT32_MAX / aSurface.Stride() <= aPoint.x, or somesuch)

I've added a MOZ_ASSERT(Factory::CheckSurfaceSize(aSurface->GetSize()));.

> Also: The offset we're adding to GetData() here is expected to be
> non-negative, yes? i.e. we're expecting to look at memory *after* the
> passed-in pointer, not before)

Hmm. Theoretically, DataSourceSurfaces could have its data stored upside down, with GetData() pointing to the beginning of the last row of the surface, and a Stride() being negative... though I've never seen that in practice.

> If so, could you store the offset in a local variable and sanity-check (with
> an assertion at least) that it's >=0? If you include that in your bail-out
> MOZ_CRASH case, it might catch this sort of thing more robustly...  Though I
> suppose an optimizing compiler could also neuter that check, so maybe we we
> need to be a bit more careful about that check. (Or we could be less
> careful, as long as we do a final sanity-check to make sure we're in-bounds
> before we actually use this data-pointer.)

I've added the dataAtOffset >= data assertion you're asking for below. (Even though that will break for negative stride surfaces, but at least we then know if somebody tries them on us.)
With these assertions, do you still think we need should have the >= 0 one?

> >+static bool
> >+IntRectOverflows(const IntRect& aRect)
> >+{
> >+  return aRect.XMost() < aRect.x || aRect.YMost() < aRect.y;
> > }
> 
> As Robert says in comment 15, compilers can optimize this function to be
> useless, unfortunately.
> 
> I think the more robust way to check this would be e.g.
>   "INT32_MAX - aRect.width <= aRect.x"
> 
> That doesn't depend on any particular overflow behavior, so compilers can't
> optimize it away.

I've changed it to use CheckedInt instead.

> >+  uint8_t* sourceData = DataAtOffset(aSrc, aSrcRect.TopLeft());
> >   uint32_t sourceStride = aSrc->Stride();
> >-  uint8_t* destData = aDest->GetData();
> >+  uint8_t* destData = DataAtOffset(aDest, aDestPoint);
> >   uint32_t destStride = aDest->Stride();
> 
> Might be worth asserting that sourceData >= aSrc and destData >= aDest here?

We'd have to do this in a lot of places if we want to be consistent... I'd rather keep just the >= assertion in DataAtOffset.

> > static void
> > FillRectWithPixel(DataSourceSurface *aSurface, const IntRect &aFillRect, IntPoint aPixelPos)
> > {
> >-  uint8_t* data = aSurface->GetData();
> >-  uint8_t* sourcePixelData = data + DataOffset(aSurface, aPixelPos);
> >+  MOZ_ASSERT(IntRect(IntPoint(), aSurface->GetSize()).Contains(aFillRect),
> >+             "aFillRect needs to be completely inside the surface");
> >+  MOZ_ASSERT(IntRect(IntPoint(), aSurface->GetSize()).Contains(IntRect(aPixelPos, IntSize(1, 1))),
> >+             "aPixelPos needs to be inside the surface");
> 
> As above, note that Contains() isn't robust, and in fact the version that
> takes a Rect as an argument (the one called here) is much less robust than
> the version that takes a point as an argument (the one discussed above).
> That's because in the rect version, x or y don't have to be *exactly* at
> INT_MAX; you just need x + width or y + height to overflow, and then they'll
> behave as if they're contained by every valid rect.
> 
> However, if you have an improved IntRectOverflows() method, then you could
> include that in these assertions to handle this. (or assert about that in a
> separate assertion, just before these ones).

I did the latter.

Comment 20

[Markus Stange [:mstange]]

Attachment: v3

Oops, forgot to qrefresh the DataAtOffset >= assertion into the patch.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Markus Stange [:mstange] from comment #19)
> > I think we can run into trouble here if aPoint.x or aPoint.y are a
> > sufficiently large fraction of INT_MAX, because then aPoint.y * Stride()
> > will overflow, and/or aPoint.x * BytesPerPixel will overflow.
> 
> aPoint.x is < surfaceWidth, and surfaceWidth * BytesPerPixel is <= Stride().
> aPoint.y is < surfaceHeight, and the fact that surfaceHeight * Stride() does
> not overflow is ensured in Factory::CheckSurfaceSize. And if the allocation
> of the surface succeeded, adding GetData() and something < allocationSize
> can't overflow either.

Gotcha. So the SurfaceContainsPoint(aPoint) & Factory::CheckSurfaceSize() calls (combined) save us.

Thanks for the explanation!

> Hmm. Theoretically, DataSourceSurfaces could have its data stored upside
> down, with GetData() pointing to the beginning of the last row of the
> surface, and a Stride() being negative... though I've never seen that in
> practice.

Hmm, yeah -- http://mxr.mozilla.org/mozilla-central/source/gfx/2d/2D.h#365 says Stride() "may be negative", so it may be problematic that we're now depending on it being positive here...

I don't know this code to be sure *how* problematic it is, though (or how many other things that would break, which would make it unlikely).

Note also that the Stride() documentation says "[DEPRECATED]" -- is there another method we should be using instead here?  (Same goes for GetData(). Maybe it's just deprecated for external-to-gfx stuff?)

> I've added the dataAtOffset >= data assertion you're asking for below. (Even
> though that will break for negative stride surfaces, but at least we then
> know if somebody tries them on us.)
> With these assertions, do you still think we need should have the >= 0 one?

The >= data assertion is sufficient -- thanks.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8377843 [details] [diff] [review]
v3

Morphing review? into feedback+; I don't know this code well enough to feel comfortable r+'ing it. Feel free to carry forward Bas's review, or re-tag him for review, as you see fit. :)

Having said that, here's my feedback:

>+static uint8_t*
>+DataAtOffset(DataSourceSurface* aSurface, IntPoint aPoint)
>+{
>+  if (!SurfaceContainsPoint(aSurface, aPoint)) {
>+    MOZ_CRASH("sample position needs to be inside surface!");
>+  }
>+  MOZ_ASSERT(Factory::CheckSurfaceSize(aSurface->GetSize()));
>+
>+  uint8_t* data = aSurface->GetData() + aPoint.y * aSurface->Stride() +
>+    aPoint.x * BytesPerPixel(aSurface->GetFormat());
>+  MOZ_ASSERT(data >= aSurface->GetData());
>+  return data;
>+}

I'm not clear on why that first condition is MOZ_CRASH-worthy (i.e. abort fatally in opt builds), whereas the other two are MOZ_ASSERTs (debug-only).

Perhaps the first check really wants to be MOZ_ASSERT? (unless it's significantly more likely and/or more evil than the other two conditions.) (I don't know how to gauge their likelihood, though they all look about the same level of evilness to me.)

> static void
> CopyRect(DataSourceSurface* aSrc, DataSourceSurface* aDest,
>          IntRect aSrcRect, IntPoint aDestPoint)
> {
>+  if (IntRectOverflows(aSrcRect) ||
>+      IntRectOverflows(IntRect(aDestPoint, aSrcRect.Size()))) {
>+    MOZ_CRASH("we should never be getting invalid rects at this point");
>+  }

Per comment 4, will the testcase now trigger this ^^ (safe) crash? Or will we bail out earlier (ideally in a non-crashy way)?

Also: hypothetically, if it is possible to hit that case, is MOZ_CRASH()'ing release builds really necessary, or would a early-return be better? (with an assertion, to catch this in debug builds)  It looks like we have a conditional early return right after this (for a different condition), so returning seems to be something we could do here without causing trouble; hence, maybe crashing is overkill.

> static void
> FillRectWithPixel(DataSourceSurface *aSurface, const IntRect &aFillRect, IntPoint aPixelPos)
> {
>-  uint8_t* data = aSurface->GetData();
>-  uint8_t* sourcePixelData = data + DataOffset(aSurface, aPixelPos);
>+  MOZ_ASSERT(!IntRectOverflows(aFillRect));
>+  MOZ_ASSERT(IntRect(IntPoint(), aSurface->GetSize()).Contains(aFillRect),
>+             "aFillRect needs to be completely inside the surface");

Might be worth combining those two assertions, since the first one is really part of checking whether aFillRect is completely inside the surface?

i.e. 
  MOZ_ASSERT(!IntRectOverflows(aFillRect) &&
             IntRect(IntPoint(), aSurface->GetSize()).Contains(aFillRect),
             "aFillRect needs to be completely inside the surface");

(makes more sense to me, but it's up to you)

(Same goes for these same assertions in FillRectWithVerticallyRepeatingHorizontalStrip, FillRectWithHorizontallyRepeatingVerticalStrip, and DuplicateEdges)

> static TemporaryRef<DataSourceSurface>
> GetDataSurfaceInRect(SourceSurface *aSurface,
>                      const IntRect &aSurfaceRect,
>                      const IntRect &aDestRect,
>                      ConvolveMatrixEdgeMode aEdgeMode)
> {
>   MOZ_ASSERT(aSurface ? aSurfaceRect.Size() == aSurface->GetSize() : aSurfaceRect.IsEmpty());
>+
>+  if (IntRectOverflows(aSurfaceRect) || IntRectOverflows(aDestRect)) {
>+    // We can't rely on the intersection calculations below to make sense when
>+    // XMost() and YMost() overflow. Bail out.
>+    return nullptr;
>+  }

nit: s/XMost() and YMost()/XMost() or YMost()/

> TemporaryRef<DataSourceSurface>
> FilterNodeSoftware::GetOutput(const IntRect &aRect)
> {
>   MOZ_ASSERT(GetOutputRectInRect(aRect).Contains(aRect));
>+
>+  if (IntRectOverflows(aRect)) {
>+    return nullptr;
>+  }

(and similar early-returns in other functions after this one in the patch):
I don't know this code well enough to be sure why these specific functions need runtime checks (& early-returns) for IntRectOverflows(aRect), but I'll take your word for it.   (But if we can get away with just checking in one top-level function and then having other functions rely on that (maybe with assertions as sanity-check), that might be better.)

>@@ -1026,18 +1122,22 @@ IntRect
> FilterNodeTransformSoftware::GetOutputRectInRect(const IntRect& aRect)
> {
>   IntRect srcRect = SourceRectForOutputRect(aRect);
>   if (srcRect.IsEmpty()) {
>     return IntRect();
>   }
> 
>   Rect outRect = mMatrix.TransformBounds(Rect(srcRect));
>-  outRect.RoundOut();
>-  return RoundedToInt(outRect).Intersect(aRect);
>+  outRect.RoundOut(); 
                       ^-------------.
You added end-of-line whitespace here-`. Revert that.

>@@ -1086,39 +1186,36 @@ ApplyMorphology(const IntRect& aSourceRe
>+    uint8_t* sourceData = DataAtOffset(aInput, destRect.TopLeft() - srcRect.TopLeft());
> 
>     int32_t tmpStride = tmp->Stride();
>-    uint8_t* tmpData = tmp->GetData();
>-    tmpData += DataOffset(tmp, destRect.TopLeft() - tmpRect.TopLeft());
>+    uint8_t* tmpData = DataAtOffset(tmp, destRect.TopLeft() - tmpRect.TopLeft());

(Observation: the math with destRect.TopLeft() here is a little interesting, since it looks to me like that is guaranteed to be 0,0. At least, destRect is declared in this function as:
  IntRect destRect = aDestRect - aDestRect.TopLeft();
which I think (?) puts its top-left corner at 0,0.  And that's odd, because that means we're calling DataOffset with a negative-territory point (0,0- tmpRect.TopLeft()), which I'd expect to cause trouble...

I'm probably misunderstanding something, though.

>-    tmpData += DataOffset(tmp, destRect.TopLeft() - tmpRect.TopLeft());
>+    uint8_t* tmpData = DataAtOffset(tmp, destRect.TopLeft() - tmpRect.TopLeft());

(above comment applies here, too -- a bit later in the same function)

Comment 23

[Andrew McCreight [:mccr8]]

Markus, it looks like this is ready for review.  We're pretty late in the beta cycle, so this will need to move along if it is going to make it there.  Thanks.

Comment 24

[Markus Stange [:mstange]]

Attachment: v4, f=dholbert, r=Bas

Fixed the end-of-line whitespace.

Comment 25

[Markus Stange [:mstange]]

Attachment: v5, f=dholbert, r=Bas

Changes compared to v4: Turned the data >= aSurface->GetData() check from MOZ_ASSERT into MOZ_CRASH, and added attachment 8371288 [details] as a crashtest.

(In reply to Daniel Holbert [:dholbert] from comment #22)
> Comment on attachment 8377843 [details] [diff] [review]
> v3
> 
> Morphing review? into feedback+; I don't know this code well enough to feel
> comfortable r+'ing it. Feel free to carry forward Bas's review, or re-tag
> him for review, as you see fit. :)

I carried forward Bas's review.

> Having said that, here's my feedback:
> 
> >+static uint8_t*
> >+DataAtOffset(DataSourceSurface* aSurface, IntPoint aPoint)
> >+{
> >+  if (!SurfaceContainsPoint(aSurface, aPoint)) {
> >+    MOZ_CRASH("sample position needs to be inside surface!");
> >+  }
> >+  MOZ_ASSERT(Factory::CheckSurfaceSize(aSurface->GetSize()));
> >+
> >+  uint8_t* data = aSurface->GetData() + aPoint.y * aSurface->Stride() +
> >+    aPoint.x * BytesPerPixel(aSurface->GetFormat());
> >+  MOZ_ASSERT(data >= aSurface->GetData());
> >+  return data;
> >+}
> 
> I'm not clear on why that first condition is MOZ_CRASH-worthy (i.e. abort
> fatally in opt builds), whereas the other two are MOZ_ASSERTs (debug-only).
> 
> Perhaps the first check really wants to be MOZ_ASSERT? (unless it's
> significantly more likely and/or more evil than the other two conditions.)

The main reason I made the first check MOZ_CRASH is that it's really cheap, and covers the most likely case of things going wrong. I don't expect content to be able to trigger it, but in case somebody manages, we still have it as a layer of protection. In this v5 patch I've also turned the data >= aSurface->GetData() check into a MOZ_CRASH, again because it is dirt cheap. The Factory::CheckSurfaceSize check, however, is not that cheap, so I've left it as MOZ_ASSERT.

> > static void
> > CopyRect(DataSourceSurface* aSrc, DataSourceSurface* aDest,
> >          IntRect aSrcRect, IntPoint aDestPoint)
> > {
> >+  if (IntRectOverflows(aSrcRect) ||
> >+      IntRectOverflows(IntRect(aDestPoint, aSrcRect.Size()))) {
> >+    MOZ_CRASH("we should never be getting invalid rects at this point");
> >+  }
> 
> Per comment 4, will the testcase now trigger this ^^ (safe) crash? Or will
> we bail out earlier (ideally in a non-crashy way)?

The testcase will already bail out in the "if (!neededRect.ToIntRect...)" check in FilterNodeTransformSoftware::GetOutputRectInRect because the offset rect is located at a position that can't be expressed in floats. So no crash.
There are other testcases imaginable that wouldn't fail this particular check, but for those we'll bail out from one of the other non-crashy IntRectOverflows() checks. I've only added MOZ_CRASH in those cases that should never be hit, because we should already have bailed out with a non-crashy check.

> > static void
> > FillRectWithPixel(DataSourceSurface *aSurface, const IntRect &aFillRect, IntPoint aPixelPos)
> > {
> >-  uint8_t* data = aSurface->GetData();
> >-  uint8_t* sourcePixelData = data + DataOffset(aSurface, aPixelPos);
> >+  MOZ_ASSERT(!IntRectOverflows(aFillRect));
> >+  MOZ_ASSERT(IntRect(IntPoint(), aSurface->GetSize()).Contains(aFillRect),
> >+             "aFillRect needs to be completely inside the surface");
> 
> Might be worth combining those two assertions, since the first one is really
> part of checking whether aFillRect is completely inside the surface?
> 
> i.e. 
>   MOZ_ASSERT(!IntRectOverflows(aFillRect) &&
>              IntRect(IntPoint(), aSurface->GetSize()).Contains(aFillRect),
>              "aFillRect needs to be completely inside the surface");
> 
> (makes more sense to me, but it's up to you)

I've left them separate. I see non-overflowing rects as a separate pre-condition to this function. Also, with a combined assertion you'd have to do slightly more work to find out which part failed in case the assertion fails.

> > static TemporaryRef<DataSourceSurface>
> > GetDataSurfaceInRect(SourceSurface *aSurface,
> >                      const IntRect &aSurfaceRect,
> >                      const IntRect &aDestRect,
> >                      ConvolveMatrixEdgeMode aEdgeMode)
> > {
> >   MOZ_ASSERT(aSurface ? aSurfaceRect.Size() == aSurface->GetSize() : aSurfaceRect.IsEmpty());
> >+
> >+  if (IntRectOverflows(aSurfaceRect) || IntRectOverflows(aDestRect)) {
> >+    // We can't rely on the intersection calculations below to make sense when
> >+    // XMost() and YMost() overflow. Bail out.
> >+    return nullptr;
> >+  }
> 
> nit: s/XMost() and YMost()/XMost() or YMost()/

fixed

> > TemporaryRef<DataSourceSurface>
> > FilterNodeSoftware::GetOutput(const IntRect &aRect)
> > {
> >   MOZ_ASSERT(GetOutputRectInRect(aRect).Contains(aRect));
> >+
> >+  if (IntRectOverflows(aRect)) {
> >+    return nullptr;
> >+  }
> 
> (and similar early-returns in other functions after this one in the patch):
> I don't know this code well enough to be sure why these specific functions
> need runtime checks (& early-returns) for IntRectOverflows(aRect), but I'll
> take your word for it.   (But if we can get away with just checking in one
> top-level function and then having other functions rely on that (maybe with
> assertions as sanity-check), that might be better.)

Yeah, I'm not confident enough in some of those to make them debug-only. I'd rather have one check too many.

> >@@ -1026,18 +1122,22 @@ IntRect
> > FilterNodeTransformSoftware::GetOutputRectInRect(const IntRect& aRect)
> > {
> >   IntRect srcRect = SourceRectForOutputRect(aRect);
> >   if (srcRect.IsEmpty()) {
> >     return IntRect();
> >   }
> > 
> >   Rect outRect = mMatrix.TransformBounds(Rect(srcRect));
> >-  outRect.RoundOut();
> >-  return RoundedToInt(outRect).Intersect(aRect);
> >+  outRect.RoundOut(); 
>                        ^-------------.
> You added end-of-line whitespace here-`. Revert that.

fixed

> >@@ -1086,39 +1186,36 @@ ApplyMorphology(const IntRect& aSourceRe
> >+    uint8_t* sourceData = DataAtOffset(aInput, destRect.TopLeft() - srcRect.TopLeft());
> > 
> >     int32_t tmpStride = tmp->Stride();
> >-    uint8_t* tmpData = tmp->GetData();
> >-    tmpData += DataOffset(tmp, destRect.TopLeft() - tmpRect.TopLeft());
> >+    uint8_t* tmpData = DataAtOffset(tmp, destRect.TopLeft() - tmpRect.TopLeft());
> 
> (Observation: the math with destRect.TopLeft() here is a little interesting,
> since it looks to me like that is guaranteed to be 0,0.

Indeed. I could remove "destRect.TopLeft()" and just have "-tmpRect.TopLeft()" there, but I think it's clearer this way.

> And that's odd, because
> that means we're calling DataOffset with a negative-territory point (0,0-
> tmpRect.TopLeft())

tmpRect.TopLeft() is negative (or zero), so we're good.
The idea here is more obvious when you see the two DataAtOffset calls next to each other:

    uint8_t* sourceData = DataAtOffset(aInput, destRect.TopLeft() - srcRect.TopLeft());
    uint8_t* tmpData = DataAtOffset(tmp, destRect.TopLeft() - tmpRect.TopLeft());

destRect, srcRect and tmpRect are all in the same coordinate system. destRect.TopLeft() is always (0,0), but the calculations don't rely on that.
Both want a pointer to the same position in this coordinate system (destRect.TopLeft()), but their surfaces are located at different positions (srcRect.TopLeft() and tmpRect.TopLeft()), so the data is offset by the difference between these positions.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8385253 [details] [diff] [review]
v5, f=dholbert, r=Bas

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Tricky, but doable. The patch shows that huge offset filters can be used to create overflowing IntRects, and that those can cause us to access data outside the image surface. Accessing the data you want and getting it back in a way the web page can read from it would need a lot of work, but is possible, as far as I can tell.

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

Which older supported branches are affected by this flaw?
28 and 29.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply as-is on all affected branches.

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

Comment 27

[Markus Stange [:mstange]]

Comment on attachment 8385253 [details] [diff] [review]
v5, f=dholbert, r=Bas

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 924102 / bug 924103
User impact if declined: security vulnerability
Testing completed (on m-c, etc.): only local testing
Risk to taking this patch (and alternatives if risky): low, it's mostly additional safety checks
String or IDL/UUID changes made by this patch: none

Comment 28

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

Comment on attachment 8385253 [details] [diff] [review]
v5, f=dholbert, r=Bas

sec-approval+ and branch approval. Please don't bork the beta builds. It should be green on trunk before going in anywhere else.

Comment 29

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2441328846

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/5d2441328846

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a0eed30f6cfe
https://hg.mozilla.org/releases/mozilla-beta/rev/e3705eb34322

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e3705eb34322