1174479 - (CVE-2015-7176) Bad sscanf argument in AnimationThread overruns stack variable

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[q1]

AnimationThread (38.0.1\widget\gonk\libdisplay\BootAnimation.cpp line 449) can overrun a stack variable if an operating system file contains oversize strings. If that happens, it also pushes elements into an std::vector that contain unterminated strings.

This problem is on lines 523-24:

253: struct AnimationPart {
254:    int32_t count;
255:    int32_t pause;
256:    char path[256];
257:    vector<AnimationFrame> frames;
258: };
...
490:     vector<AnimationPart> parts;
...
519:        AnimationPart part;
...
523:        } else if (sscanf(line, "p %d %d %s",
524:                          &part.count, &part.pause, part.path)) {
525:            parts.push_back(part);
...

The "%s" specifier will write into part.path as many characters as are available, thus potentially overrunning that variable.

The variable "line" that line 523 parses comes from an operating system file that is documented on line 492 et seq.

It is unclear what security effects this overrun can have, but at least the data causing the overrun appears to be under operating system control.

Comment 1

[q1]

Also, the if statement on line 523 doesn't check whether sscanf set all of the arguments, so if line is malformed, line 525 could add an incompletely-initialized element to parts.

Comment 2

[q1]

This is probably a much worse bug than I initially thought.

It turns out that users commonly replace bootanimation.zip with files they download. See, e.g., http://androidbootanimation.com/ .

Unless the OS rejects invalid files when installation is attempted, an attacker could obtain complete control of the application (and possibly the entire device; I don't know the Android security model) using this bug. Basically she could construct a bootanimation.zip to contain an entry whose path string overflows part.path on line 519, and whose overflow portion contains arguments to, and a return address of, a desirable function, such that, when the current function executes its return instruction (e.g., ARM pop {r<whatever>, pc}, control enters that function. This approach should work for ARM-architecture devices because they have grow-down stacks just like the x86/x64, so a function's return address resides at a higher address than that of the function's stack variables.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: crash8.patch

I think I can do something better than this, but this small patch fixes the problem. In general we should not use sscanf...

Comment 4

[q1]

(In reply to Andrea Marchesini (:baku) from comment #3)
> Created attachment 8622388 [details] [diff] [review]
> crash8.patch
> 
> I think I can do something better than this, but this small patch fixes the
> problem. In general we should not use sscanf...

That fixes the overflow, but there is another problem:

        } else if (sscanf(line, "p %d %d %255s",
                           &part.count, &part.pause, part.path)) {
             parts.push_back(part);
         }

which is that part gets pushed even if sccanf returns EOF or if it correctly parses only the first 1 or 2 items, in which case the other item(s) would be uninitialized, possibly causing some other security issue and certainly causing a correctness issue.

Comment 5

[Andrea Marchesini [:baku]]

> which is that part gets pushed even if sccanf returns EOF or if it correctly
> parses only the first 1 or 2 items, in which case the other item(s) would be
> uninitialized, possibly causing some other security issue and certainly
> causing a correctness issue.

Good point. thanks q1. New patch in a few secs.

Comment 6

[(no longer active)]

Comment on attachment 8622388 [details] [diff] [review]
crash8.patch

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

::: widget/gonk/libdisplay/BootAnimation.cpp
@@ +574,5 @@
>          AnimationPart part;
>          if (headerRead &&
>              sscanf(line, "%d %d %d", &width, &height, &fps) == 3) {
>              headerRead = false;
> +        } else if (sscanf(line, "p %d %d %255s",

Nit: please add a comment to the declaration of AnimationPart::path to indicate that if the length is updated there, it should be updated here as well.  Even better, add a helper ReadFromLine() method or some such to AnimationPart to do this work right next to the member to make things more obvious.

@@ +579,1 @@
>                            &part.count, &part.pause, part.path)) {

As per comment 4, please check the return value.

Comment 7

[q1]

Speaking of sscanf, its behavior is undefined "if the result of the conversion cannot be represented
in the object", C11 N1570 s.7.21.6.2(10). I'm pretty sure that this means that, for example, using %d to convert 9082340983209832498384 invoked undefined behavior. Also, s(2) says "If there are insufficient arguments for the format, the behavior is undefined." It seems, then, that sscanf is formally useless (similar observation in 38.0.1\mfbt\IntegerPrintfMacros.h line 13) as well as dangerous.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: crash8.patch

Comment 9

[q1]

(In reply to Andrea Marchesini (:baku) from comment #8)
> Created attachment 8622530 [details] [diff] [review]
> crash8.patch
> +static bool
> +ReadAnimationPartLineHeader(const char* aLine, int32_t* aWidth,
> +                            int32_t* aHeight, int32_t* aFps)
> +{
> +    MOZ_ASSERT(aLine && aWidth && aHeight && aFps);
> +    return sscanf(aLine, "%d %d %d", &aWidth, &aHeight, &aFps) == 3;
> +}

The & operators on the sscanf are incorrect.

Comment 10

[(no longer active)]

Comment on attachment 8622530 [details] [diff] [review]
crash8.patch

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

Can we just avoid using sscanf here?

::: widget/gonk/libdisplay/BootAnimation.cpp
@@ +499,5 @@
>      aDisplay->QueueBuffer(buffer);
>  }
>  
> +static bool
> +ReadAnimationPartLineHeader(const char* aLine, int32_t* aWidth,

I meant make this a member of AnimationPart.

@@ +503,5 @@
> +ReadAnimationPartLineHeader(const char* aLine, int32_t* aWidth,
> +                            int32_t* aHeight, int32_t* aFps)
> +{
> +    MOZ_ASSERT(aLine && aWidth && aHeight && aFps);
> +    return sscanf(aLine, "%d %d %d", &aWidth, &aHeight, &aFps) == 3;

Too many levels of indirection!

Comment 11

[Andrea Marchesini [:baku]]

Attachment: crash8.patch

Removing sscanf is a bigger task. Maybe a follow up to remove all the sscanf in gecko code would be nice.

Comment 12

[q1]

(In reply to Andrea Marchesini (:baku) from comment #11)

> Maybe a follow up to remove all the sscanf in gecko code would be nice.

I'd vote for that!

Comment 13

[Andrew McCreight [:mccr8]]

Is the OS returning bogus data on B2G a real concern? Do you have any idea mwu?

Comment 14

[Daniel Veditz [:dveditz]]

Going to rate this sec-low only because we don't let people change these files. The phones (especially dev versions) are hackable so I can't rule it out, but then the damage would either be self-inflicted or a carrier-caused bug they should fix before shipping.

Comment 15

[Michael Wu [:mwu]]

Agreed with dveditz. You need root in order to change these files, so you've already lost if things are getting swapped out.

Comment 16

[Michael Wu [:mwu]]

(In reply to q1 from comment #2)
> This is probably a much worse bug than I initially thought.
> 
> It turns out that users commonly replace bootanimation.zip with files they
> download. See, e.g., http://androidbootanimation.com/ .
> 

Root is necessary to do this on B2G. Android *might* support loading boot animations from different locations (I don't remember), but B2G doesn't.

Comment 17

[q1]

(In reply to Michael Wu [:mwu] from comment #16)
> (In reply to q1 from comment #2)
> > This is probably a much worse bug than I initially thought.
> > 
> > It turns out that users commonly replace bootanimation.zip with files they
> > download. See, e.g., http://androidbootanimation.com/ .
> > 
> 
> Root is necessary to do this on B2G. Android *might* support loading boot
> animations from different locations (I don't remember), but B2G doesn't.

What do ordinary users need to do to obtain root privileges? Is this something users commonly do? On Android, it looks like replacing these files is pretty common: http://www.addictivetips.com/mobile/how-to-change-customize-create-android-boot-animation-guide/ . BTW, is BootAnimation.cpp used only by the Firefox OS?

Comment 18

[q1]

From http://www.addictivetips.com/mobile/how-to-change-customize-create-android-boot-animation-guide/ :

So you have found a boot animation that you want to install on your phone? Created one of your own and can’t wait to see it in action on your device? All you have to do is copy it at a certain location on your device. There are two locations you can copy it: /data/local and /system/media, and both have their advantages and disadvantages.

Advantages of copying it to /data/local is that you will not require root access for it and it should work for all non-rooted devices without running the risk of changing anything in the /system partition. Furthermore, if a bootanimation.zip file is found in both locations, Android ignores the one found in /system/media and gives priority to the one in /data/local. A disadvantage of this method is that upon a hard reset (also called a full data wipe or a factory reset), the new boot animation will be lost.

Copying the boot animation to /system/media/ is possible only if your device is rooted and you have read+write access to the /system partition.

Comment 19

[Andrew McCreight [:mccr8]]

(In reply to q1 from comment #17)
> BTW, is BootAnimation.cpp used only by the Firefox OS?

The directory of the file widget/gonk indicates that it is Firefox OS only. "Gonk" is the name for the kernel level stuff in Firefox OS. There's a bit more explanation here: https://developer.mozilla.org/Firefox_OS/Platform/Gonk

Comment 20

[Michael Wu [:mwu]]

BootAnimation.cpp is a mostly independent implementation based on what Android does with bootanimation files.

Root is normally not available on phones unless the phone is a development device (Flame, Nexus) or the vendor has ways for you to load your own code (HTC, Sony). There are often exploits to get root access too. But it isn't a supported thing. Once you go off rooting your device and making changes, you're on your own.

Comment 21

[q1]

(In reply to Andrew McCreight [:mccr8] from comment #19)
> (In reply to q1 from comment #17)
> > BTW, is BootAnimation.cpp used only by the Firefox OS?
> 
> The directory of the file widget/gonk indicates that it is Firefox OS only.
> "Gonk" is the name for the kernel level stuff in Firefox OS. There's a bit
> more explanation here: https://developer.mozilla.org/Firefox_OS/Platform/Gonk

Thanks.

Comment 22

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6961dc3eba19

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6961dc3eba19