1125925 - UAF in gfxContext::Clip

Status: RESOLVED WORKSFORME

(Core :: Graphics: Layers, defect)

Description

[Byron Campen [:bwc]]

Attachment: uaf.txt

While trying to test a talky.io call on ASAN, I got the attached crash.

Comment 1

[Byron Campen [:bwc]]

Attachment: Another ASAN crash when closing the browser on a talky.io call

Comment 2

[Matt Woodrow (:mattwoodrow)]

This looks to be because we're using the clip path pool on multiple threads.

It all looks thread safe though, and I can't see how this would actually happen.

Comment 3

[Andrew McCreight [:mccr8]]

Byron, are you able to reproduce this regularly or did this just happen one time?

Comment 4

[Byron Campen [:bwc]]

I got two slightly different crashes, both attached here. I have not seen this again, although I don't think I've run talky.io under ASAN since then.

Comment 5

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> This looks to be because we're using the clip path pool on multiple threads.
> 
> It all looks thread safe though, and I can't see how this would actually
> happen.

First crash has the original allocate, free and uaf all on the same thread?  Second crash does have the two threads involved.  I could be reading it wrong.

Comment 6

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> This looks to be because we're using the clip path pool on multiple threads.

According to Jeff, we're not actually supposed to be using the clip path pool?

Comment 7

[Byron Campen [:bwc]]

