_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)

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

#include <unistd.h>

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

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

Compile like this:

gcc -fsanitize=address -o main main.c printf.c

The output:

./main
Reads past this string
Should not see this: it's beyond the NUL terminator!=================================================================
==71716==ERROR: AddressSanitizer: global-buffer-overflow on address 0x561c9bbae091 at pc 0x561c9bbac9e1 bp 0x7ffe9610e970 sp 0x7ffe9610e960
READ of size 1 at 0x561c9bbae091 thread T0
    #0 0x561c9bbac9e0 in _vsnprintf (/home/francis/dev/git/printf/tst+0x59e0)
    #1 0x561c9bbacbda in printf_ (/home/francis/dev/git/printf/tst+0x5bda)
    #2 0x561c9bba83f8 in main (/home/francis/dev/git/printf/tst+0x13f8)
    #3 0x7f4594eadd8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #4 0x7f4594eade3f in __libc_start_main_impl ../csu/libc-start.c:392
    #5 0x561c9bba8204 in _start (/home/francis/dev/git/printf/tst+0x1204)

0x561c9bbae091 is located 0 bytes to the right of global variable '*.LC1' defined in 'main.c' (0x561c9bbae040) of size 81
SUMMARY: AddressSanitizer: global-buffer-overflow (/home/francis/dev/git/printf/tst+0x59e0) in _vsnprintf
Shadow bytes around the buggy address:
  0x0ac41376dbc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac41376dbd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac41376dbe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac41376dbf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac41376dc00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ac41376dc10: 00 00[01]f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0ac41376dc20: 00 00 00 00 04 f9 f9 f9 f9 f9 f9 f9 05 f9 f9 f9
  0x0ac41376dc30: f9 f9 f9 f9 05 f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9
  0x0ac41376dc40: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac41376dc50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ac41376dc60: 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
==71716==ABORTING

[eyalroz]

@aptly-io : This library is unmaintained and has a large number of unresolved bugs. The maintained for is mine. Could you please test with my fork to see whether you see the bug there as well?

[ledvinap]

Fixing this needs sanitizing width/precision parsing, adding code that is used only in malformed program.
Passing invalid format string is probably in 'undefined behavior' category, so it may be better to ignore it.
There are tons of other things you can do with malformed format string that can't be sanitized (%n etc.)

[aptly-io]

@ledvinap I agree with you all input needs to be sanitized by the caller of printf().
I dont agree with leaving security holes. If %. cannot be supported correctly, it is better to remove it from this code base.
@eyalroz I see it also in that fork. I will make an issue there as well.
Thanks all for looking into it.

[ledvinap]

It's not '%.'. '%\0' will behave the same, putting '\0' into output stream and continuing format expansion.

Special-casing '\0' in format character switch is possible, but see above.

[aptly-io]

@ledvinap I will try that case as well. Thanks! It also fails in the fork.

[eyalroz]

So, this is not a bug, as incomplete format specifier at string end cause undefined behavior. I am considering an opt-in "sanitization" via a CMake option - please have a look at eyalroz#140. But - this issue needs to be closed. @aptly-io : Please close this...