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

Null pointer dereference vulnerability in getName (util/decompile.c:380) #121

Closed
fantasy7082 opened this issue Mar 9, 2018 · 8 comments · Fixed by #127
Closed

Null pointer dereference vulnerability in getName (util/decompile.c:380) #121

fantasy7082 opened this issue Mar 9, 2018 · 8 comments · Fixed by #127

Comments

@fantasy7082
Copy link

fantasy7082 commented Mar 9, 2018

Hi, i found a null pointer dereference bug in the libming 0.4.8. It crashed in function getName .the details are below(ASAN):

./swftocxx 013-NULL-ptr-swf /dev/null
....
...
ASAN:SIGSEGV
=================================================================
==1645==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f9aac0d3746 bp 0x7ffe01d6c3d0 sp 0x7ffe01d6bb58 T0)
    #0 0x7f9aac0d3745 in strlen (/lib/x86_64-linux-gnu/libc.so.6+0x8b745)
    #1 0x7f9aaccab1a5 in __interceptor_strlen (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x701a5)
    #2 0x4112fd in getName /root/libming-asan/util/decompile.c:380
    #3 0x41d38b in decompileCALLMETHOD /root/libming-asan/util/decompile.c:2865
    #4 0x41e7ea in decompileAction /root/libming-asan/util/decompile.c:3285
    #5 0x41eba0 in decompileActions /root/libming-asan/util/decompile.c:3419
    #6 0x41eccd in decompile5Action /root/libming-asan/util/decompile.c:3441
    #7 0x4066c6 in outputSWF_DEFINEBUTTON2 /root/libming-asan/util/outputscript.c:931
    #8 0x40e331 in outputBlock /root/libming-asan/util/outputscript.c:2083
    #9 0x40f3d9 in readMovie /root/libming-asan/util/main.c:286
    #10 0x40fb0e in main /root/libming-asan/util/main.c:359
    #11 0x7f9aac06882f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #12 0x401b58 in _start (/usr/local/libming-asan/bin/swftocxx+0x401b58)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ??:0 strlen
==1645==ABORTING

POC FILE:https://github.com/fantasy7082/image_test/blob/master/013-NULL-ptr-swf

@hlef
Copy link
Contributor

hlef commented Mar 12, 2018

Reproduced on latest master. AFAIK this issue wasn't assigned a CVE number.

@attritionorg
Copy link

Related to #122 maybe? Same file/function.

@hlef
Copy link
Contributor

hlef commented Mar 13, 2018

It looked like a different issue to me. This file contains a lot of functions with a lot of different flaws, so it isn't really surprising to find different issues in this same small portion of code. I'm currently investigating it and will PR a fix soon.

hlef added a commit to hlef/libming that referenced this issue Mar 13, 2018
Whenever getString or getName are called with an act such that act->p.String
is a NULL pointer, a NULL pointer dereference might happen
(strlen(act->p.string) is called).

In this commit we add checks at the beginning of the PUSH_STRING block so
that a warning is displayed and an empty string is returned in this case.

This patch fixes libming#121.
hlef added a commit to hlef/libming that referenced this issue Mar 13, 2018
Whenever getString or getName are called with an act such that act->p.String
is a NULL pointer, a NULL pointer dereference might happen
(strlen(act->p.string) is called).

In this commit we add checks at the beginning of the PUSH_STRING block so
that a warning is displayed and an empty string is returned in this case.

This patch fixes libming#121.
@hlef
Copy link
Contributor

hlef commented Mar 13, 2018

I've written a patch for this issue, but I'm not sure yet that it's the right one (or at least that it's enough).

In fact, my patch simply adds checks handling the case where getString or getName are called with NULL act->p.String, but the question is: Is it normal that a NULL act->p.String even exists ? It seems like a bug to me, and if it's the case it should be fixed as well.

@hlef
Copy link
Contributor

hlef commented Mar 14, 2018

After extensive debugging it looks like there is a very nasty bug underneath. Here are some interesting things:

$ ASAN_OPTIONS=abort_on_error=1 gdb swftocxx
[snip]
(gdb) break util/parser.c:1012
Breakpoint 1 at 0x39ab1: file parser.c, line 1012.
(gdb) r ~/Downloads/013-NULL-ptr-swf
Starting program: /usr/local/bin/swftocxx ~/Downloads/013-NULL-ptr-swf
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
header indicates a filesize of 2904 but filesize is 57228
#include <mingpp.h>
[snip]
character12295->drawLine(0, 291);
 
