[PATCH] resolv: Add test for gethostbyname_r unaligned buffer [BZ #18287]

[Florian Weimer]

* Sergey Kolosov:

>  resolv/tst-resolv-gethostbyname_r-unaligned.c | 158 ++++++++++++++++++

The test file contains a couple of lines with trailing whitespace.
Please fix.

>  2 files changed, 161 insertions(+)
> diff --git a/resolv/tst-resolv-gethostbyname_r-unaligned.c b/resolv/tst-resolv-gethostbyname_r-unaligned.c
> new file mode 100644
> index 0000000000..7298c6f8e6
> --- /dev/null
> +++ b/resolv/tst-resolv-gethostbyname_r-unaligned.c
> @@ -0,0 +1,158 @@

> +static int
> +query_host (const char *host_name, size_t size, int align)

size_t align?

> +{
> +  struct hostent ret;
> +  struct hostent *result;
> +  int err;
> +
> +  /* Allocate memory for buffer, align and 64 bytes.  */

“alignment and a 64-byte checking area”?

> +  size_t total_alloc = size + align + 64;
> +  unsigned char *raw_buf = malloc (total_alloc);
> +  TEST_VERIFY_EXIT (raw_buf != NULL);
> +
> +  /* Fill in buffer with 0xAA to detect buffer overflow.  */
> +  memset (raw_buf, 0, size + align);
> +  memset (raw_buf + size + align, 0xAA, 64);
> +  
> +  char *ptr = (char *) raw_buf + align;
> +
> +  int res = gethostbyname_r (host_name, &ret, ptr, size, &result, &err);
> +
> +  /* Verify that the overflow guard region remains unchanged.  */
> +  int overflow_detected = 0;
> +  for (int i = 0; i < 64; i++)
> +    {
> +      if (raw_buf[size + align + i] != 0xAA)
> +        {
> +          overflow_detected = 1;
> +          break;
> +        }
> +    }
> +
> +  free (raw_buf);
> +
> +  if (overflow_detected)
> +    {
> +      printf ("Buffer overflow was detected! (align=%d, size=%zu)\n",
> +              align, size);
> +      /* Return 99 to indicate that a buffer overflow was detected.  */
> +      return 99;
> +    }

I think it's okay to use FAIL_EXIT1 and avoid the 99 special return
code.

Then you can switch the return to bool.  Please also add a function
comment for query_host that explains the parameters and the return
value.

> +  if (res == 0 && result != NULL)
> +    return 0; /* Buffer is enough.  */

Maybe also use check_hostent to check that the response is as expected?
(This may require changeing response so that it always provides the
same response.)

> +  else if (res == ERANGE || err == NETDB_INTERNAL)
> +    return 1; /* Buffer is too small.  */

I think the || should be &&.


> +  /* Trigger the vulnerability.
> +     Test all misalignments (1-7 bytes). A patched glibc safely
> +     returns 0 (success) or 1 (ERANGE) depending on padding.
> +     A vulnerable glibc will fail to account for alignment padding,
> +     overflow the buffer, corrupt the 0xAA canary, and return 99.  */

This comment needs updating once you eliminate the special 99 return
value.

> +  for (int align = 1; align < 8; align++)
> +    {
> +      int qres = query_host (host_name, upper_bound, align);
> +      
> +      if (qres == 99)
> +        FAIL_EXIT1 ("Buffer overflow detected at align %d\n", align);
> +      
> +      /* Return value other than 99 means no overflow occurred.  */
> +      TEST_VERIFY (qres == 0 || qres == 1);

Maybe add a comment that it is expected that for certain alignments, the
required buffer size increases.

Thanks,
Florian