Bug 1174479 (CVE-2015-7176)

Bad sscanf argument in AnimationThread overruns stack variable

RESOLVED FIXED in Firefox 41, Firefox OS master

Status

()

--
major
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: q1, Assigned: baku)

Tracking

({sec-low})

38 Branch
mozilla41
sec-low
Points:
---

Firefox Tracking Flags

(firefox41 fixed, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main41+])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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.
(Reporter)

Comment 2

3 years ago
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.
(Reporter)

Updated

3 years ago
Severity: normal → major
(Assignee)

Comment 3

3 years ago
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...
Attachment #8622388 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini

Updated

3 years ago
Component: Untriaged → Widget: Gonk
Product: Firefox → Core
(Reporter)

Comment 4

3 years ago
(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.
(Assignee)

Comment 5

3 years ago
> 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

3 years ago
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.
Attachment #8622388 - Flags: review?(ehsan) → review-
(Reporter)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8622530 [details] [diff] [review]
crash8.patch
Attachment #8622388 - Attachment is obsolete: true
Attachment #8622530 - Flags: review?(ehsan)
(Reporter)

Comment 9

3 years ago
(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

3 years ago
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!
Attachment #8622530 - Flags: review?(ehsan) → review-
(Assignee)

Comment 11

3 years ago
Created attachment 8622990 [details] [diff] [review]
crash8.patch

Removing sscanf is a bigger task. Maybe a follow up to remove all the sscanf in gecko code would be nice.
Attachment #8622530 - Attachment is obsolete: true
Attachment #8622990 - Flags: review?(ehsan)
(Reporter)

Comment 12

3 years ago
(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!
Is the OS returning bogus data on B2G a real concern? Do you have any idea mwu?
Flags: needinfo?(mwu)
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.
Keywords: sec-low

Comment 15

3 years ago
Agreed with dveditz. You need root in order to change these files, so you've already lost if things are getting swapped out.
Flags: needinfo?(mwu)

Comment 16

3 years ago
(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.
(Reporter)

Comment 17

3 years ago
(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?
(Reporter)

Comment 18

3 years ago
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.
(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

3 years ago
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.

Updated

3 years ago
Attachment #8622990 - Flags: review?(ehsan) → review+
(Reporter)

Comment 21

3 years ago
(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.
(Assignee)

Updated

3 years ago
URL:
https://hg.mozilla.org/mozilla-central/rev/6961dc3eba19
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Whiteboard: [post-critsmash-triage]

Updated

3 years ago
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+]
Alias: CVE-2015-7176
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.