Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer overflow in getString in decompile.c:362 #144

Closed
c1208828 opened this issue May 16, 2018 · 4 comments
Closed

buffer overflow in getString in decompile.c:362 #144

c1208828 opened this issue May 16, 2018 · 4 comments
Assignees

Comments

@c1208828
Copy link

https://docs.google.com/document/d/1NtI3PiiL55SMj-kmdwJhMViIALGHPnLZYRxOgNCfhYA/edit
https://drive.google.com/open?id=1qT1VRbszb343p7_w56pHPn3eLUCdj-ep

Program received signal SIGABRT, Aborted.
0x00007ffff751f428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
54 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 0x00007ffff751f428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1 0x00007ffff752102a in __GI_abort () at abort.c:89
#2 0x00007ffff75617ea in __libc_message (do_abort=do_abort@entry=2,
fmt=fmt@entry=0x7ffff767949f "*** %s ***: %s terminated\n") at ../sysdeps/posix/libc_fatal.c:175
#3 0x00007ffff760315c in __GI___fortify_fail (msg=,
msg@entry=0x7ffff7679430 "buffer overflow detected") at fortify_fail.c:37
#4 0x00007ffff7601160 in __GI___chk_fail () at chk_fail.c:28
#5 0x00007ffff76006c9 in _IO_str_chk_overflow (fp=, c=) at vsprintf_chk.c:31
#6 0x00007ffff75656b0 in __GI__IO_default_xsputn (f=0x7fffffffddb0, data=, n=10) at genops.c:455
#7 0x00007ffff7537e00 in _IO_vfprintf_internal (s=s@entry=0x7fffffffddb0, format=,
format@entry=0x4824af "%ld", ap=ap@entry=0x7fffffffdee8) at vfprintf.c:1631
#8 0x00007ffff7600754 in ___vsprintf_chk (s=0x6b1430 "264435123", flags=1, slen=10, format=0x4824af "%ld",
args=args@entry=0x7fffffffdee8) at vsprintf_chk.c:82
#9 0x00007ffff76006ad in ___sprintf_chk (s=s@entry=0x6b1430 "264435123", flags=flags@entry=1, slen=slen@entry=10,
format=format@entry=0x4824af "%ld") at sprintf_chk.c:31
#10 0x0000000000418e04 in sprintf (__fmt=0x4824af "%ld", __s=0x6b1430 "264435123")
at /usr/include/x86_64-linux-gnu/bits/stdio2.h:33
#11 getString (act=act@entry=0x691ee0) at decompile.c:362
#12 0x00000000004199bb in getName (act=act@entry=0x691ee0) at decompile.c:465
#13 0x000000000041e9e9 in decompileSETVARIABLE (islocalvar=islocalvar@entry=0, maxn=8, actions=0x6a6e20, n=4)
at decompile.c:1863
#14 0x000000000042bc9b in decompileAction (n=4, actions=0x6a6e20, maxn=8) at decompile.c:3303
#15 0x0000000000451d6d in decompileActions (indent=, actions=, n=8) at decompile.c:3494
#16 decompile_SWITCH (n=0, off1end=, maxn=, actions=0x6a6ce0) at decompile.c:2235
#17 decompileIF (n=, actions=, maxn=) at decompile.c:2594
#18 0x0000000000440a65 in decompileActions (indent=, actions=0x69c780, n=11) at decompile.c:3494
#19 decompileSETTARGET (n=, actions=, maxn=, is_type2=)
at decompile.c:3169
#20 0x0000000000451d6d in decompileActions (indent=, actions=, n=13) at decompile.c:3494
#21 decompile_SWITCH (n=0, off1end=, maxn=, actions=0x69c5f0) at decompile.c:2235
#22 decompileIF (n=, actions=, maxn=) at decompile.c:2594
#23 0x0000000000440a65 in decompileActions (indent=, actions=0x6921e0, n=12) at decompile.c:3494
#24 decompileSETTARGET (n=, actions=, maxn=, is_type2=)
at decompile.c:3169
#25 0x000000000045752d in decompileActions (indent=, actions=0x692140, n=13) at decompile.c:3494
#26 decompile5Action (n=13, actions=0x692140, indent=indent@entry=0) at decompile.c:3517
#27 0x000000000040f34a in outputSWF_DOACTION (pblock=0x691250) at outputscript.c:1551
#28 0x000000000040211e in readMovie (f=0x690010) at main.c:281
#29 main (argc=, argv=) at main.c:354

