Hi Bertrand,
Bertrand Garrigues via wrote on Thu, Oct 15, 2020 at 12:03:44AM +0200:
> On Mon, Oct 12 2020 at 06:08:27 PM, Ingo Schwarze <schwa...@usta.de> wrote:
>> A major problem with gnulib emerged just now that we should perhaps
>> treat as a blocker.
> [...]
>> The code in
>>
>> gnulib/lib/vasnprintf.c
>>
>> line 4879 puts a format string containing a %n directive into
>> writeable memory and subsequently passes that memory as a first
>> argument to printf(3).
>>
>> Using %n at all is insecure programming practice.
> [...]
> That seems to be only for old versions of glibc or a few operating
> systems:
>
> # if ! (((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3)) \
> && !defined __UCLIBC__) \
> || (defined __APPLE__ && defined __MACH__) \
> || (defined _WIN32 && ! defined __CYGWIN__))
> fbp[1] = '%';
> fbp[2] = 'n';
> fbp[3] = '\0';
> # else
Well, what that code means is "*everywhere* except on newer glibc
and some cases of Apple and Windows" - so it did trigger on OpenBSD.
> From what I understand in their comment after the #else they try do to
> their best to avoid using %n. You are referring to line 4879, which
> means you were looking on the gnulib integrated on the 1.22.4. We also
> have the same logic on the up-to-date gnulib (line 5126):
>
> /* On systems where we know that snprintf's return value
> conforms to ISO C 99 (HAVE_SNPRINTF_RETVAL_C99) and that
> snprintf always produces NUL-terminated strings
> (HAVE_SNPRINTF_TRUNCATION_C99), it is possible to avoid
> using %n
> [...]
Oh, you have a point here. After your recent gnulib update - thanks
for doing that, even though it caused a minor glitch, see my Savannah
ticket 59276, keeping libraries up to date does make sense - the
condition is now inverted and reads as follows:
# if ((HAVE_SNPRINTF_RETVAL_C99 && HAVE_SNPRINTF_TRUNCATION_C99) \
|| ((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3)) \
&& !defined __UCLIBC__) \
|| (defined __APPLE__ && defined __MACH__) \
|| defined __ANDROID__ \
|| (defined _WIN32 && ! defined __CYGWIN__))
Even though the nasty OS-tests are still there, they are now obsolete
and instead, %n is only used if HAVE_SNPRINTF_RETVAL_C99
or HAVE_SNPRINTF_TRUNCATION_C99 (or both) fail.
Both succeed on OpenBSD, so i confirmed with the follwoing command
that the insecure code with %n is no longer compiled, for now:
gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I.. -I./src/include
-I../src/include -I../lib -I./src/include -I./lib -I/usr/local/include
-g -O2 -c -save-temps -o lib/vasnprintf.o ../lib/vasnprintf.c
(Notice the -save-temps in there.)
Meanwhile, i heard rumours that the upcoming change in libc that will
cause %n in writeable memory to abort the program will cause some
gnulib ./configure tests to (wrongly) fail, again resulting in
compilation of vasnprintf.c with %n and ultimately killing groff
at run-time. I'll have to check whether such a problem really
exists for groff. If yes, i'll have to investigate further and
find a fix. If not, all this can be postponed until after release.
> #else /* AIX <= 5.1, HP-UX, IRIX, OSF/1, Solaris <= 9, BeOS */
Uh oh. Are we really trying to support *those*?
Well, maybe that's a different discussion...
> fbp[1] = '%';
> fbp[2] = 'n';
> fbp[3] = '\0';
> #endif
>
> Does your build bump into the code after the #else?
It did until last week, it does not right now, but i have to check
whether it will start doing that again in the near future.
I'll come back to you after checking that.
>> The way gnulib detects whether to compile that code seems to be
>> very broken. On the one hand, gnulib performs extremely complex
>> tests that resemble an (overly aggressive) regression suite rather
>> than mere feature tests. If i understand correctly, if *anything*
>> in that suite fails, no matter whether it's important or just a
>> very weird corner case, gnulib tends to compile the file
>> gnulib/lib/vasnprintf.c.
> I think we have vasnprintf.c only because it is a dependency of the
> vsnprintf module (listed in 'gnulib_modules' in bootstrap.conf).
Are you really sure?
Merely for testing (not intended for commit at this point),
i just deleted "vsnprintf" from bootstrap.conf. While that does
remove "vsnprintf" , "lib/vsnprintf.c", and "m4/vsnprintf.m4"
from bootstrap output, "vasnprintf" is still there as a dependency
of "snprintf", and "lib/vasnprintf.c" and "m4/vasnprintf.m4" are
still in the file list. Also, vasnprintf.c still gets compiled
for me, presumably because (hard to be sure due to the complexity,
but that's my guess at this point):
checking whether printf supports the 'a' and 'A' directives... no
checking whether printf survives out-of-memory conditions... no
Of course, these two results are bogus. We do support %a/%A, and
i didn't investigate yet what the second one means, but whatever
it may mean, that's clearly not a good reason to replace native
printf(3).
>> On the other hand, that file does lots
>> of very complicated version number and operating system name tests
>> on its own at compile time rather than at ./configure time, so it
>> is very difficult to figure out what is actually happening, and
>> even more so to check whether what is happening in a specific
>> compilation run is correct.
>>
>> For example, it appears that one among large numbers of effects
>> that can trigger compilation of gnulib/lib/vasnprintf.c is that the
>> operating system denies %n in writeable memory, and as a consequence
>> gnulib/lib/vasnprintf.c is compiled and then proceeds to use %n in
>> writeable memory.
> [...]
> From what I understand we build this code only on some old platforms.
> The code that uses %n is not built on my system.
You probably have glibc, right?
> [...]
>> While i did rarely run into operating systems that failed to provide
>> the non-standard function vasprintf(3) - a GNU extension also
>> available in all BSDs - groff does not use that function. I do not
>> remember ever running into a system lacking any of the standard
>> functions * fprintf(3) - first appeared in Version 7 AT&T UNIX *
>> snprintf(3) and vsnprintf(3) - first in 4.4BSD
>>
>> So my favourite solution would be to just stop using all the gnulib
>> *printf* modules. I'm not aware of any portability problems they
>> might help to solve, not even on historic systems like Solaris 9,
> the *printf* modules from gnulib were used in replacement of some
> printf functions implemented directly in groff's source code.
That's probably not a good idea either.
> I believe that
> the gnulib's versions are much better maintained. Perhaps that now
> groff builds fine without any replacement function at all, but that
> would require to spend extra time to check that, so unless it is clearly
> proven that there is a problem in gnulib's replacement functions I don't
> see any reason to remove them.
If i manage to confirm that the tests for HAVE_SNPRINTF_RETVAL_C99
and HAVE_SNPRINTF_TRUNCATION_C99 are reasoably robust, we can
postpone further deliberation until after release, i think.
>> but they most definitly cause severe portability and security issues
>> on several operating systems, in particular on modern ones.
>>
>> What do you think?
>>
>> Should i do testing without these three modules on OpenBSD, Linux,
>> and various versions of Solaris to see if that improves matters?
> I think that unless you find out that the code using %n is really
> compiled and used on one of your build, it's not worth spending time on
> that.
I will re-test ASAP with the upcoming libc change; to save time,
i will do so without waiting for that change to be committed.
Yours,
Ingo
