#27 flip4mac wmv
Landon Fuller
Landon Fuller
annotated it as a step-through of the PoC wmv I sent.
My first question is 0x11afe3e6. Why cmovle? It almost looks like the
bug is accidental MAX() instead of MIN().
The caller of this function can be disassembled from 0x11b01708 to
0x11b017ed (i386). I'm going to start digging there next to see where
the destination argument comes from.
(gdb) disassemble 0x11afe3cc 0x11afe40b
Dump of assembler code from 0x11afe3cc to 0x11afe40b:
0x11afe3cc <MjpgDecompressorComponentDispatch+1537746>: push %ebp
0x11afe3cd <MjpgDecompressorComponentDispatch+1537747>: mov %esp,%ebp
0x11afe3cf <MjpgDecompressorComponentDispatch+1537749>: push %edi
0x11afe3d0 <MjpgDecompressorComponentDispatch+1537750>: push %esi
0x11afe3d1 <MjpgDecompressorComponentDispatch+1537751>: sub $0x10,%
esp
0x11afe3d4 <MjpgDecompressorComponentDispatch+1537754>: mov 8(%
ebp),%esi <-- function arg1, a struct pointer
0x11afe3d7 <MjpgDecompressorComponentDispatch+1537757>: mov 16(%
ebp),%edx <-- function arg3, ??, 0x10
0x11afe3da <MjpgDecompressorComponentDispatch+1537760>: mov 20(%
esi),%eax <-- struct member ??, 0x0
0x11afe3dd <MjpgDecompressorComponentDispatch+1537763>: mov 16(%
esi),%ecx <-- struct member, our provided length. (0xff000050)
0x11afe3e0 <MjpgDecompressorComponentDispatch+1537766>: sub %eax,%
ecx <-- 0x0 - 0xff000050 == 0xff000050
0x11afe3e2 <MjpgDecompressorComponentDispatch+1537768>: cmp %edx,%
ecx <-- cmp(0x10, 0xff000050)
0x11afe3e4 <MjpgDecompressorComponentDispatch+1537770>: mov %edx,%
edi <-- mov 0x10 to $edi
0x11afe3e6 <MjpgDecompressorComponentDispatch+1537772>: cmovle %ecx,%
edi <-- if (%edx < %ecx) mov %ecx,%edi [%edi is now 0xff000050. Why a
*less than* comparison??)
0x11afe3e9 <MjpgDecompressorComponentDispatch+1537775>: add 12(%
esi),%eax
0x11afe3ec <MjpgDecompressorComponentDispatch+1537778>: mov %edi,8
(%esp) <-- memcpy length, 0xff000050
0x11afe3f0 <MjpgDecompressorComponentDispatch+1537782>: mov %eax,4
(%esp) <-- memcpy source, function arg1+12
0x11afe3f4 <MjpgDecompressorComponentDispatch+1537786>: mov 12(%
ebp),%eax
0x11afe3f7 <MjpgDecompressorComponentDispatch+1537789>: mov %eax,(%
esp) <-- memcpy dest, function arg2
0x11afe3fa <MjpgDecompressorComponentDispatch+1537792>: call
0x11b7105a <dyld_stub_memcpy> memcpy(12(%ebp), struct member arg1+12,
0xff000050) <-- kaboom
0x11afe3ff <MjpgDecompressorComponentDispatch+1537797>: add %edi,20
(%esi)
0x11afe402 <MjpgDecompressorComponentDispatch+1537800>: mov %edi,%eax
0x11afe404 <MjpgDecompressorComponentDispatch+1537802>: add $0x10,%
esp
0x11afe407 <MjpgDecompressorComponentDispatch+1537805>: pop %esi
0x11afe408 <MjpgDecompressorComponentDispatch+1537806>: pop %edi
0x11afe409 <MjpgDecompressorComponentDispatch+1537807>: pop %ebp
0x11afe40a <MjpgDecompressorComponentDispatch+1537808>: ret
End of assembler dump.
Landon Fuller
On Jan 28, 2007, at 4:20 PM, Landon Fuller wrote:
> The caller of this function can be disassembled from 0x11b01708 to
> 0x11b017ed (i386). I'm going to start digging there next to see
> where the destination argument comes from.
Sorry, wrong address. Here's a correct backtrace:
#0 0x11afe3e0 in MjpgDecompressorComponentDispatch () <-- in the
middle of the function that calls memcpy(). I'll call this function
copier()
#1 0x11b02504 in MjpgDecompressorComponentDispatch () <-- next
caller, disassembled below. We'll call this copydriver()
#2 0x11aff955 in MjpgDecompressorComponentDispatch ()
#3 0x1196d617 in MmsDataHandlerComponentDispatch ()
#4 0x11b01791 in MjpgDecompressorComponentDispatch ()
#5 0x1195dc82 in AsfMovieImportComponentDispatch ()
#6 0x11963c28 in AsfMovieImportComponentDispatch ()
#7 0x019a1f6e in CallComponentFunctionCommon ()
#8 0x1195b005 in AsfMovieImportComponentDispatch ()
#9 0x019a1a3c in CallComponentDispatch ()
Note that copydriver() allocates 0x10 on the stack at 0x11b024e1, and
then passes 0x10 as the third argument to copier() at 0x11b024f2.
(gdb) disassemble 0x11b024dc 0x11b02514
Dump of assembler code from 0x11b024dc to 0x11b02514:
0x11b024dc <MjpgDecompressorComponentDispatch+1554402>: push %ebp
0x11b024dd <MjpgDecompressorComponentDispatch+1554403>: mov %esp,%ebp
0x11b024df <MjpgDecompressorComponentDispatch+1554405>: push %edi
0x11b024e0 <MjpgDecompressorComponentDispatch+1554406>: push %esi
0x11b024e1 <MjpgDecompressorComponentDispatch+1554407>: sub $0x10,%
esp
0x11b024e4 <MjpgDecompressorComponentDispatch+1554410>: mov 8(%
ebp),%esi
0x11b024e7 <MjpgDecompressorComponentDispatch+1554413>: mov 12(%
ebp),%edi
0x11b024ea <MjpgDecompressorComponentDispatch+1554416>: lea 32(%
esi),%edx
0x11b024ed <MjpgDecompressorComponentDispatch+1554419>: mov 8(%
edi),%eax
0x11b024f0 <MjpgDecompressorComponentDispatch+1554422>: mov (%eax),
%ecx
0x11b024f2 <MjpgDecompressorComponentDispatch+1554424>: movl $0x10,8
(%esp)
0x11b024fa <MjpgDecompressorComponentDispatch+1554432>: mov %edx,4
(%esp)
0x11b024fe <MjpgDecompressorComponentDispatch+1554436>: mov %eax,(%
esp)
0x11b02501 <MjpgDecompressorComponentDispatch+1554439>: call *40(%ecx)
0x11b02504 <MjpgDecompressorComponentDispatch+1554442>: cmp $0x10,%
eax
0x11b02507 <MjpgDecompressorComponentDispatch+1554445>: je
0x11b02515 <MjpgDecompressorComponentDispatch+1554459>
0x11b02509 <MjpgDecompressorComponentDispatch+1554447>: mov
$0xffffb563,%eax
0x11b0250e <MjpgDecompressorComponentDispatch+1554452>: add $0x10,%
esp
0x11b02511 <MjpgDecompressorComponentDispatch+1554455>: pop %esi
0x11b02512 <MjpgDecompressorComponentDispatch+1554456>: pop %edi
0x11b02513 <MjpgDecompressorComponentDispatch+1554457>: pop %ebp
End of assembler dump.
(gdb)
William A. Carrel
> 0x11afe3e2 <MjpgDecompressorComponentDispatch+1537768>: cmp %edx,%
> ecx <-- cmp(0x10, 0xff000050)
> 0x11afe3e4 <MjpgDecompressorComponentDispatch+1537770>: mov %edx,%
> edi <-- mov 0x10 to $edi
> 0x11afe3e6 <MjpgDecompressorComponentDispatch+1537772>: cmovle %ecx,%
> edi <-- if (%edx < %ecx) mov %ecx,%edi [%edi is now 0xff000050. Why a
> *less than* comparison??)
Actually %edx<=%ecx and since this is CMOVLE it's presuming these are
signed. So we get 0xff000050 - 0x10 to set the flags in CMP. This
might want to be CMOVBE instead, which would guarantee a memcpy of
size arg3 or less.
William A. Carrel
> On 1/28/07, Landon Fuller <lan...@bikemonkey.org> wrote:
> > 0x11afe3e2 <MjpgDecompressorComponentDispatch+1537768>: cmp %edx,%
> > ecx <-- cmp(0x10, 0xff000050)
> > 0x11afe3e4 <MjpgDecompressorComponentDispatch+1537770>: mov %edx,%
> > edi <-- mov 0x10 to $edi
> > 0x11afe3e6 <MjpgDecompressorComponentDispatch+1537772>: cmovle %ecx,%
> > edi <-- if (%edx < %ecx) mov %ecx,%edi [%edi is now 0xff000050. Why a
> > *less than* comparison??)
>
> Actually %edx<=%ecx and since this is CMOVLE it's presuming these are
> signed. So we get 0xff000050 - 0x10 to set the flags in CMP. This
> might want to be CMOVBE instead, which would guarantee a memcpy of
> size arg3 or less.
This seems especially the case since the arguments are going to
memcpy. Sending a signed length to memcpy doesn't strike me as an
astoundingly good move.
Landon Fuller
On Jan 28, 2007, at 5:11 PM, William A. Carrel wrote:
> This seems especially the case since the arguments are going to
> memcpy. Sending a signed length to memcpy doesn't strike me as an
> astoundingly good move.
An integer-overflow safe reimplemention:
http://moab-fixes.googlecode.com/svn/trunk/flip4mac_asf_handler.c
This works with valid .wmvs, and safely handles too-long length fields.
I haven't looked at PPC yet. I sure hope I can tweak the function
offset and re-use the fix.
-landonf