Breakpoint 1, getString (act=act@entry=0x691ee0) at decompile.c:361
361 t=malloc(10); /* 32-bit decimal /
(gdb) l
356 t = malloc(needed_length);
357 sprintf(t, "%g", act->p.Double );
358 return t;
359 }
360 case PUSH_INT: /
INTEGER /
361 t=malloc(10); /
32-bit decimal /
362 sprintf(t,"%ld", act->p.Integer );
363 return t;
364 case PUSH_CONSTANT: /
CONSTANT8 /
365 if (act->p.Constant8 > poolcounter)
(gdb) n
362 sprintf(t,"%ld", act->p.Integer );
(gdb) n
361 t=malloc(10); /
32-bit decimal */
(gdb) n
362 sprintf(t,"%ld", act->p.Integer );
(gdb) n
*** buffer overflow detected ***: /home/afl/parse/eval/new_swftophp/swftophp terminated

@c1208828 c1208828 changed the title buffer overflow in getString in decompile:362 buffer overflow in getString in decompile.c:362 May 16, 2018
@hlef
Copy link
Contributor

hlef commented May 25, 2018

Reproduced on latest master. FTR, this issue was assigned CVE-2018-11226.

@hlef
Copy link
Contributor

hlef commented May 25, 2018

Hum, this is a big one. The actions[i+/-something].SWF_ACTIONRECORD.Offset pattern can be found a bit everywhere in util/decompile.c. This is a critical place because the actions array is accessed without proper bound-checking, only relying on the robustness of the loop/function manipulating i. I'm pretty sure a ton of similar flaws can be found, just like in the OpCode case.

In this case: For each i from 0 to [size of array - 1] we access element [i+1]. This is obviously oob when i = [size of array - 1].

Fix: I propose to add a new function similar to OpCode() that would return the offset and do bound checking. This would allow us to replace all potentially affected places with very low regression risks.

For example:

diff --git a/util/decompile.c b/util/decompile.c
index e9341356..2ea2b06b 100644
--- a/util/decompile.c
+++ b/util/decompile.c
@@ -939,6 +939,24 @@ decompileGETURL2 (SWF_ACTION *act)
        return 0;
 }
 
