_vsnprintf reads past the format parameter value

[aptly-io]

This code snippet illustrates the issue (the issue does not manifest when using libc's printf):
(it can likely be abused by a hacker for malicious purposes)
It originated here mpaland#139

/* enable one of these 2 lines to use printf()*/
#include <printf/printf.h>  // enable this and see wrong print
#include <unistd.h>


void putchar_(char character) {
    int dummy = write(1, &character, 1);
}


int main(int argc, char *argv[]) {
    // case 1
    printf_("Reads past this string%.\x00\nShould not see this: it's beyond the NUL terminator!%.");

    // case 2 (mentioned by https://github.com/ledvinap)
    printf_("Reads past this string%");
    
    return 0;
}

Compile like this:

gcc -fsanitize=address -o tst -I src main.c src/printf/printf.c

The output:

./tst
Reads past this string
Should not see this: it's beyond the NUL terminator!=================================================================
==73260==ERROR: AddressSanitizer: global-buffer-overflow on address 0x563bc4f2a091 at pc 0x563bc4f28e70 bp 0x7fff7ffeabb0 sp 0x7fff7ffeaba0
READ of size 1 at 0x563bc4f2a091 thread T0
    #0 0x563bc4f28e6f in _vsnprintf (/home/francis/dev/git/printf.maintained/tst+0x7e6f)
    #1 0x563bc4f29051 in vprintf_ (/home/francis/dev/git/printf.maintained/tst+0x8051)
    #2 0x563bc4f29596 in printf_ (/home/francis/dev/git/printf.maintained/tst+0x8596)
    #3 0x563bc4f22478 in main (/home/francis/dev/git/printf.maintained/tst+0x1478)
    #4 0x7f13fcf08d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #5 0x7f13fcf08e3f in __libc_start_main_impl ../csu/libc-start.c:392
    #6 0x563bc4f22284 in _start (/home/francis/dev/git/printf.maintained/tst+0x1284)

0x563bc4f2a091 is located 47 bytes to the left of global variable '*.LC2' defined in 'main.c' (0x563bc4f2a0c0) of size 24
  '*.LC2' is ascii string 'Reads past this string%'
0x563bc4f2a091 is located 0 bytes to the right of global variable '*.LC1' defined in 'main.c' (0x563bc4f2a040) of size 81
SUMMARY: AddressSanitizer: global-buffer-overflow (/home/francis/dev/git/printf.maintained/tst+0x7e6f) in _vsnprintf
Shadow bytes around the buggy address:
  0x0ac7f89dd3c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac7f89dd3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac7f89dd3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac7f89dd3f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac7f89dd400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ac7f89dd410: 00 00[01]f9 f9 f9 f9 f9 00 00 00 f9 f9 f9 f9 f9
  0x0ac7f89dd420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac7f89dd430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac7f89dd440: 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 00 00 00 00
  0x0ac7f89dd450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac7f89dd460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==73260==ABORTING

[eyalroz]

Thanks for your effort filing this. I will try to look into it within the next few days. If I can perhaps even by tomorrow, but no guarantees.

[eyalroz]

First - I can verify that this is not a bug. The standard states that only with a valid conversion specifier is the behavior defined. With UB, it is legitimate for _vsnprintf to read past the format parameter value. So, it is valid behavior. You're asking for a more graceful behavior in this case than the standard requires. However, I'm not sure I buy the argument: A "hacker" does not provide you with a format string. The format string is almost always baked in to the program. And if you wrote a program which accepts a format string from the user, then you have much greater problems than this issue.

[eyalroz]

Please try out the tip of branch fix-140, and let me know what you think.

[aptly-io]

Compiling like this:

gcc -DPRINTF_CHECK_FOR_NUL_IN_FORMAT_SPECIFIER=1 -fsanitize=address -o tst -I src main.c src/printf/printf.c

Works safely:

$ ./tst
Reads past this stringReads past this string

Let's hope users define -DPRINTF_CHECK_FOR_NUL_IN_FORMAT_SPECIFIER prior to compiling.
And ... apply your advice above on the sound usage of the format string!

PS a diff with the master seems to contain more changes than necessary, is that your intention?

[eyalroz]

Let's hope users define -DPRINTF_CHECK_FOR_NUL_IN_FORMAT_SPECIFIER prior to compiling.

If you use CMake to build the program, it'll default to ON. In fact, I can make it default to on even without CMake...

[eyalroz]

So, my own tests weren't passing; I've amended the commit.

[eyalroz]

@aptly-io : Are you ok with the changes I've made on this branch? If so, I'll merge them into develop

[bemeyvif]

@eyalroz Thank you. I will try it out during the weekend.

I had this initial thought. I can find myself in @ledvinap argument that it's cleaner to validate format while it's de-referenced. Rather than using ADVANCE_IN_FORMAT_STRING. Current code sometimes does a simple format++ and at times uses the ADVANCE_IN_FORMAT_STRING. This makes it not trivial for someone unfamiliar with the code. (I understand your choice since other approaches require more refactoring).

So for instance I'm pondering when you check for 0 after returning from parse_flags, why also not after returning from atou_?

[eyalroz]

So for instance I'm pondering when you check for 0 after returning from parse_flags, why also not after returning from atou_?

That's because after the atou_ calls, if the current character is '\0', we will not find a precision field, length field, or a specifier, and will just cycle back.

... and actually, checking after parse_flags() is redundant! I'll remove it. And actually, there are even more redundant checks, like the one before parse_flags() - but I'll keep that one, so as not to have implicit requirements of safety from parse_flags().

[eyalroz]

Ok, merged this change into the develop branch.

[aptly-io]

@eyalroz Thank you for the fix:
In branch develop it correctly executes the test program from above.

Note I noticed that static int vsnprintf_impl(output_gadget_t* output, const char* format, va_list args)is defined twice in printf.c (line 1356 and 1370). clang complained:

src/printf/printf.c:1370:12: error: redefinition of 'vsnprintf_impl'
static int vsnprintf_impl(output_gadget_t* output, const char* format, va_list args)
           ^
src/printf/printf.c:1356:12: note: previous definition is here
static int vsnprintf_impl(output_gadget_t* output, const char* format, va_list args)

On your earlier reply:

... and actually, checking after parse_flags() is redundant! I'll remove it. And actually, there are even more redundant

It proves my point made earlier that format++ or the ADVANCE_IN_FORMAT_STRING is not trivial :-)

[eyalroz]

I noticed that static int vsnprintf_impl(output_gadget_t* output, const char* format, va_list args)is defined twice in

Fixed.

It proves my point made earlier that format++ or the ADVANCE_IN_FORMAT_STRING is not trivial :-)

Fair enough.