1258230 - crash in compute_image_info, called from _moz_pixman_image_composite32

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

This bug was filed from the Socorro interface and is 
report bp-87052bfb-8687-4fd6-b74f-591892160313.
=============================================================

Note: this is a UAF with 700 UAF crashes in the last week

Comment 1

[Bas Schouten (:bas.schouten)]

Some of these actually are null pointer dereferences (see for example https://crash-stats.mozilla.com/report/index/e3c2e308-6d16-4868-a61b-d56b52160329).

Some of these seem to be UAF (As the one posted in the initial comment)

Some of these seem to be utterly random addresses (https://crash-stats.mozilla.com/report/index/54eda41b-ebb8-4da9-9ed5-802d12160329)

The stacks tend to be slightly different for most of them but I've failed to find one with a useful stack. It appears this bug is probably lumping several issues together.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

The last (random) crash looks text-ish, but more to the point the crash location implies (but doesn't prove, especially with optimizers!) that the image got freed/reallocated while this code was running.  I generally look at "random" addresses as barely "better" than clear UAFs (e5e5, etc).  The nulls can be the same thing, if they're not consistent.

I agree there may well be multiple sources here.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Peter, can you get somebody to dig a bit into this?

Comment 4

[Peter Chang[:pchang]]

Take this bug to check more detail.

Comment 5

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

Any updates here? It has been two months and this is a "high" rated security bug.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

~4500 UAF crashes in the last month.  This is really bad both for uptime *and* security.  IMHO (and I can be overridden here) this is sec-crit, given the wide exposure of a UAF signature, implying it's easy to provoke (compared to many UAFs)

https://crash-stats.mozilla.com/signature/?date=%3E%3D2016-05-12T22%3A41%3A16.394747&date=%3C2016-06-09T22%3A41%3A16.394747&signature=compute_image_info&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#reports

NI to Milan to vet the severity change

Comment 7

[Peter Chang[:pchang]]

I'm investigating this issue now.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll keep needinfo on me, but Peter is looking at this.

Comment 9

[Peter Chang[:pchang]]

Based on user dump, the transform of drawtarget got changed and was crashed at [1].

Checking the places that modify the image->common.transform.
[1]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-image.c#284

Comment 10

[Peter Chang[:pchang]]

After checking several minidumps, I saw the pixel_image that caused problem was created from _pixman_image_for_solid. Inside pixman_image_for_solid, there is one preprocessor option 'PIXMAN_HAS_ATOMIC_OPS'. It looks like we doesn't enable this option in windows because the compiler didn't blame me when I miss ';' between the scope of this option.

Bas, do we enable the 'PIXMAN_HAS_ATOMIC_OPS' option for FF on windows?

If we don't enable this option, it always creates a new pixel_image_for_solid with NULL transform[3]. But I don't understand why this solid pixel_image contain the transform with incorrect address[2].

[1]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-image-surface.c#1685 
[2]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-image.c#284
[3]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-solid-fill.c#55

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Peter Chang[:pchang] from comment #10)
> After checking several minidumps, I saw the pixel_image that caused problem
> was created from _pixman_image_for_solid. Inside pixman_image_for_solid,
> there is one preprocessor option 'PIXMAN_HAS_ATOMIC_OPS'. It looks like we
> doesn't enable this option in windows because the compiler didn't blame me
> when I miss ';' between the scope of this option.
> 
> Bas, do we enable the 'PIXMAN_HAS_ATOMIC_OPS' option for FF on windows?
> 
> If we don't enable this option, it always creates a new
> pixel_image_for_solid with NULL transform[3]. But I don't understand why
> this solid pixel_image contain the transform with incorrect address[2].
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> image-surface.c#1685 
> [2]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/
> pixman-image.c#284
> [3]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/
> pixman-solid-fill.c#55

The address you're looking at is an offset from e5e5e5e5, this is jemalloc's magic number for freed memory, i.e. the image has been destroyed since it was created, that is why you get the incorrect address.

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Peter, back in your court now. Any ideas?

Comment 13

[Peter Chang[:pchang]]

(In reply to Peter Chang[:pchang] from comment #10)
> After checking several minidumps, I saw the pixel_image that caused problem
> was created from _pixman_image_for_solid. Inside pixman_image_for_solid,
> Bas, do we enable the 'PIXMAN_HAS_ATOMIC_OPS' option for FF on windows?
> 

I just confirmed that we didn't enable this option on windows.

Comment 14

[Peter Chang[:pchang]]

For the random crash address, I think Bug 1277122[1] should help some of them.
I'm working on the patch for UAF case.

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

Comment 15

[Peter Chang[:pchang]]

Attachment: Skip transform configuration for solid pixel_image

Jeff, about the UAF case, I always see we got problem for solid pixel_image from user dumps.

During image composition, gecko creates a pixel_image with solid type. And the transform property of solid pixel_image should be NULL by default[2].

Therefore, it shouldn't have value about pixel_image->common->transform, but we got UAF when checking transform data. Inside pixman, I only saw tranform was changed inside pixman_image_set_transform. Therefore, I skip transform configuration when image type is solid. How do you think? After we land this patch, we could monitor the UAF number for this crash again.

[1]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-image-surface.c#4293
[2]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-solid-fill.c#53

Comment 16

[Peter Chang[:pchang]]

(In reply to Bas Schouten (:bas.schouten) from comment #1)
> Some of these actually are null pointer dereferences (see for example
> https://crash-stats.mozilla.com/report/index/e3c2e308-6d16-4868-a61b-
> d56b52160329).
> 
Put the call stack of UAF case.
https://pastebin.mozilla.org/8889758

> Some of these seem to be UAF (As the one posted in the initial comment)
>

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8778187 [details] [diff] [review]
Skip transform configuration for solid pixel_image

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

I'd like to better understand what's going on before taking a bandaid like this.

Comment 18

[Peter Chang[:pchang]]

I did the carsh search today but there is no UAF case after FF 48. But there are two crashes at random address after FF49.

https://crash-stats.mozilla.com/signature/?product=Firefox&signature=compute_image_info#aggregations

Comment 19

[Peter Chang[:pchang]]

Is this still security high issue? 

I didn't see too much crash after 48, and bug 1277122 should help this crash. Now I'm working on bug 1293598 to avoid potential "dereferencing freed memory".

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Crashes are definitely lower. I see 2 49.b2 crashes in the last week, so I'm not quite ready to say we're not seeing it anymore.  How often it happens isn't always the driver for severity, though, since that doesn't tell you if it's possible to force with a specific testcase.

Comment 21

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

Randall, Milan, can you re-rate the severity of the remaining crashes now that we are in beta 6?  We still may want to fix this issue (though not necessarily uplift to beta if it isn't critical).

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Looks like 6 crashes in 49b since Aug 7th.  Some are 0x0's, others small numbers, 3 are scary addresses though not e5e5

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

https://crash-stats.mozilla.com/signature/?version=51.0a1&version=50.0a2&version=49.0b&version=50.0a1&date=%3E2016-07-01&signature=compute_image_info&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports

Comment 24

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

Wontfix for 49 at this point as we release next week.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Still seeing 2-4 per week, not high frequency but still definitely a sec issue.  If someone were to figure out how to provoke it this might be exploitable (given the non-null-deref addresses, and one crash being a PRIV_INSTRUCTION crash with a non-nullptr address).  Obviously much better than before.  I suspect there are either 2 bugs in this region/pattern, and we solved one, or the fix has slightly imperfect and can lead to rare crashes (but not as clearly UAFs).

Comment 26

[Peter Chang[:pchang]]

For the rare crashes, I think bug 1293598 can help or reduce the trouble when dereferencing freed memory.

Comment 27

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

I don't feel the need to track this. I'd be happy to take a beta50 uplift if a fix is ready soon.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

1-2 reports/day.
One this week is an e5e5: https://crash-stats.mozilla.com/report/index/2d33f09a-a110-4ca2-a0e5-1e4b92160919

Comment 29

[Matt Wobensmith [:mwobensmith][:matt:]]

CritSmash consensus is to mark as incomplete due to severely reduced frequency of crashes, as well as the lack of current actionability due to missing details.

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

I (respectively) disagree that a sec-crit happening a few times a week should be closed because we haven't found the source of those remaining crashes - especially if no one has tried looking for the remaining crashes.  If you want to clone the bug and handle the remainder there, fine, but a UAF is a potential exploit - it might be 100% if you figure out the sequence to tickle it.

Comment 31

[Peter Chang[:pchang]]

I just landed bug 1293598, and I think we could have more clues when we get the crash next time.

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Peter Chang[:pchang] from comment #31)
> I just landed bug 1293598, and I think we could have more clues when we get
> the crash next time.

Good, though we'll likely have to wait until beta or release unless we're lucky.  We should uplift that patch.

Matt: please reopen or clone

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

Happy to reopen. We'll continue to track.

Comment 34

[Milan Sreckovic [:milan] (needinfo for best results)]

I can't see any crashes in 50 or later, but most of the "fixes" were done only starting in 51.

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Yup.  Crashes in 49.*, none in 50/51/52 that I see

Comment 36

[Andrew McCreight [:mccr8]]

(In reply to Randell Jesup [:jesup] from comment #35)
> Yup.  Crashes in 49.*, none in 50/51/52 that I see

Should we close this now?

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

We have 1 hit in 50b6:
https://crash-stats.mozilla.com/report/index/74b5ffb9-2db9-486c-89b9-a05d32161014
while the line differs, and it's an illegal instruction, but if you look at the 49 crashes they do vary anyways (and some are similar).  Certainly the number of hits is *WAY* down, and that's the only one currently in 50 or later.

Comment 38

[Peter Chang[:pchang]]

I landed bug 1293598 in FF 51 but I didn't see crash in FF 51/52. Any concern to close this bug?

Comment 39

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

Can you request uplift to ESR (in bug 1293598) as well? Aiming this at the ESR to be released in December. Thanks.

Comment 40

[Gerry Chang [:gchang]]

Track 51- as no crashes in 51.

Comment 41

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

Hi Peter, Dan, should we consider uplifting the fix from bug 1293598 to ESR45.7 (go-live late Jan) as this bug has a esr45 affected flag?

Comment 42

[Daniel Veditz [:dveditz]]

The higher volume of UAF crashes stopped[*] abruptly after 47.0b7 -- what was fixed/changed in 47 beta 8? That seems far more useful for ESR45 than something that landed in 51. ESR-45 doesn't seem to have a lot of these crashes and I'm not convinced this is the main fix so let's not mess with bug 1293598 on ESR.

[*] they didn't literally "stop", but they are few and far between now. Looking at UAF crashes (address contains e5e5) in the last 3 months nothing newer than 47.0b7 makes it into the "top 50" versions reporting crashes. There was one in 51.0b1, one in 50.0, one in 50.b10, two in 49.0.2...

https://crash-stats.mozilla.com/signature/?address=~e5e5&signature=compute_image_info&date=%3E%3D2016-10-11T20%3A35%3A16.000Z&date=%3C2017-01-11T20%3A35%3A16.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-version&_sort=-date&page=1#aggregations

Comment 43

[Peter Chang[:pchang]]

Basically I agree Dan's opinion in comment 42.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Did we ever follow-up on the question in comment 42 about what made the frequency drop off after 47.0b7? Is there something to do for ESR45 here at this point?

Comment 45

[Peter Chang[:pchang]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #44)
> Did we ever follow-up on the question in comment 42 about what made the
> frequency drop off after 47.0b7? Is there something to do for ESR45 here at
> this point?
I just checked the changes between 47.0b7 and 47.0b8 from below change set.
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=0723a0212f5e&tochange=a112e55fc5e0

I found bug 1273701 should be related to this crash but it was already uplifted to ESR 45. Am I correct?

Comment 46

[Daniel Veditz [:dveditz]]

In the past six months I don't see any UAF-looking crashes with this signature on any ESR-45 release after 45.1.1. ESR-45.2 would have corresponded to 47 which is consistent with comment 45 and bug 1273701