#19 - Panic's Transmit

[Landon Fuller]

From MoAB's advisory:

(gdb) back
#0 0x900257e2 in flockfile ()
#1 0x900107f2 in vfprintf ()
#2 0x00190cf3 in PrintF ()
#3 0x00187bc5 in FTPInitialLogEntry ()
#4 0x00187d10 in FTPOpenHost ()
#5 0x00150626 in -[FTPConnectionWorker _connectTo:port:user:password:

initialPath:localPath:redial:listFiles:encoding:] ()
#6 0x90a58c56 in objc_msgSendv ()
#7 0x925f443e in -[NSInvocation invoke] ()
#8 0x9261a433 in -[NSInvocation invokeWithTarget:] ()
#9 0x001611d3 in -[AbstractConnectionWorker workerThreadWithPorts:] ()
#10 0x925ed36c in forkThreadForFunction ()
#11 0x90023d87 in _pthread_body ()

I've started stepping through -[FTPConnectionWorker
_connectTo:port:user:password:
initialPath:localPath:redial:listFiles:encoding:] () -- I have a
suspicion that things go haywire before FTPOpenHost(), namely, I
crash here:

#0 0x9495a83d in SSL_shutdown ()
#1 0x0018cc0b in TLSClose ()
#2 0x0014ff05 in -[FTPConnectionWorker
_connectTo:port:user:password:initialPath:localPath:redial:listFiles:enc
oding:] ()

But I could be wrong.

William Carrel and I started talking briefly about this one, I think
the general consensus was:
1) Verify the location of the actual overflow
2) Check the stack allocation, try to determine the size of the buffer.

There's a simple DoS reproduction here:
http://landonf.bikemonkey.org/static/moab-tests/regression.html

-landonf

[William A. Carrel]

I see the same crash. The stack is overwritten with A's in all sorts
of places by the time we get to this crash.

On the initial call there the args look like
(gdb) x/9x $ebp+0x8
0xb0332d90: 0x01983800 0x0019d728 0x160c0f10 0x00000000
0xb0332da0: 0x00102c94 0x00102c94 0xa080b988 0x1602a5f0
0xb0332db0: 0x00000001

The target string is a CFConstantString(?!) at 16(%ebp) 0x160c0f10.

The TLSClose crash is not all that far into the FTPConnectionWorker call.

It does something with the target string here
0x0014fdfc <-[FTPConnectionWorker
_connectTo:port:user:password:initialPath:localPath:redial:listFiles:encoding:]+67>:
mov %eax,4(%esp)
0x0014fe00 <-[FTPConnectionWorker
_connectTo:port:user:password:initialPath:localPath:redial:listFiles:encoding:]+71>:
mov 16(%ebp),%eax
0x0014fe03 <-[FTPConnectionWorker
_connectTo:port:user:password:initialPath:localPath:redial:listFiles:encoding:]+74>:
mov %eax,(%esp)
0x0014fe06 <-[FTPConnectionWorker
_connectTo:port:user:password:initialPath:localPath:redial:listFiles:encoding:]+77>:
call 0x1af005 <dyld_stub_objc_msgSend>

and calls strcpy(?!) a few times before it gets to TLSClose().

More as I dig further after lunch...
--
wac

[William A. Carrel]

so there are a couple of [string retains];
and then a -[FTPConnectionWorker encodeString:]
which calls -[NSString(NSStringOtherEncodings)
dataUsingEncoding:allowLossyConversion:]
...morestuff...
TLSClose() is later. it's first argument is already 0x41414141 by the
time we get there so this damage is coming before that...

[William A. Carrel]

So the damage is happening in [FTPConnectionWorker encodeString:] it
should be returning a C string to be used by a subsequence strcpy() as
src but returns something that isn't terminated right. This may be a
separate bug from the one reported in MoAB.

Dump of assembler code for function -[FTPConnectionWorker
_connectTo:port:user:password:initialPath:localPath:redial:listFiles:encoding:]:
...
0x0014feb9 <+256>: mov 16(%ebp),%edx
0x0014febc <+259>: mov %edx,8(%esp)
0x0014fec0 <+263>: mov 375505(%ebx),%eax
0x0014fec6 <+269>: mov %eax,4(%esp)
0x0014feca <+273>: mov 8(%ebp),%ecx
0x0014fecd <+276>: mov %ecx,(%esp)
[FTPConnectionWorker encodeString:(CFConstantString of "AAAAAAAAAAA...")]
0x0014fed0 <+279>: call 0x1af005 <dyld_stub_objc_msgSend>