Breakpoint 1, parseSWF_ACTIONRECORD (f=0x616000000080, thisactionp=0x6020000002f4, actions=0x6070000001e0)
    at parser.c:1012
1012            break;
(gdb) c 4
Will ignore next 3 crossings of breakpoint 1.  Continuing.
parseSWF_BUTTONCONDACTION: expected actionEnd flag
 
Breakpoint 1, parseSWF_ACTIONRECORD (f=0x616000000080, thisactionp=0x603000000294, actions=0x611000001080)
    at parser.c:1012
1012            break;
(gdb) l
1007                }
1008                    act->Params = (struct SWF_ACTIONPUSHPARAM *) realloc (act->Params,
1009                                 (act->NumParam + 1) *
1010                                 sizeof (struct SWF_ACTIONPUSHPARAM));
1011                }
1012            break;
1013            }
1014        case SWFACTION_LOGICALNOT:
1015            {
1016            ACT_BEGIN_NOLEN(SWF_ACTIONNOT)
(gdb) p act->Params.p
$2 = {String = 0x6110000011c0 '0' <repeats 12 times>, Float = 6.36750022e-42, RegisterNumber = 4544,
  Boolean = 4544, Double = 5.2727351416150121e-310, Integer = 106721347375552, Constant8 = 192 '\300',
  Constant16 = 4544}
(gdb) p &act->Params.p
$3 = (union {...} *) 0x606000000338
(gdb) c
Continuing.
 
Breakpoint 1, parseSWF_ACTIONRECORD (f=0x616000000080, thisactionp=0x603000000294, actions=0x614000000240)
    at parser.c:1012
1012            break;
(gdb) c
Continuing.
[snip]
"),SWFBUTTON_MOUSEOVER);
 
 
Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:106
106 ../sysdeps/x86_64/strlen.S: No such file or directory.
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x00007ffff6e768ad in ?? () from /usr/lib/x86_64-linux-gnu/libasan.so.4
#2  0x0000555555577272 in getName (act=0x606000000320) at decompile.c:395
#3  0x00005555555837c1 in decompileCALLMETHOD (n=5, actions=0x616000001880, maxn=7) at decompile.c:2906
#4  0x0000555555584c20 in decompileAction (n=5, actions=0x616000001880, maxn=7) at decompile.c:3326
#5  0x0000555555584fd6 in decompileActions (n=7, actions=0x616000001880, indent=0) at decompile.c:3460
#6  0x000055555558510c in decompile5Action (n=7, actions=0x616000001880, indent=0) at decompile.c:3483
#7  0x000055555556c886 in outputSWF_DEFINEBUTTON2 (pblock=0x612000001540) at outputscript.c:931
#8  0x0000555555574054 in outputBlock (type=34, blockp=0x612000001540, stream=0x616000000080)
    at outputscript.c:2083
#9  0x000055555557510d in readMovie (f=0x616000000080) at main.c:281
#10 0x000055555557589c in main (argc=2, argv=0x7fffffffe1d8) at main.c:354
(gdb) up 2
#2  0x0000555555577272 in getName (act=0x606000000320) at decompile.c:395
395         t=malloc(strlen(act->p.String)+3);
(gdb) p &act->p
$4 = (union {...} *) 0x606000000338
(gdb) p act->p
$7 = {String = 0x0, Float = 0, RegisterNumber = 0, Boolean = 0, Double = 0, Integer = 0, Constant8 = 0 '\000',
  Constant16 = 0}

So, after manually verifying all SWF_ACTIONPUSHPARAM generated by parseSWF_ACTIONRECORD, I can say without doubts that there's nothing wrong with this function, that is the NULL act->p.String isn't coming from this piece of code. However, if we look at the corrupt act->p after the crash has occured, we can see that it's an object we've already met during the manual inspection, and obviously it has been corrupted at some point in between (act->p.String has been NULL-ed).

In order to see where, let's set a watchpoint:

$ ASAN_OPTIONS=abort_on_error=1 gdb swftocxx
[snip]
(gdb) watch  *(int*) 0x606000000338
Hardware watchpoint 1: *(int*) 0x606000000338
(gdb) r ~/Downloads/013-NULL-ptr-swf
Starting program: /usr/local/bin/swftocxx ~/Downloads/013-NULL-ptr-swf
[snip]
character12295->drawLine(0, 291);
parseSWF_BUTTONCONDACTION: expected actionEnd flag
 
Hardware watchpoint 1: *(int*) 0x606000000338
 
Old value = <unreadable>
New value = -1094795586
__memset_avx2_unaligned_erms () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:143
143 ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S: No such file or directory.
(gdb) c
Continuing.
 
Hardware watchpoint 1: *(int*) 0x606000000338
 
Old value = -1094795586
New value = 4544
memcpy () at ../sysdeps/x86_64/multiarch/../multiarch/memmove-vec-unaligned-erms.S:149
149 ../sysdeps/x86_64/multiarch/../multiarch/memmove-vec-unaligned-erms.S: No such file or directory.
(gdb) c
Continuing.
parseSWF_BUTTONCONDACTION: expected actionEnd flag
[snip]
"),SWFBUTTON_MOUSEOVER);
 
 
Hardware watchpoint 1: *(int*) 0x606000000338
 
Old value = 4544
New value = 0 <-- Here it is ! Our valid act->p is NULL-ed !
decompileCALLMETHOD (n=5, actions=0x616000001880, maxn=7) at decompile.c:2896
2896        if (meth->Type == PUSH_UNDEF)   /* just undefined, like in "super();" */
(gdb) l
2891        }
2892    #ifdef DEBUG
2893        printf("*CALLMethod* objName=%s (type=%d) methName=%s (type=%d)\n",
2894            getName(obj), obj->Type, getName(meth), meth->Type);
2895    #endif
2896        if (meth->Type == PUSH_UNDEF)   /* just undefined, like in "super();" */
2897            push(newVar_N(getName(obj),"","","(", nparam->p.Integer,")"));
2898        else
2899        {
2900            if (meth->Type == PUSH_INT || meth->Type == PUSH_DOUBLE || meth->Type == PUSH_VARIABLE
(gdb) l 2891
2886        {
2887            INDENT
2888            println("// Problem getting method arguments (%d ignored) below:",
2889                    nparam->p.Integer);
2890            nparam->p.Integer=0;
2891        }
2892    #ifdef DEBUG
2893        printf("*CALLMethod* objName=%s (type=%d) methName=%s (type=%d)\n",
2894            getName(obj), obj->Type, getName(meth), meth->Type);
2895    #endif
(gdb) p &nparam->p
$1 = (union {...} *) 0x606000000338
(gdb) p &obj->p
$2 = (union {...} *) 0x606000000338
(gdb) l 2886
2881        struct SWF_ACTIONPUSHPARAM *meth, *obj, *nparam;
2882        meth=pop();
2883        obj=pop();
2884        nparam=pop();
2885        if (nparam->p.Integer>25)
2886        {
2887            INDENT
2888            println("// Problem getting method arguments (%d ignored) below:",
2889                    nparam->p.Integer);
2890            nparam->p.Integer=0;

So, now we know that our act->p.String becomes NULL at util/decompile.c:2890. Also, this is a real bug because obviously nparam is meant to be modified, not act !

Why ? If we look at the addresses, it appears that nparam and act are both the same here.

Why ? Maybe something wrong with pop(), or some bug earlier. I'll have to push the investigations further.

@hlef
Copy link
Contributor

hlef commented Mar 15, 2018

Let's track calls to push and push-like function pushdup, whose arguments are our affected structure:

$ ASAN_OPTIONS=abort_on_error=1 gdb swftocxx
[snip]
(gdb) break util/decompile.c:573 if (val == 0x606000000320)
Breakpoint 1 at 0x24053: file decompile.c, line 573.
(gdb) break util/decompile.c:601 if (t->val == 0x606000000320)
Breakpoint 2 at 0x242d5: file decompile.c, line 601.
(gdb) r ~/Downloads/013-NULL-ptr-swf
Starting program: /usr/local/bin/swftocxx ~/Downloads/013-NULL-ptr-swf
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[snip]
character12296->addAction(new SWFAction("00000.000000000000.00000000000(808464432);\
"),SWFBUTTON_MOUSEOVER);
 
 
Breakpoint 1, push (val=0x606000000320) at decompile.c:577
577     t = calloc(1,sizeof(*Stack));
(gdb) c
Continuing.
 
Breakpoint 2, pushdup () at decompile.c:601
601     Stack = t;
(gdb) l
596     }
597     t = calloc(1,sizeof(*Stack));
598     t->type = Stack->type;
599     t->val =  Stack->val;
600     t->next = Stack;
601     Stack = t;
602 }
603
604
605 void
(gdb) p t->val
$1 = (struct SWF_ACTIONPUSHPARAM *) 0x606000000320

Now I can explain why pop is returning twice the exact same result: It's because pushdup was called, and pushdup duplicates the top stack entry. In the case of libming, pushdup was implemented to do a shallow copy, not deep copy. Unfortunately I can't tell whether this behavior is right or not, because the specification is very vague about it:

ActionPushDuplicate pushes a duplicate of top of stack (the current return value) to the stack.

@hlef
Copy link
Contributor

hlef commented Apr 1, 2018

Well, after some careful thoughts on the subject, I think that shallow copy is wrong in this case.

In the SWF specification, a String is defined as a sequential list of bytes terminated by the null character byte. This means that whenever a String is at the top of the stack, ActionPushDuplicate should push a duplicate of the list of bytes, not a pointer/reference to the list.

i.e. pushdup should perform a deep copy.

I'll update the PR accordingly.

hlef added a commit to hlef/libming that referenced this issue Apr 1, 2018
Until now, the element duplication in pushdup was performed via
t->val = Stack->val.

While this is perfectly fine for integer/double/register values,
this may create nasty, hard to debug issues with Strings. In fact,
when called with a String at the top of the stack, pushdup would
only push *a reference* to the same String element (shallow copy),
later allowing to modify several stack elements at once, which may
potentially lead to NULL pointer dereferences or any other
unspecified impact.

In this patch we implement deep copy in pushdup:
* If the type of the stack element is 's' (for String), we
  allocate a new buffer and copy the String into it.
* Otherwise we simply proceed as before, that is we do t->val = Stack->val
  which is perfectly fine since we are not dealing with pointers.

This patch is the last part of the patch for libming#121 (fixes libming#121), which
should now be completely fixed.
hlef added a commit to hlef/libming that referenced this issue Apr 1, 2018
Until now, the element duplication in pushdup was performed via
t->val = Stack->val.

While this is perfectly fine for integer/double/register values,
this may create nasty, hard to debug issues with Strings. In fact,
when called with a String at the top of the stack, pushdup would
only push *a reference* to the same String element (shallow copy),
later allowing to modify several stack elements at once, which may
potentially lead to NULL pointer dereferences or any other
unspecified impact.

In this patch we implement deep copy in pushdup:
* If the type of the stack element is 's' (for String), we
  allocate a new buffer and copy the String into it.
* Otherwise we simply proceed as before, that is we do t->val = Stack->val
  which is perfectly fine since we are not dealing with pointers.

This patch is the last part of the patch for libming#121 (fixes libming#121), which
should now be completely fixed.
@hlef
Copy link
Contributor

hlef commented Apr 1, 2018

FTR, this issue was assigned id CVE-2018-9165.

@strk strk closed this as completed in #127 May 20, 2018
strk pushed a commit that referenced this issue May 20, 2018
Whenever getString or getName are called with an act such that act->p.String
is a NULL pointer, a NULL pointer dereference might happen
(strlen(act->p.string) is called).

In this commit we add checks at the beginning of the PUSH_STRING block so
that a warning is displayed and an empty string is returned in this case.

This patch fixes #121.
strk pushed a commit that referenced this issue May 20, 2018
Until now, the element duplication in pushdup was performed via
t->val = Stack->val.

While this is perfectly fine for integer/double/register values,
this may create nasty, hard to debug issues with Strings. In fact,
when called with a String at the top of the stack, pushdup would
only push *a reference* to the same String element (shallow copy),
later allowing to modify several stack elements at once, which may
potentially lead to NULL pointer dereferences or any other
unspecified impact.

In this patch we implement deep copy in pushdup:
* If the type of the stack element is 's' (for String), we
  allocate a new buffer and copy the String into it.
* Otherwise we simply proceed as before, that is we do t->val = Stack->val
  which is perfectly fine since we are not dealing with pointers.

This patch is the last part of the patch for #121 (fixes #121), which
should now be completely fixed.
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

Successfully merging a pull request may close this issue.

3 participants