snprintf (was: [PINENTRY PATCH] gnome3: Avoid risk of uinitialized memory access)

[Werner Koch]

On Sat,  5 Nov 2016 22:53, dkg at fifthhorseman.net said:

> snprintf doesn't write the final NUL byte if the string exceeds the
> buffer, so anyone reading specific_err_info as a NUL-terminated string

I have some comments on this problem, given that I changed your original
commit and removed the explicit string termination before Neal changed
this again to use g_strdup_printf.  This is just a FWIW and there is no
need for any action.

The specs are not fully clear on what shall happen when the result needs
to be truncated but there would be space for a terminating Nul.  It is
probably not only my interpretation that surprises should be avoid and
snprintf(3) should behave similar to fgets(3); the Linux man page,
which should be aligned with C89 and POSIX-2001 says:

  fgets() reads in at most one less than size characters from stream and
  stores them into the buffer pointed to by s.  [...] A terminating null
  byte ('\0') is stored after the last character in the buffer.

This behaviour makes a lot of sense because, like snprintf, fgets works
on strings (i.e. Nul terminated buffers).  GnuPG's own snprintf
implementations (meanwhile taken from libgpg-error) makes sure that the
buffer is always Nul terminated (unless SIZE is 0),  This is the same
behaviour as glibc and other other snprintf implementations.  A test
program (see below) shows this:

  $ ./a.out
  ->xxxxx<-
  ->123<-
  ->123<-

Now, the Windows snprintf does not behave in this sane way:

  $ wine ./a.exe
  ->xxxxx<-
  ->123<-
  ->1234x<-

I knew that this was the case for Windows XP but I assumed that this was
fixed in later Windows versions.  Only now I looked at more recent
versions and unfortunately I see the same behaviour from XP and Wine
also on Vista.

Old code in GnuPG used to use this pattern:

  snprintf (buf, sizeof buf, format);
  buf[sizeof buf -1] = 0;

which was only for that Windows problem.  After we switched to our own
snprintf (which also allows for arg reordering and all kind of modern
format specifiers) that failsafe assignment was removed.

I removed that code also from Pinentry without realizing that we do not
use our wrapped function.

> written buffer.  It would be simpler to use asprintf, but i suspect
> that's not portable enough for use in pinentry.

Well, we could make use of the asprintf from libgpg-error.  But similar
to g_strdup_printf (which is now used), there is the problem of
different malloc systems in Windows - you need to use the free from the
same libc DLL which also implemnts the used malloc.  For larger
applications it may happen that several libc versions are in use!  Thus
to use the asprintf from libgpg-error the free would also needed to be
remapped to es_free.  Thus using the old explicit termination is often
a better way.  


Shalom-Salam,

   Werner


p.s.
--8<---------------cut here---------------start------------->8---
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

int
main (void)
{
  char buffer[6];

  memset (buffer, 'x', 5);
  buffer[5] = 0;
  printf ("->%s<-\n", buffer);

  snprintf (buffer, 4, "123");
  printf ("->%s<-\n", buffer);

  memset (buffer, 'x', 5);
  buffer[5] = 0;

  snprintf (buffer, 4, "1234");
  printf ("->%s<-\n", buffer);

  return 0;
}
--8<---------------cut here---------------end--------------->8---


-- 
Die Gedanken sind frei.  Ausnahmen regelt ein Bundesgesetz.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: not available
URL: </pipermail/attachments/20161107/3e40beff/attachment.sig>