+static inline int Offset(SWF_ACTION *actions, int n, int maxn)
+{
+       if(!n || n >= maxn)
+       {
+#if DEBUG
+               SWF_warn("Offset: want %i, max %i\n", n, maxn);
+#endif
+               return -999;
+       } else if (n < 1) {
+
+#if DEBUG
+               SWF_warn("Offset: want %i < 1\n", n);
+#endif
+               return -998;
+        }
+       return actions[n].SWF_ACTIONRECORD.Offset;
+}
+
 static inline int OpCode(SWF_ACTION *actions, int n, int maxn)
 {
        if(!n || n >= maxn)
@@ -2101,7 +2119,7 @@ decompile_SWITCH(int n, SWF_ACTION *actions, int maxn, int off1end)
        int offSave;
        for (i=0; i<n_firstactions; i++) // seek last op in 1st if
        {
-               if (actions[i+1].SWF_ACTIONRECORD.Offset==off1end)
+               if (Offset(actions, i+1, maxn) == off1end)
                {
                        // println("found #off end first= %d",i+1);
                        if (OpCode(actions, i, maxn) == SWFACTION_JUMP)

(only fixing the current vulnerability, but we can do the same for all potentially affected places)

However, fixing this vulnerability revealed another one in countAllSwitchActions so this patch might not be sufficient to fully address the CVE.

@hlef
Copy link
Contributor

hlef commented Jun 30, 2018

Looks like the second issue is in decompileSETTARGET.

We are given a n which is the current index in the actions array and a maxn which is the number of elements in the array. Here n is 0 and maxn is 13.

The loop

	while(action_cnt+n<maxn)
	{
		if (OpCode(actions, n+1+action_cnt, maxn)==SWFACTION_SETTARGET
		    || OpCode(actions, n+1+action_cnt, maxn)==SWFACTION_SETTARGET2
		    || OpCode(actions, n+1+action_cnt, maxn)==SWFACTION_DEFINEFUNCTION
		    || OpCode(actions, n+1+action_cnt, maxn)==SWFACTION_DEFINEFUNCTION2
		    || OpCode(actions, n+1+action_cnt, maxn)==SWFACTION_END) 
		{
			break;
		}
		action_cnt++;
	}

counts the number of operations until the next SWFACTION_END, SWFACTION_DEFINEFUNCTION or SWFACTION_SETTARGET operation. All these operations are then processed in

decompileActions(action_cnt,&actions[n+1],gIndent+1);

This means that action_cnt + n should always be smaller than maxn, because &actions[n+1] has maxn - 1 elements (IIRC actions[n] is the current decompileSETTARGET action, so we don't want to re-process it). However in this case we have action_cnt + n = 13 = maxn.

Why ?

Because the end condition of the while loop is action_cnt+n<maxn, and not action_cnt+n<maxn-1.

Fix:

diff --git a/util/decompile.c b/util/decompile.c
index e444e2f7..cf1a372d 100644
--- a/util/decompile.c
+++ b/util/decompile.c
@@ -3197,7 +3197,7 @@ decompileSETTARGET(int n, SWF_ACTION *actions, int maxn, int is_type2)
        {
                INDENT
                println("tellTarget('%s') {" ,name);
-               while(action_cnt+n<maxn)
+               for (; action_cnt+n < maxn-1; action_cnt++)
                {
                        if (OpCode(actions, n+1+action_cnt, maxn)==SWFACTION_SETTARGET
                            || OpCode(actions, n+1+action_cnt, maxn)==SWFACTION_SETTARGET2
@@ -3207,7 +3207,6 @@ decompileSETTARGET(int n, SWF_ACTION *actions, int maxn, int is_type2)
                        {
                                break;
                        }
-                       action_cnt++;
                }
                decompileActions(action_cnt,&actions[n+1],gIndent+1);
                INDENT

These two patches should be sufficient to fully address this CVE. I'll submit a PR once the current one has been reviewed.

@hlef hlef self-assigned this Jun 30, 2018
hlef added a commit to hlef/libming that referenced this issue Jun 30, 2018
The getString method in decompile.c is vulnerable to a buffer
overflow which can be triggered using a crafted SWF file.

This vulnerability is the consequence of unchecked accesses to the
actions array when getting the offset of SWF_ACTIONRECORD objects.

This pattern is present a bit everywhere in the source code, leading
to a large number of potential flaws similar to this one. In this
commit we introduce a new Offset method similar to the OpCode method
which handles bound checking when retrieving the offset of
SWF_ACTIONRECORD objects.

This commit also modifies getString to use this newly introduced
method and address the previously explained bug.

Usage of the newly introduced Offset method will be generalized in a
future commit.

Please, note that this commit won't be sufficient to fix libming#144
(CVE-2018-11226) since another issue is triggered by the same sample.
hlef added a commit to hlef/libming that referenced this issue Jun 30, 2018
In decompileSETTARGET a while loop is used to count the number of
operations until a certain type of operation has been reached. This
loop uses action_cnt+n < maxn as stop condition, meaning that
action_cnt+n = maxn might be true after the loop.

This is wrong because action_cnt is used as the number of operations
to process in an array of maxn-n-1 elements.

Fix the loop's stop condition and switch to for loop for better
readability.

This patch is the second part of the CVE-2018-11226 fix (fixes: libming#144).
@hlef
Copy link
Contributor

hlef commented Jun 30, 2018

Oh, well, just pushed the commits as part of the current PR.

@strk strk closed this as completed in 6e5a28d Jul 12, 2020
strk pushed a commit that referenced this issue Jul 12, 2020
In decompileSETTARGET a while loop is used to count the number of
operations until a certain type of operation has been reached. This
loop uses action_cnt+n < maxn as stop condition, meaning that
action_cnt+n = maxn might be true after the loop.

This is wrong because action_cnt is used as the number of operations
to process in an array of maxn-n-1 elements.

Fix the loop's stop condition and switch to for loop for better
readability.

This patch is the second part of the CVE-2018-11226 fix (fixes: #144).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants