vasnprintf: avoid using %n in the general case

vasnprintf: avoid using %n in the general case

[Jeremie Courreges-Anglas]

[-- Attachment #1.1: Type: text/plain, Size: 708 bytes --]


Hi,

The attached patch changes vasnprintf.c to avoid using %n in the general
case, ie when the return value of snprintf is usable.  This should help
if more systems decide to make tighten %n usage.  There are plans for
that in OpenBSD land.

The existing comments in vasnprintf.c mention systems where
gl_SNPRINTF_RETVAL_C99 and gl_SNPRINTF_TRUNCATION_C99 pass.  This patch
only considers the usability of the return value of snprintf, as lack
of truncation seems to be a different problem (apparently handled later
in the code).

The patch is kept short so that no copyright assignement is needed, but
further cleanup can be done.  For example the list of systems where %n
is avoided could be shortened.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-vasnprintf-avoid-using-n-in-the-general-case.patch --]
[-- Type: text/x-patch, Size: 1379 bytes --]

From e1e92bafa5ed7476c3facfa8b0bbc3e5cc7e6b52 Mon Sep 17 00:00:00 2001
From: Jeremie Courreges-Anglas <jca@wxcvbn.org>
Date: Sat, 3 Oct 2020 22:24:37 +0200
Subject: [PATCH] vasnprintf: avoid using %n in the general case

Several systems have started deprecating or tightening %n
usage, for security reasons.  Some ignore %n, some abort if the format
string is in writable memory, some just abort unconditionally.
Hardcoding a list of such systems doesn't scale.
* lib/vasnprintf.c: use the return value of snprintf if it is reliable.
---
 lib/vasnprintf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 7f7513956..b52629c0a 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -5117,7 +5117,9 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
 #endif
                   *fbp = dp->conversion;
 #if USE_SNPRINTF
-# if ! (((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3))        \
+# if HAVE_SNPRINTF_RETVAL_C99
+                fbp[1] = '\0';
+# elif ! (((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3))	\
          && !defined __UCLIBC__)                                            \
         || (defined __APPLE__ && defined __MACH__)                          \
         || defined __ANDROID__                                              \
-- 
2.28.0


[-- Attachment #1.3: Type: text/plain, Size: 82 bytes --]

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]

Re: vasnprintf: avoid using %n in the general case

[Bruno Haible]

Hi Jeremie,

> The attached patch changes vasnprintf.c to avoid using %n in the general
> case, ie when the return value of snprintf is usable.  This should help
> if more systems decide to make tighten %n usage.  There are plans for
> that in OpenBSD land.

Thanks for the suggestion. Yes, this is a good idea. (I didn't know about
the OpenBSD plans.)

> The existing comments in vasnprintf.c mention systems where
> gl_SNPRINTF_RETVAL_C99 and gl_SNPRINTF_TRUNCATION_C99 pass.  This patch
> only considers the usability of the return value of snprintf, as lack
> of truncation seems to be a different problem (apparently handled later
> in the code).

I prefer to limit the shortcut to those platforms where both
gl_SNPRINTF_RETVAL_C99 and gl_SNPRINTF_TRUNCATION_C99 pass. These are
the conditions I found when considering this code in detail a couple of
years ago. Maybe you are right that only one of the conditions is needed;
but I don't want to live on the risky edge here.


2020-10-04  Bruno Haible  <bruno@clisp.org>

	vasnprintf: Don't use %n on modern, ISO C 99 compliant platforms.
	Suggested by Jeremie Courreges-Anglas <jca@wxcvbn.org> in
	<https://lists.gnu.org/archive/html/bug-gnulib/2020-10/msg00010.html>.
	* m4/vasnprintf.m4 (gl_PREREQ_VASNPRINTF): Define
	HAVE_SNPRINTF_TRUNCATION_C99.
	* lib/vasnprintf.c (VASNPRINTF): Don't use %n if
	HAVE_SNPRINTF_RETVAL_C99 && HAVE_SNPRINTF_TRUNCATION_C99.

diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 7f75139..b232d14 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -5117,39 +5117,32 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
 #endif
                   *fbp = dp->conversion;
 #if USE_SNPRINTF
