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

[Ryan S. Arnold]

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