Re: [PATCH] vfprint: validate nargs and argument-based offsets

["Ryan S. Arnold" <ryan dot arnold at gmail dot com>]

On Thu, Feb 2, 2012 at 10:04 AM, Kees Cook <kees@outflux.net> wrote:
> 2012-02-02 ÂKees Cook Â<keescook@chromium.org>
>
> Â Â Â Â* stdio-common/vfprintf.c (vfprintf): Checks for nargs overflow and
> Â Â Â Âvalidates argument-based array offsets.
> Â Â Â Â* stdio-common/tst-vfprintf-nargs.c: New file.
> Â Â Â Â* stdio-common/Makefile (tests): Add nargs overflow test.

Hi Kees, Thanks for the contribution.

The addition of the tst-vfprintf-nargs file makes this contribution
legally significant.  Please verify your FSF copyright-assignment
status with your employer.

> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 006f546..7d34b7a 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -60,7 +60,8 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \
> Â Â Â Â tst-popen tst-unlockedio tst-fmemopen2 tst-put-error tst-fgets \
> Â Â Â Â tst-fwrite bug16 bug17 tst-swscanf tst-sprintf2 bug18 bug18a \
> Â Â Â Â bug19 bug19a tst-popen2 scanf13 scanf14 scanf15 bug20 bug21 bug22 \
> - Â Â Â Âscanf16 scanf17 tst-setvbuf1 tst-grouping bug23 bug24
> + Â Â Â Âscanf16 scanf17 tst-setvbuf1 tst-grouping bug23 bug24 \
> + Â Â Â Âtst-vfprintf-nargs
>
> Âtest-srcs = tst-unbputc tst-printf
>
> diff --git a/stdio-common/tst-vfprintf-nargs.c b/stdio-common/tst-vfprintf-nargs.c
> new file mode 100644
> index 0000000..7c48e20
> --- /dev/null
> +++ b/stdio-common/tst-vfprintf-nargs.c

I believe that you should add a GLIBC copyright header, including
copyright year and a "Contributed by " statement but perhaps someone
else with an opinion can comment.

Does the 64-bit case blowing the stack may preclude the use of
test-skeleton.c for the testcase?

> @@ -0,0 +1,91 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +#include <inttypes.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +
> +int
> +format_failed(const char *fmt, const char *expected, int wanted)
> +{
> + Âchar output[80];
> + Âpid_t pid;
> + Âint status;
> +
> + Âprintf("%s : ", fmt);
> + Âfflush(NULL);
> +
> + Â/* Since the stack could be extremely wrecked by this test, use
> + Â Â an external supervisor process to catch the signals, since a
> + Â Â signal handler may not be able to recover.
> + Â */
> + Âpid = fork();
> + Âif (pid < 0)
> + Â Â{
> + Â Â Âperror("fork");
> + Â Â Âreturn 1;
> + Â Â}
> + Âif (pid)
> + Â Â{
> + Â Â Âwaitpid(pid, &status, 0);
> + Â Â Âif (WIFEXITED(status))
> + Â Â Â Â{
> + Â Â Â Â Âputs(WEXITSTATUS(status) == wanted ? "ok" : "FAILED");
> + Â Â Â Â Âreturn WEXITSTATUS(status);
> + Â Â Â Â}
> + Â Â Âelse if (WIFSIGNALED(status))
> + Â Â Â Â{
> + Â Â Â Â Âif (WTERMSIG(status) == -wanted)
> + Â Â Â Â Â Â{
> + Â Â Â Â Â Â Âprintf("ok (ignored expected signal %d)\n", -wanted);
> + Â Â Â Â Â Â Âreturn 0;
> + Â Â Â Â Â Â}
> + Â Â Â Â Âfprintf(stderr, "signal %d\n", WTERMSIG(status));
> + Â Â Â Â Âreturn 1;
> + Â Â Â Â}
> + Â Â Âelse
> + Â Â Â Â{
> + Â Â Â Â Âfprintf(stderr, "Unexpected failure\n");
> + Â Â Â Â Âreturn 2;
> + Â Â Â Â}
> + Â Â}
> +
> + Âmemset(output, 0, sizeof(output));
> + Â/* Having sprintf itself detect a failure is good. Â*/
> + Âif (sprintf(output, fmt, 1, 2, 3, "test") < 0)
> + Â Â Âexit(0);
> + Âif (strcmp(output, expected))
> + Â Â{
> + Â Â Âfprintf(stderr, "(output '%s' != expected '%s') : ", output, expected);
> + Â Â Âexit(1);
> + Â Â}
> + Âexit(0);
> +}
> +
> +int
> +main(int argc, char *argv[])
> +{
> + Âint rc = 0, wanted;
> + Âchar buf[64];
> +
> + Â/* Positional arguments are constructed via read_int(), so nargs
> + Â Â can only overflow on 32bit systems. On 64bit systems, it will
> + Â Â attempt to allocate a giant amount of stack memory and crash,
> + Â Â which is the expected situation. Â*/
> + Âif (sizeof(long) == 4)
> + Â Âwanted = 0;
> + Âelse
> + Â Âwanted = -11;

I believe using a compile time macro is more common:

#if __WORDSIZE == 32
  wanted = 0;
#else
  wanted = -11;
#endif

> + Âsprintf(buf, "%%1$d %%%" PRIdPTR "$d", UINT32_MAX / sizeof(int));
> + Âif (format_failed(buf, "1 %$d", wanted)) rc = 1;

I believe the precedent is to newline and indent the "rc = 1;".

> +
> + Â/* Regular positionals work. Â*/
> + Âif (format_failed("%1$d", "1", 0)) rc = 1;
> +
> + Â/* Regular width positionals work. Â*/
> + Âif (format_failed("%1$*2$d", " 1", 0)) rc = 1;

In general you needs a space between the function name and the
parenthesis throughout the patch, e.g., printf() should be printf ().

Regards,
Ryan S. Arnold