-# if ! (((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3))        \
-         && !defined __UCLIBC__)                                            \
-        || (defined __APPLE__ && defined __MACH__)                          \
-        || defined __ANDROID__                                              \
-        || (defined _WIN32 && ! defined __CYGWIN__))
-                fbp[1] = '%';
-                fbp[2] = 'n';
-                fbp[3] = '\0';
-# else
-                /* On glibc2 systems from glibc >= 2.3 - probably also older
-                   ones - we know that snprintf's return value conforms to
-                   ISO C 99: the tests gl_SNPRINTF_RETVAL_C99 and
-                   gl_SNPRINTF_TRUNCATION_C99 pass.
-                   Therefore we can avoid using %n in this situation.
-                   On glibc2 systems from 2004-10-18 or newer, the use of %n
-                   in format strings in writable memory may crash the program
-                   (if compiled with _FORTIFY_SOURCE=2), so we should avoid it
-                   in this situation.  */
-                /* On Mac OS X 10.3 or newer, we know that snprintf's return
-                   value conforms to ISO C 99: the tests gl_SNPRINTF_RETVAL_C99
-                   and gl_SNPRINTF_TRUNCATION_C99 pass.
-                   Therefore we can avoid using %n in this situation.
-                   On Mac OS X 10.13 or newer, the use of %n in format strings
-                   in writable memory by default crashes the program, so we
-                   should avoid it in this situation.  */
-                /* On Android, we know that snprintf's return value conforms to
-                   ISO C 99: the tests gl_SNPRINTF_RETVAL_C99 and
-                   gl_SNPRINTF_TRUNCATION_C99 pass.
-                   Therefore we can avoid using %n in this situation.
-                   Starting on 2018-03-07, the use of %n in format strings
-                   produces a fatal error (see
-                   <https://android.googlesource.com/platform/bionic/+/41398d03b7e8e0dfb951660ae713e682e9fc0336>),
-                   so we should avoid it.  */
+# 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__))
+                /* 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.  And it is desirable to do so, because more and
+                   more platforms no longer support %n, for "security reasons".
+                   In particular, the following platforms:
+                     - On glibc2 systems from 2004-10-18 or newer, the use of
+                       %n in format strings in writable memory may crash the
+                       program (if compiled with _FORTIFY_SOURCE=2).
+                     - On Mac OS X 10.13 or newer, the use of %n in format
+                       strings in writable memory by default crashes the
+                       program.
+                     - On Android, starting on 2018-03-07, the use of %n in
+                       format strings produces a fatal error (see
+                       <https://android.googlesource.com/platform/bionic/+/41398d03b7e8e0dfb951660ae713e682e9fc0336>).
+                   On these platforms, HAVE_SNPRINTF_RETVAL_C99 and
+                   HAVE_SNPRINTF_TRUNCATION_C99 are 1. We have listed them
+                   explicitly in the condition above, in case of cross-
+                   compilation (just to be sure).  */
                 /* On native Windows systems (such as mingw), we can avoid using
                    %n because:
                      - Although the gl_SNPRINTF_TRUNCATION_C99 test fails,
@@ -5166,6 +5159,10 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
                      <https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/set-printf-count-output>
                    So we should avoid %n in this situation.  */
                 fbp[1] = '\0';
+# else           /* AIX <= 5.1, HP-UX, IRIX, OSF/1, Solaris <= 9, BeOS */
+                fbp[1] = '%';
+                fbp[2] = 'n';
+                fbp[3] = '\0';
 # endif
 #else
                 fbp[1] = '\0';
diff --git a/m4/vasnprintf.m4 b/m4/vasnprintf.m4
index 4567061..43f070e 100644
--- a/m4/vasnprintf.m4
+++ b/m4/vasnprintf.m4
@@ -1,4 +1,4 @@
-# vasnprintf.m4 serial 37
+# vasnprintf.m4 serial 38
 dnl Copyright (C) 2002-2004, 2006-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -74,6 +74,16 @@ AC_DEFUN_ONCE([gl_PREREQ_VASNPRINTF],
          if the buffer had been large enough.])
       ;;
   esac
+  dnl Additionally, the use of %n can be eliminated by assuming that snprintf
+  dnl always produces NUL-terminated strings (no truncation).
+  AC_REQUIRE([gl_SNPRINTF_TRUNCATION_C99])
+  case "$gl_cv_func_snprintf_truncation_c99" in
+    *yes)
+      AC_DEFINE([HAVE_SNPRINTF_TRUNCATION_C99], [1],
+        [Define if the string produced by the snprintf function is always NUL
+         terminated.])
+      ;;
+  esac
 ])
 
 # Extra prerequisites of lib/vasnprintf.c for supporting 'long double'

Re: vasnprintf: avoid using %n in the general case

[Jeremie Courreges-Anglas]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

On Sun, Oct 04 2020, Bruno Haible <bruno@clisp.org> wrote:
> Hi Jeremie,
>
>> The attached patch changes vasnprintf.c to avoid using %n in the general
>> case, ie when the return value of snprintf is usable.  This should help
>> if more systems decide to make tighten %n usage.  There are plans for
>> that in OpenBSD land.
>
> Thanks for the suggestion. Yes, this is a good idea. (I didn't know about
> the OpenBSD plans.)
>
>> The existing comments in vasnprintf.c mention systems where
>> gl_SNPRINTF_RETVAL_C99 and gl_SNPRINTF_TRUNCATION_C99 pass.  This patch
>> only considers the usability of the return value of snprintf, as lack
>> of truncation seems to be a different problem (apparently handled later
>> in the code).
>
> I prefer to limit the shortcut to those platforms where both
> gl_SNPRINTF_RETVAL_C99 and gl_SNPRINTF_TRUNCATION_C99 pass. These are
> the conditions I found when considering this code in detail a couple of
> years ago. Maybe you are right that only one of the conditions is needed;
> but I don't want to live on the risky edge here.

Your approach certainly makes sense.  I considered something similar
earlier.  Your changes look good to me, thanks!

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]