I've done a fair bit of digging, but need to turn in for the night.
Find attached the disassembly of the particular function
[SUAppDelegate application:id openFile:id]
that is at the top of the backtrace in the advisory.

NSRunCriticalAlertPanel seems to not be handling the input very safely
for its part. I'm not sure this function is actually at fault. The
places where it is working with what I think is the filename of the
file it is opening look to be fairly safe. I reserve the right to be
wrong though.

It appears to be passed as the contents for part of the format string
which is derived from localizedStringForKey:value:table: rather than
the format string itself. This is then passed as the "msg" argument to
NSRunCrit...Panel

I think it may be possible to mitigate this by patching
NSRunCriticalAlertPanel to properly escape % in it's arguments if it's
going to have to do formatty string things with them.

Odds are high that other programs may not be calling this function
cleanly either. The documentation at
http://developer.apple.com/documentation/Cocoa/Reference/ApplicationK...
doesn't give any hints that format strings inside the strings you pass
through to these functions might get interpreted.
--
wac

SUCallDump.txt 7K Download

Ack, at 1/24/07, William A. Carrel said:

This is indeed the correct method with the problem.

Then I'll have to take you up on that.

NSRun*AlertPanel is not at fault. It is documented what it does. And
msg is a format string. The problem is that many cocoa developers
seem to not be familiar with format strings and format arguments when
*not* in the NSLog() or stringWithFormat: context. Hell, this SWUP
bug is a perfect example of this.

The issue is the fact that SWUP gets the localized format of
"Software Update can't open the file "%@"." (which is fine) then SWUP
calls [NSString stringWithFormat:localizedString, [filepath
lastPathComponent]];

Then SWUP passes this as the second arg to NSRunCriticalAlertPanel,
which it should not be doing.

It can skip the call to stringWithFormat: completely and just call
NSRunCriticalAlertPanel(@"The catalog file could not be opened.",
"Software Update can't open the file "%@".", @"OK", NULL, NULL,
[filepath lastPathComponent]);

For Apple to fix this, all they'd have to do is nix the call to
stringWithFormat: and pass the filename as stated directly above.

An outside fixer would probably have to patch - (BOOL) [SUAppDelegate
application:(NSApplication *)sender openFile:(NSString *)filename] to
check the extension of the filename. If it isn't @"sucatalog", then
modificate the filename string to convert all the %'s in the
lastPathComponent portion to %%. Then just call the original function.
--

Sincerely,
Rosyna Keller
Technical Support/Carbon troll/Always needs a hug

Unsanity: Unsane Tools for Insanely Great People

It's either this, or imagining Phil Schiller in a thong.

On 1/25/07, Rosyna <ros...@gmail.com> wrote:

Yeah, it turns out this "feature" is documented up through two layers
of indirection in the API docs with NSBeginAlertSheet that has the msg
argument a little closer to the var args that can impact it. I wonder
how many developers actually take advantage of the ability to print
format strings here? Wouldn't it be safer to just take this ability
away in the name of safety?

One of the problems were facing here is the fact that varargs ObjC
calls have no argument count. In order to suss that out we'd need our
own varargs msgSend that can look at the stack pointers to try and
determine varargc and do the right thing for functions that take
format string args (make sure the format string matches up with the
optional var args at the end of the call). Without this, we have
little way of knowing how many arguments are safe to read which turns
into a eleventy jillion security problems.

Patching this function as you suggest will break its ability to work
with filenames with % in them, since they won't actually be found on
disk. The other option is to include the function's assembler code in
the patch up to the point where we need to call NSRun...Panel and
change that call to hit a helper patch function we've written that can
do the % -> %% substitution.

I personally would prefer globally patching NSRun...Panel to disallow
the varargs because I suspect that feature isn't widely used. For
instance, Apple isn't using it here, evidenced by the stringWithFormat
call rather than passing that format string through to NSRun...Panel.
And the documentation of this feature is only mentioned if people dig
a couple layers deeper. And the failure mode is that some error
dialogs will show "File %s couldn't be loaded." Seeing that message
would seem to be a lot more favorable than having a format string vuln
that can potentially do Bad Things™.
--
wac

On 25/01/07, William A. Carrel <willia...@carrel.org> wrote:

I have used this ability before, it can be handy. There are also clear
notes about msg being a format string in the headers. Arguing about
whether this was a poor design decision or not is moot, since it
obviously can't be changed now. Note that all the alert-related
functions in AppKit are pretty much superceded by the NSAlert class,
which is more flexible in a number of ways (and whose convenience
constructor has an argument informativeTextWithFormat: which should be
a fairly clear and obvious indicator of the argument's true nature).

Why not just context patch NSRunAlertPanel within the vulnerable
function in Software Update with a version which escapes the %?

 -- Finlay

Finlay Dobbie:

Given that NSAlert exists, and provides a clearer indication of the
presence of a format string, it would be possible and reasonable to mark
NSRun*AlertPanel as deprecated (thus generating a warning when it is
used), and perhaps to remove it in 64-bit Cocoa. I have filed an
enhancement request with Apple to this effect. (It is of course possible
this has already been done, but anyone knowing whether this is the case
can't comment.)

--
Jens Ayton

Sed quis custodiet ipsos custodes?

On 1/25/07, Finlay Dobbie <finlay.dob...@gmail.com> wrote:

Yeah, but there are no such notes here...
http://developer.apple.com/documentation/Cocoa/Reference/ApplicationK...
...which I think is part of the reason that this is being used
unsafely. That the new(?) NSAlert API has more blunt hints in the
arguments that you're looking at a format string is a big plus.
Anyway, you're right, this conversation is just ranting about the API
docs which isn't helping us any.

That was basically what I'm suggesting above. I suppose it could do
some voodoo to examine the stack to see that it was being called from
the known bad place, or it could just do the %->%% substitution for
every call to NSRunCriticalAlertPanel (or whatever that function was)
in Software Update.app.

I guess my point was that Software Update.app probably is not the only
application that makes this mistake and my own thought is defaulting
to considering the applications to be using this wrong (not doubling
up % and therefore needing this call patched) unless there is good
reason to think that they are doing it right. Whether this API was a
good call originally is, I think, orthogonal to whether all programs
calls to this function should be patched. The real consideration is
whether enough programs have used this API correctly so that having
some error messages that have %s and %d in them where they might have
originally had useful information is bad enough to justify not
patching this problem.

In dreamland, there would some sort of "taint" mechanism to find out
if the format string was trustworthy. That is probably also outside
the scope of this discussion.
--
wac

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

So just to follow up the online conversation for those keeping score
on the group here. There is a  feeling that it may be best to do a
patch only in the context of this call, patching the NSRun... call on
the way into [SUAppDelegate application:id openFile:id], and then
unpatching it on the way back out again.

I guess we can deal with other applications that use this unsafely as
they pop up.
--
wac

Well, no, it wouldn't break it. That's what the check for the
extension of "sucatalog" is for. If the file doesn't have that
extension, SWUP never actually accesses the file at all and just
displays the file name in an error dialog.

Try dragging any arbitrary file onto the SWUP application while
holding down the command and option keys on the keyboard.

I guess my point is that the disassembly you posted never shows the
file being accessed if it doesn't have a sucatalog extension. So
changing the file path is safe since it is just being using for
displaying an error.

Ack, at 1/25/07, William A. Carrel said:

--

Sincerely,
Rosyna Keller
Technical Support/Carbon troll/Always needs a hug

Unsanity: Unsane Tools for Insanely Great People

It's either this, or imagining Phil Schiller in a thong.

ummm, how many ISVs supply updates via SWU ?
I haven't seen any, but then I may not have looked close enough.
SWU.app in normal operation calls the mother ship at
Cupertino, checks the catalog, and then if the user chooses
to install or download, is redirected to the nearest Akamai cache.

The desired files live inside subdirectories on Akamai, or
some other local SWUpdate server, where the subdirectory
name is a checksum of its contents. It seems to me that
the only way to inject a borked filename into SWU.app
in "normal" operation is to simultaneously carry out a
MIM attack on apple.com and/or some indeterminate cache server.

Yes, the FormatString problem needs fixing, but
in the short term is there any way to stop SWU.app
handling files that it didn't itself fetch from home?

or maybe I'm day-dreaming, again...

d

On Jan 25, 2007, at 3:58 PM, Rosyna wrote:

Commented assembly; looks like the case to me. Anyone want to review  
to make sure I'm not missing a jmp somewhere?
If not, then I'll roll together the fix Rosyna suggested:

0x00005ece:     push   %ebp
0x00005ecf:     mov    %esp,%ebp
0x00005ed1:     push   %edi
0x00005ed2:     push   %esi
0x00005ed3:     push   %ebx
0x00005ed4:     sub    $0x3c,%esp
0x00005ed7:     mov    20(%ebp),%edi
0x00005eda:     mov    0x11704,%eax
0x00005edf:     mov    %eax,4(%esp)
0x00005ee3:     mov    %edi,(%esp)
0x00005ee6:     call   0x10057 <dyld_stub_objc_msgSend> -[NSString  
pathExtension] for file path
0x00005eeb:     movl   $0xf36c,8(%esp) <--                   arg3, CFSTR("sucatalog")
0x00005ef3:     mov    71108,%edx <--                                arg2, isEqualToString:
0x00005ef9:     mov    %edx,4(%esp)
0x00005efd:     mov    %eax,(%esp) <--                               arg1, result of pathExtensions
0x00005f00:     call   0x10057 <dyld_stub_objc_msgSend> -[NSString  
isEqualToString:]
0x00005f05:     test   %al,%al
0x00005f07:     je     0x5fee <--                                    if file extension is not  
sucatalog, jump to 0x5fee (error handler)
0x00005f0d:     mov    %edi,8(%esp)
0x00005f11:     mov    0x11700,%eax
0x00005f16:     mov    %eax,4(%esp)
0x00005f1a:     mov    0x11940,%eax
0x00005f1f:     mov    %eax,(%esp)
0x00005f22:     call   0x10057 <dyld_stub_objc_msgSend>
0x00005f27:     mov    71420,%edx
0x00005f2d:     mov    %edx,4(%esp)
0x00005f31:     mov    %eax,(%esp)
0x00005f34:     call   0x10057 <dyld_stub_objc_msgSend>
0x00005f39:     mov    %eax,%edi
0x00005f3b:     mov    0x116f8,%eax
0x00005f40:     mov    %eax,4(%esp)
0x00005f44:     mov    0x1198c,%eax
0x00005f49:     mov    %eax,(%esp)
0x00005f4c:     call   0x10057 <dyld_stub_objc_msgSend>
0x00005f51:     mov    %eax,%esi
0x00005f53:     mov    0x10038,%eax
0x00005f58:     mov    (%eax),%eax
0x00005f5a:     mov    %eax,8(%esp)
0x00005f5e:     mov    0x116f4,%eax
0x00005f63:     mov    %eax,4(%esp)
0x00005f67:     mov    %esi,(%esp)
0x00005f6a:     call   0x10057 <dyld_stub_objc_msgSend>
0x00005f6f:     mov    %eax,%edx
0x00005f71:     test   %eax,%eax
0x00005f73:     je     0x5f83
0x00005f75:     mov    0x116f0,%eax
0x00005f7a:     mov    %eax,4(%esp)
0x00005f7e:     mov    %edx,(%esp)
0x00005f81:     jmp    0x5f94
0x00005f83:     mov    0x11654,%eax
0x00005f88:     mov    %eax,4(%esp)
0x00005f8c:     mov    0x11988,%eax
0x00005f91:     mov    %eax,(%esp)
0x00005f94:     call   0x10057 <dyld_stub_objc_msgSend>
0x00005f99:     mov    %eax,%ebx
0x00005f9b:     movl   $0xf37c,12(%esp)
0x00005fa3:     mov    %edi,8(%esp)
0x00005fa7:     mov    0x116ec,%eax
0x00005fac:     mov    %eax,4(%esp)
0x00005fb0:     mov    %ebx,(%esp)
0x00005fb3:     call   0x10057 <dyld_stub_objc_msgSend>
0x00005fb8:     mov    0x10038,%eax
0x00005fbd:     mov    (%eax),%eax
0x00005fbf:     mov    %eax,12(%esp)
0x00005fc3:     mov    %ebx,8(%esp)
0x00005fc7:     mov    0x116e8,%eax
0x00005fcc:     mov    %eax,4(%esp)
0x00005fd0:     mov    %esi,(%esp)
0x00005fd3:     call   0x10057 <dyld_stub_objc_msgSend>
0x00005fd8:     mov    0x1147c,%eax
0x00005fdd:     mov    %eax,4(%esp)
0x00005fe1:     mov    %ebx,(%esp)
0x00005fe4:     call   0x10057 <dyld_stub_objc_msgSend>
0x00005fe9:     jmp    0x612a

<-- Jumped if file extension is not sucatalog

0x00005fee:     mov    %edi,8(%esp) <--                      arg3, file path
0x00005ff2:     mov    0x116e4,%eax
0x00005ff7:     mov    %eax,4(%esp) <--                      arg2,  
productsWithContentsOfFile:
0x00005ffb:     mov    0x11984,%eax <--                      arg1, SUCatalog
0x00006000:     mov    %eax,(%esp)
0x00006003:     call   0x10057 <dyld_stub_objc_msgSend>
0x00006008:     test   %eax,%eax
0x0000600a:     jne    0x612e <-- If null was returned, jump to 0x612e
0x00006010:     mov    0x11490,%eax <--              arg2, mainBundle
0x00006015:     mov    %eax,4(%esp)
0x00006019:     mov    0x11930,%eax <--              arg1, application bundle?
0x0000601e:     mov    %eax,(%esp)
0x00006021:     call   0x10057 <dyld_stub_objc_msgSend>
0x00006026:     movl   $0x0,16(%esp) <--             arg5, NULL
0x0000602e:     movl   $0xf38c,12(%esp) <--          arg4, ""
0x00006036:     movl   $0xf39c,8(%esp) <--           arg3, "Software Update  
can't open the file <name>."
0x0000603e:     mov    70728,%edx <--                        arg2,  
localizedStringForKey:value:table:
0x00006044:     mov    %edx,4(%esp)
0x00006048:     mov    %eax,(%esp) <--                       arg1, the main bundle from  
above
0x0000604b:     call   0x10057 <dyld_stub_objc_msgSend>   -[NSbundle  
localizedStringForKey:value:table]
0x00006050:     mov    %eax,%esi
0x00006052:     mov    0x11490,%eax <--              arg2, "mainBundle"
0x00006057:     mov    %eax,4(%esp)
0x0000605b:     mov    0x11930,%eax <--              arg1, application main bundle
0x00006060:     mov    %eax,(%esp)
0x00006063:     call   0x10057 <dyld_stub_objc_msgSend>
0x00006068:     movl   $0x0,16(%esp) <--             arg5, NULL
0x00006070:     movl   $0xf38c,12(%esp) <--          arg4, ""
0x00006078:     movl   $0xf3ac,8(%esp) <--           arg3, "The catalog file  
could not be opened."
0x00006080:     mov    70728,%edx <--                        arg2,  
localizedStringForKey:value:table:
0x00006086:     mov    %edx,4(%esp)
0x0000608a:     mov    %eax,(%esp)                      arg1, application main bundle
0x0000608d:     call   0x10057 <dyld_stub_objc_msgSend> -[NSbundle  
localizedStringForKey:value:table]
0x00006092:     mov    %eax,-28(%ebp) <--            copy the result to our  
stack -28($ebp)
0x00006095:     mov    71976,%ebx <--                        NSString class (not used for  
the immediate msgSend call)
0x0000609b:     mov    0x116e0,%eax <--              arg2, lastPathComponent
0x000060a0:     mov    %eax,4(%esp)
0x000060a4:     mov    %edi,(%esp) <--                       arg1, file path
0x000060a7:     call   0x10057 <dyld_stub_objc_msgSend> -[NSString  
lastPathComponent]
0x000060ac:     mov    %eax,12(%esp) <--             arg4, Final path component  
of file path
0x000060b0:     mov    %esi,8(%esp) <--              arg3, localized "Software  
Update can't open the file <name>."
0x000060b4:     mov    0x11434,%eax <--              arg2, stringWithFormat:
0x000060b9:     mov    %eax,4(%esp)
0x000060bd:     mov    %ebx,(%esp) <--                       arg1, NSString class
0x000060c0:     call   0x10057 <dyld_stub_objc_msgSend> +[NSString  
stringWithFormat:...]
0x000060c5:     mov    %eax,%ebx <--                 Save the localized error  
message for later
0x000060c7:     mov    0x11490,%eax <--              arg2, "mainBundle"
0x000060cc:     mov    %eax,4(%esp)
0x000060d0:     mov    0x11930,%eax <--              arg1, application main bundle
0x000060d5:     mov    %eax,(%esp)
0x000060d8:     call   0x10057 <dyld_stub_objc_msgSend> <-- Retrieve  
the main bundle. Again. Yeesh.
0x000060dd:     movl   $0x0,16(%esp) <--             arg5, NULL
0x000060e5:     movl   $0xf38c,12(%esp) <--          arg4, @""
0x000060ed:     movl   $0xf3bc,8(%esp) <--           arg3, @"OK"
0x000060f5:     mov    70728,%edx <--                        arg2,  
"localizedStringForKey:value:table:"
0x000060fb:     mov    %edx,4(%esp)
0x000060ff:     mov    %eax,(%esp) <--     &n