(In reply to Milan Sreckovic [:milan] from comment #5)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> > This looks to be because we're using the clip path pool on multiple threads.
> > 
> > It all looks thread safe though, and I can't see how this would actually
> > happen.
> 
> First crash has the original allocate, free and uaf all on the same thread? 
> Second crash does have the two threads involved.  I could be reading it
> wrong.

That is correct.

Comment 8

[Byron Campen [:bwc]]

One thing that jumps out at me is the use of pool->top (eg; here https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-freed-pool-private.h#80). There are no threadsafety protections on that integer, so it could literally take any value depending on the whims of the compiler, including values beyond the end of the pool array.

Comment 9

[Byron Campen [:bwc]]

This looks flat-out wrong:

https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-freed-pool.c#50

    for (i = ARRAY_LENGTH (pool->pool); i--;) {
	ptr = _atomic_fetch (&pool->pool[i]);
	if (ptr != NULL) {
	    pool->top = i;
	    return ptr;
	}
    }

Surely they meant to do something like

    for (i = ARRAY_LENGTH (pool->pool); i > 0; i--) {
	ptr = _atomic_fetch (&pool->pool[i]);
	if (ptr != NULL) {
	    pool->top = i;
	    return ptr;
	}
    }

Comment 10

[Byron Campen [:bwc]]

I should also point out that if we mess up the refcounting somehow, we could end up placing a clip_path into the pool, and then destroying it again and freeing it this time because the pool is full, leading to the possibility that some code later will pull it out of the pool and UAF.

Comment 11

[Byron Campen [:bwc]]

Given that this was an ASAN run, the problem in comment 8 is probably not the cause, since an array out of bounds error would have shown up. I also don't think the problem in comment 9 will get us; it is just a busywait or maybe a deadlock at worst. My money is on comment 10.

Comment 12

[Byron Campen [:bwc]]

I've only looked a little bit, but I'm already seeing signs that cairo is not doing an awesome job of keeping its refcounts correct:

https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.c#270

Before this line, the object pointed to by clip->path (call it X) has a refcount of a, and the object pointed to by clip->path->prev (Y) has a refcount of b. After this line, the refcounts remain the same, but X should have a refcount of a - 1, and Y should have a refcount of b + 1 (X still points at Y, so now clip->path is a new reference). The best-case scenario here is for X to be leaked. However, if anything else is holding a ref to X, we're in real trouble and will UAF.

Comment 13

[Byron Campen [:bwc]]

On closer inspection, in comment 12 |clip_path| is another ref to X, so the code is definitely setting up a UAF.

Comment 14

[Byron Campen [:bwc]]

The same bad error handling in some other places:

https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.c#388
https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.c#431
https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.c#449

Comment 15

[Byron Campen [:bwc]]

Searching cairo_clip_ on crash-stats finds some stuff, and searching for cairo_ finds more stuff, some of which is operating on some member of a clip path.

Comment 16

[Byron Campen [:bwc]]

FWIW, the latest cairo source does not seem to have these problems.

Comment 17

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

(In reply to Byron Campen [:bwc] (PTO Feb 18-20) from comment #9)
> This looks flat-out wrong:
> 
> https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> freed-pool.c#50
> 
>     for (i = ARRAY_LENGTH (pool->pool); i--;) {
> ...
> 
> Surely they meant to do something like
> 
>     for (i = ARRAY_LENGTH (pool->pool); i > 0; i--) {
> ...

Yeah, I got caught when I first read this as well; it isn't wrong, but it is annoyingly written.  The two for loop statements are equivalent, so the bug is elsewhere.

Comment 18

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

Milan, can someone look into this so it can be moved forward?

Comment 19

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

Byron, with all the investigation you've done, do you have an idea of a patch?

Comment 20

[Byron Campen [:bwc]]

Well, I've seen how cairo fixed the bad error handling identified in comment 12 and comment 14, but who knows whether those are actually the cause. It would take a lot more investigation to figure out where the problem actually lies, since I only scratched the surface. We probably want to update our cairo version.

Comment 21

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

Good point, updating the cairo version would be a good thing to do, though probably not the quick fix we're looking for.  Al, let me see what we can do here, quickly.

Comment 22

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

Didn't mean to remove NI on me.

Comment 23

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

(In reply to Byron Campen [:bwc] from comment #16)
> FWIW, the latest cairo source does not seem to have these problems.

Is this the latest cairo with all of our patches applied, or as it is upstream?

Comment 24

[Byron Campen [:bwc]]

(In reply to Milan Sreckovic [:milan] from comment #23)
> (In reply to Byron Campen [:bwc] from comment #16)
> > FWIW, the latest cairo source does not seem to have these problems.
> 
> Is this the latest cairo with all of our patches applied, or as it is
> upstream?

Upstream.

Comment 25

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

We do need to track this for 38 since we don't want to ship this sec-critical problem in 38. Milan can you help us find an owner on this bug?  Thanks.

Comment 26

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

Does this also affect 37?

Comment 27

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

This probably affects even older releases, don't think this is a recent regression.

Comment 28

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

Can we schedule to take the newer Cairo? This is sec-critical and it has been sitting around for more than a month.

Comment 29

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

Lee, can you take a look?

Comment 30

[Lee Salzman [:lsalzman]]

(In reply to Byron Campen [:bwc] from comment #8)
> One thing that jumps out at me is the use of pool->top (eg; here
> https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> freed-pool-private.h#80). There are no threadsafety protections on that
> integer, so it could literally take any value depending on the whims of the
> compiler, including values beyond the end of the pool array.

At least in this case, the value of top seems to be advisory, functioning more like a fuzzy value for a search start, and thread safety is guaranteed by atomic exchanges (with NULL) used to actually fetch pointers from the pool's array, however unreliable the actual value of top may be.

So when it tries to get something from the pool, it looks at the top value, checks for a non-NULL pointer there. If it doesn't find one, it searches the entire pool in any case. Upon analysis, I don't see how this particular piece of the code could be at fault.

The freed pool does appear to be getting used in any case.

Comment 31

[Lee Salzman [:lsalzman]]

(In reply to Byron Campen [:bwc] from comment #0)
> Created attachment 8554646 [details]
> uaf.txt
> 
> While trying to test a talky.io call on ASAN, I got the attached crash.

Are you still able to reproduce the bug, and if so, is there any more boiled down/reliable procedure that other people might be able to reproduce it by? I at least tried doing a few talky calls, but couldn't get anything suspicious to occur for me.

Comment 32

[Byron Campen [:bwc]]

This still happens pretty frequently for me. It seems to happen much more often when the window that is running a webrtc call is backgrounded (say, switching from the browser to a terminal window).

Comment 33

[Lee Salzman [:lsalzman]]

Attachment: Patch to properly handle undoing clip paths in error cases

Okay, I put up a hypothetical patch to handle some clearly erroneous behavior when it is trying to destroy paths in failure cases.

The clip_path->prev link points to the old path stored in the clip info. But when it destroys clip_path, it never nulls out the prev link, so _clip_path_destroy tries to free it, even though the clip info got altered so that the clip info now points to the very same path (clip->path = clip->path->prev) that may get accidentally destroyed. This is a case of "but how did this ever work at all?" 

This seems to agree with the fact the first trace all happened on a single thread, so wasn't necessarily a result of a race condition.

While this is definitely a bug in the code, I'm not sure it is THE bug we are experiencing here, so if you could try applying this small patch that handles properly NULLing out the prev link before freeing the new clip path to your tree and see if it makes the problem go away for you... If it does I'll clean up the patch and make it more official-like.

Comment 34

[Byron Campen [:bwc]]

Attachment: Crash with patch applied

Patch doesn't seem to have fixed the crashiness.

Comment 35

[Byron Campen [:bwc]]

Attachment: Mozconfig I'm using. Probably needs to be scrubbed of distcc/ccache stuff for it to work for you.

Comment 36

[Lee Salzman [:lsalzman]]

(In reply to Byron Campen [:bwc] from comment #12)
> I've only looked a little bit, but I'm already seeing signs that cairo is
> not doing an awesome job of keeping its refcounts correct:
> 
> https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> clip.c#270
> 
> Before this line, the object pointed to by clip->path (call it X) has a
> refcount of a, and the object pointed to by clip->path->prev (Y) has a
> refcount of b. After this line, the refcounts remain the same, but X should
> have a refcount of a - 1, and Y should have a refcount of b + 1 (X still
> points at Y, so now clip->path is a new reference). The best-case scenario
> here is for X to be leaked. However, if anything else is holding a ref to X,
> we're in real trouble and will UAF.

It seems like the intention of the reference counts is to keep track of how many clip structs are referring to clip_path structs, and that the primary means of decrementing the RC is through _cairo_clip_path_destroy. So, the bug here, as per the undo patch, seems to be rather that the discarded clip path didn't give up its own reference.

But still, it doesn't seem like these error handling cases, though clearly buggy, are what is causing the bug, as they only seem to trigger in out of memory conditions. Whatever is going on here is somewhat less than obvious...

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Byron, do you think you could try reproducing this in rr and investigating what's going on?

Comment 38

[Byron Campen [:bwc]]

I'm not going to have the cycles for this in the near term, I'm afraid.

Comment 39

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

Would taking the newer Cairo have a chance of addressing this?

Comment 40

[Byron Campen [:bwc]]

It certainly seems plausible, but there are no guarantees.

Comment 41

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

This sec-critical bug has been around for almost six months now. We need either to find someone who can work on it or we need to reassess its severity. Any ideas, Lee?

Comment 42

[Lee Salzman [:lsalzman]]

(In reply to Matt Wobensmith from comment #41)
> This sec-critical bug has been around for almost six months now. We need
> either to find someone who can work on it or we need to reassess its
> severity. Any ideas, Lee?

Further work is stalled until Byron can collaborate on reproducing this.

Comment 43

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

Byron, see comment 37. I know you are busy, but we are blocked on this critical bug.

Comment 44

[Byron Campen [:bwc]]

I'll only be able to help when it starts happening again, sadly.

Comment 45

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

OK, let's reopen when it starts happening again, so that we don't spin on this bug too much.

Comment 46

[Lawrence Mandel [:lmandel] (use needinfo)]

We don't have a worksforme status option so I'm marking this as wontfix for all active branches. We should reset status if this bug is reopened.

Comment 47

[Byron Campen [:bwc]]

This is happening again, and I've developed a (temporary?) knack for making it happen reliably. Disabling the memory pooling for cairo fixes the problem. Digging into this some more.

Comment 48

[Byron Campen [:bwc]]

Oh now that's interesting. The atomics stuff we're using (HAVE_CXX11_ATOMIC_PRIMITIVES) was written by us. We upstreamed it, but I'm not sure anyone but us is using it. Maybe there's some flaw?

Comment 49

[Byron Campen [:bwc]]

https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairoint.h?case=true&from=cairoint.h#114

ಠ_ಠ