# String at (%eax) is not correctly terminated, and it will be the source
# for the upcoming strcpy... :(
# 0x19120f0: 0x41414141 0xd0f80041 0x65fc9afa 0x0000ffff

0x0014fed5 <+284>: mov %eax,4(%esp) # arg 2 for next function
0x0014fed9 <+288>: mov 8(%ebp),%eax
0x0014fedc <+291>: add $0x4c,%eax
0x0014fedf <+294>: mov %eax,(%esp) # arg 1 for next function
strcpy(bad string, ...)
0x0014fee2 <+297>: call 0x1af02d <dyld_stub_strcpy>
0x0014fee7 <+302>: mov 8(%ebp),%edx
0x0014feea <+305>: mov 16(%edx),%eax
0x0014feed <+308>: mov %eax,332(%edx)
0x0014fef3 <+314>: mov 1328(%edx),%eax
0x0014fef9 <+320>: test %eax,%eax
0x0014fefb <+322>: je 0x14ff12 <-[FTPConnectionWorker
_connectTo:port:user:password:initialPath:localPath:redial:listFiles:encoding:]+345>
%eax contains trash
0x0014fefd <+324>: mov %eax,(%esp)
(%esp) is trashed by here.
0x0014ff00 <+327>: call 0x18cbfa <TLSClose>
crash in TLSClose

[Landon Fuller]

On Jan 20, 2007, at 4:01 PM, William A. Carrel wrote:

>
> so there are a couple of [string retains];
> and then a -[FTPConnectionWorker encodeString:]
> which calls -[NSString(NSStringOtherEncodings)
> dataUsingEncoding:allowLossyConversion:]
> ...morestuff...
> TLSClose() is later. it's first argument is already 0x41414141 by the
> time we get there so this damage is coming before that...

(gdb) po *((id *) ($ebp+16))
AAAAAAAAAAAAAAAAAAA...

So it calls -[self encodeString: ($ebp+16)], which returns a const
char * string pointer in $eax, and then comes a strcpy:

0x0014fed5 <-[FTPConnectionWorker

_connectTo:port:user:password:initialPath:localPath:redial:listFiles:enc

oding:]+284>: mov %eax,4(%esp) <- copy the const char * hostname
to 2nd arg
0x0014fed9 <-[FTPConnectionWorker

_connectTo:port:user:password:initialPath:localPath:redial:listFiles:enc

oding:]+288>: mov 8(%ebp),%eax <- copy self pointer to %eax
0x0014fedc <-[FTPConnectionWorker

_connectTo:port:user:password:initialPath:localPath:redial:listFiles:enc

oding:]+291>: add $0x4c,%eax <- add 76 to self pointer
0x0014fedf <-[FTPConnectionWorker

_connectTo:port:user:password:initialPath:localPath:redial:listFiles:enc

oding:]+294>: mov %eax,(%esp) <-- copy self+76 to 1st arg
0x0014fee2 <-[FTPConnectionWorker

_connectTo:port:user:password:initialPath:localPath:redial:listFiles:enc

oding:]+297>: call 0x1af02d <dyld_stub_strcpy> <-- call strcpy(self
+76, hostname)

@interface FTPConnectionWorker : AbstractConnectionWorker
{
struct FTPConnectionInfo ci;
...
}

struct FTPConnectionInfo {
char magic[16];
char host[64];
char user[64];
char pass[64];
char acct[64];
...
}

[Landon Fuller]

On Jan 20, 2007, at 4:36 PM, William A. Carrel wrote:

> # String at (%eax) is not correctly terminated, and it will be the
> source
> # for the upcoming strcpy... :(
> # 0x19120f0: 0x41414141 0xd0f80041 0x65fc9afa
> 0x0000ffff

That's wacky! Are you sure?

My test URL is 4853 characters long:

0x00151bc6 in -[FTPConnectionWorker encodeString:] ()
(gdb) print ((char *) $eax)[4853]
$47 = 0 '\0'

[Landon Fuller]

On Jan 20, 2007, at 4:36 PM, William A. Carrel wrote:

> 0x0014fed5 <+284>: mov %eax,4(%esp) # arg 2 for next function
> 0x0014fed9 <+288>: mov 8(%ebp),%eax
> 0x0014fedc <+291>: add $0x4c,%eax
> 0x0014fedf <+294>: mov %eax,(%esp) # arg 1 for next function
> strcpy(bad string, ...)
> 0x0014fee2 <+297>: call 0x1af02d <dyld_stub_strcpy>

I just tested with a really long username -- it looks like this
strcpy stomps on the instance variables -- host, username, or
password. The struct kept as the first instance variable:

[William A. Carrel]

On 1/20/07, Landon Fuller <lan...@bikemonkey.org> wrote:
>

You're right. I think I followed the wrong pointer here.
--
wac

[Landon Fuller]

On Jan 20, 2007, at 4:01 PM, William A. Carrel wrote:

> which calls -[NSString(NSStringOtherEncodings)
> dataUsingEncoding:allowLossyConversion:]

Did you see what arguments are being used here? I'm thinking:

-[FTPConnectionWorker _connectTo:port:user:password:
initialPath:localPath:redial:listFiles:encoding:]

In the guard, check the length of:
- user (NSString)
- password (NSString)
- connectTo host (NSString)

pseudo-code:

NSData *data = [user dataUsingEncoding: ascii? allowLossyConversion: ?]
len = [data bytes];
if (len > 63) {
... replace with @"Safe String" ...
}

-landonf

[William A. Carrel]

On 1/20/07, Landon Fuller <lan...@bikemonkey.org> wrote:
>

> On Jan 20, 2007, at 4:01 PM, William A. Carrel wrote:
>
> > which calls -[NSString(NSStringOtherEncodings)
> > dataUsingEncoding:allowLossyConversion:]
>
> Did you see what arguments are being used here? I'm thinking:

NSISOLatin1StringEncoding (= 5), True (= 1)

> -[FTPConnectionWorker _connectTo:port:user:password:
> initialPath:localPath:redial:listFiles:encoding:]
> In the guard, check the length of:
> - user (NSString)
> - password (NSString)
> - connectTo host (NSString)
>
> pseudo-code:
>
> NSData *data = [user dataUsingEncoding: ascii? allowLossyConversion: ?]
> len = [data bytes];
> if (len > 63) {
> ... replace with @"Safe String" ...
> }

Hmm, yeah. I'm not sure what the value of @"Safe String" would be.
Maybe @"Host name too long to be safe for Transmit."

Or to be exactly 63 characters and suggest a fix...
@"Host name too long to be safe for Transmit. Panic: Use strncpy."

--
wac

[William A. Carrel]

Okay I've committed r48 (and r49 to remove a debugging print out)
which has transmit_ftp_handler.m which pastes over #19.

Users see a dialog that says they couldn't connect to host "URL part

too long to be safe for Transmit. Panic: Use strncpy."

The version checking has to be a little strange compared to the other
code because Panic have non-ASCII characters in their Info.plist
version section for some reason.

I've also committed the bom-shelter script that i had mentioned
earlier here and on my blog.

On 1/20/07, William A. Carrel <will...@carrel.org> wrote:

[Eric Hall]

On Sat, Jan 20, 2007 at 07:02:56PM -0800, William A. Carrel wrote:
>
> Okay I've committed r48 (and r49 to remove a debugging print out)
> which has transmit_ftp_handler.m which pastes over #19.
>
> Users see a dialog that says they couldn't connect to host "URL part
> too long to be safe for Transmit. Panic: Use strncpy."

Recommending strlcpy is a better choice, as it (mostly) guarantees
a NULL-terminated string.

[snip]

[William A. Carrel]

Indeed. It would be nice to add something like OpenBSD's "warning:
strcpy() is almost always misused, please use strlcpy()" for all OS X
developers. Microsoft, to their credit, deprecated strcpy() because of
these sorts of issues.

"warning: gun aimed at foot, please aim within the firing range."
--
wac