"Save 38% now" C string aborts the program

user_1492173 0
Viewed 118

TL;DR

The C string "% n", with a space between '%' and 'n' characters (like in "Save 38% now" string) is treated as "%n", which is considered a vulnerability since latest OS releases, and it leads to abort the program.

(please see UPD 1 and UPD 2)

Background

I am investigating an issue with the logging in my app. The app receives some string from the server, which we log with function similar to NSLog. The format we call that function is:

MyLog(@"%@", message);

In one case, message contains text "Save 38% now". At this point the app crashes.

Issue

While looking into it I was able to pinpoint the issue. The full code is below.

#define FOO(FORMAT, ...) (\
{\
char *str;\
str = [[NSString stringWithFormat:FORMAT, ##VA_ARGS] UTF8String];\
str;\
}\
)\

#import "MyClass.h"

@implementation MyClass

- (instancetype)init
{
    self = [super init];
    if (self) {
        char *foo = FOO(@"%@", @"Save 38% now");
        printf(foo);
    }
    return self;
}

@end

Instantiate class anywhere,

MyClass *my = [[MyClass alloc] init];

, and the app will crash upon printf(foo); with the message:

%n used in a non-immutable format string

Basically, the string "% n", with a space between "%" and "n" is threated as "%n", which is considered a vulnerability since latest OS releases.

Discussion

I am still trying to proof my thoughts and find a way to solve the issue...

  1. I believe, the code responsible for the crash is this: https://opensource.apple.com/source/Libc/Libc-1244.30.3/stdio/FreeBSD/vfprintf.c Am I risgt?

  2. Why the space between two characters does not make any difference for the code?

  3. How could I possibly work around it leaving the same approach?

...so far, it looks like a bug.

Solution

Based on answers given by the community, right now my workaround to the issue is this (see also "UPD" part where I added the actual code where the issue originally occurred):

message = [plainText stringByReplacingOccurrencesOfString:@"%"
withString:@"%%" 
options:NSRegularExpressionSearch 
range:NSMakeRange(0, message.length)];

DebugOnlyLog(@"%@", message);

UPD 1

This is the code I use to log some

#import <os/log.h>

extern struct os_log_s _os_log_default;
extern __attribute__((weak)) void _os_log_internal(void *dso, os_log_t log, os_log_type_t type, const char *message, ...);

#define DebugOnlyLog(FORMAT, ...) \
void(*ptr_os_log_internal)(void *, __strong os_log_t, os_log_type_t type, const char *, ...) = _os_log_internal;\
if (ptr_os_log_internal != NULL) {\
_os_log_internal(&__dso_handle, OS_OBJECT_GLOBAL_OBJECT(os_log_t, _os_log_default), 0x00, [[NSString stringWithFormat:FORMAT, ##__VA_ARGS__] UTF8String]);\
}

UPD 2

After the community response, it has become clear that there are two issues here:

  1. There is a problem with the approach of passing format string into print() function.
  2. There is a problem in using private API (_os_log_internal(...)) instead of os_log in similar fashion (see the "UPD 1" section, where I provided the code being used).

Note: I also posted the same question to Apple Developer forum.

stdio char objective-c freebsd c
2 Answers

This is simply wrong, and the system is telling you so, and exactly why:

printf(foo);

This is a classic security problem. The correct code is:

printf("%s", foo);

foo is not a static string, and it is a security mistake to pass foo to a function that performs %-substitutions.

The reason a space doesn't matter is because the space is part of the format string. % n is a format specifier that includes space padding (even though that doesn't make sense for the n type, it's still legal). See the printf man page for a full description of the % specifiers. They're quite complicated and powerful, which is exactly why they're so dangerous, and you can't hand uncontrolled strings to them.

There's a quick description of the security problem at Wikipedia's Uncontrolled format string.

In your os_log wrapper, you seem to be trying to bypass the existing os_log define. First, you should use that rather than reaching into undocumented internals. The problem (which os_log likely would have made clearer at compile time) is that you cannot pass a non-static string to os_log in this field. os_log does very tricky and non-obvious interpolation on the string it's passed. It does this for both performance and security reasons. Rewrite this around os_log directly, and see the WWDC video explaining os_log.

user_973370

You need use puts instead of printf, because printf expects first argument to be string format. puts just puts plain string into output.

user_19347500
Related