Re: cc claims ISO C99 support, but %n printf format specifier calls abort()

[Jeremie Courreges-Anglas]

On Fri, Dec 16 2022, Vincent Lefevre  wrote:
> On 2022-12-15 18:56:15 -0700, Theo de Raadt wrote:
>> There are almost no %n left in the software ecosystem.  If we are able
>> to make this crossing, everyone else is also capable, and eventually
>> will.  Just like with gets().
>
> FYI, this breaks GMP, whose configure script insists on %n being
> available, otherwise GMP uses its own, buggy implementation of
> vsnprintf, which triggers an assertion failure when %a/%A is used
> (and this bug affects MPFR). AFAIK, the GMP developers haven't
> reacted to the bug report sent in October.

https://gmplib.org/list-archives/gmp-bugs/2022-October/005200.html
Between the dubious antique check in acinclude.m4 and the bug in the
*printf reimplementation, this really sound like self-inflicted pain.
The diff below (against gmp-6.2.1) skips the %n test and is enough to
fix the misdetection.  Some tests need to stop using %n too.

Not an OpenBSD bug IMO, better discuss this with the GMP developers.


--- acinclude.m4.orig   Fri Dec 16 20:45:25 2022
+++ acinclude.m4Fri Dec 16 20:51:48 2022
@@ -3664,9 +3664,9 @@ dnl  literal string is enough to provoke the problem, 
 dnl  needed.  There seems to be something weird going on with the optimizer
 dnl  or something, since on the first system adding a second check with
 dnl  "%n", or even just an initialized local variable, makes it work.  In
-dnl  any case, without bothering to get to the bottom of this, the two
-dnl  program runs in the code below end up successfully detecting the
-dnl  problem.
+dnl  any case, without bothering to get to the bottom of this, using %n is
+dnl  frowned upon on several modern systems.  So just assume that vsnprintf
+dnl  works if a simple test works...
 dnl
 dnl  glibc 2.0.x returns either -1 or bufsize-1 for an overflow (both seen,
 dnl  not sure which 2.0.x does which), but still puts the correct null
@@ -3682,7 +3682,6 @@ else
   AC_CACHE_CHECK([whether vsnprintf works],
  gmp_cv_func_vsnprintf,
   [gmp_cv_func_vsnprintf=yes
-   for i in 'return check ("hello world");' 'int n; return check ("%nhello 
world", );'; do
  AC_TRY_RUN([
 #include   /* for strcmp */
 #include/* for vsnprintf */
@@ -3713,13 +3712,12 @@ check (const char *fmt, ...)
 int
 main ()
 {
-$i
+   return check ("hello world");
 }
 ],
-  [:],
-  [gmp_cv_func_vsnprintf=no; break],
-  [gmp_cv_func_vsnprintf=probably; break])
-  done
+  [gmp_cv_func_vsnprintf=yes],
+  [gmp_cv_func_vsnprintf=no],
+  [gmp_cv_func_vsnprintf=probably])
   ])
   if test "$gmp_cv_func_vsnprintf" = probably; then
 AC_MSG_WARN([cannot check for properly working vsnprintf when cross 
compiling, will assume it's ok])

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

Re: cc claims ISO C99 support, but %n printf format specifier calls abort()

[Theo de Raadt]

That could almost be an entry for calendars.openbsd

Dec 16 Vincent Lefevre arrives and tries to educate the OpenBSD developers
   about format string vulnerabilities, which they have been fixing
   since 1996

Vincent Lefevre  wrote:

> On 2022-12-16 09:03:39 -0700, Theo de Raadt wrote:
> > Vincent Lefevre  wrote:
> > 
> > > BTW, if developers use an untrusted format string, then sprintf()
> > > is unsafe too (possible buffer overflow), and at some point,
> > > printf() too.
> > 
> > what are you trying to say?
> 
> According to
> 
>   https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2834.htm
> 
> it has been proposed to deprecate %n mainly because:
> 
>   If the format argument of a printf-style-function originates from an
>   unverified source an attacker may be able to write arbitrary values
>   to the stack.
> 
> But I'm saying that from this point of view, sprintf() is unsafe too,
> actually much more unsafe. Something like %n injection in the format
> argument can easily be detected (actually any injection of a format
> specifier). For instance, if someone writes
> 
>   printf (buf);
> 
> assuming that the string buf will be output, where buf has untrusted
> contents, then if buf contains %n, the number of arguments will be
> incorrect, so that this can be detected at run time (ditto if printf
> has arguments that correspond to a trusted part of buf). However,
> with
> 
>   sprintf (s, buf);
> 
> (rather than using snprintf), even if the number of arguments is
> checked to be correct, there could still be a buffer overflow with
> an untrusted buf.
> 
> It's a pity that N2834 doesn't give real-world examples of insecure
> use of %n, in particular ones that could not be possible to detect
> at run time.
> 
> -- 
> Vincent Lefèvre  - Web: 
> 100% accessible validated (X)HTML - Blog: 
> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: cc claims ISO C99 support, but %n printf format specifier calls abort()

[Vincent Lefevre]

On 2022-12-16 09:03:39 -0700, Theo de Raadt wrote:
> Vincent Lefevre  wrote:
> 
> > BTW, if developers use an untrusted format string, then sprintf()
> > is unsafe too (possible buffer overflow), and at some point,
> > printf() too.
> 
> what are you trying to say?

According to

  https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2834.htm

it has been proposed to deprecate %n mainly because:

  If the format argument of a printf-style-function originates from an
  unverified source an attacker may be able to write arbitrary values
  to the stack.

But I'm saying that from this point of view, sprintf() is unsafe too,
actually much more unsafe. Something like %n injection in the format
argument can easily be detected (actually any injection of a format
specifier). For instance, if someone writes

  printf (buf);

assuming that the string buf will be output, where buf has untrusted
contents, then if buf contains %n, the number of arguments will be
incorrect, so that this can be detected at run time (ditto if printf
has arguments that correspond to a trusted part of buf). However,
with

  sprintf (s, buf);

(rather than using snprintf), even if the number of arguments is
checked to be correct, there could still be a buffer overflow with
an untrusted buf.

It's a pity that N2834 doesn't give real-world examples of insecure
use of %n, in particular ones that could not be possible to detect
at run time.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: cc claims ISO C99 support, but %n printf format specifier calls abort()

[Theo de Raadt]

Vincent Lefevre  wrote:

> BTW, if developers use an untrusted format string, then sprintf()
> is unsafe too (possible buffer overflow), and at some point,
> printf() too.

what are you trying to say?

are you trying to say everyone including you should review and audit and
re-audit all of them?

or are you saying the opposite: that we should all _give up_ trying to fix
things, because some ISO commitees lack the balls to remove very dangerous
features which noone actually needs, and we should just accept the imperfect
world we live in?

You lost me..

Re: cc claims ISO C99 support, but %n printf format specifier calls abort()

[Theo de Raadt]

Well they need to respond, or openbsd ports needs a diff.

Vincent Lefevre  wrote:

> On 2022-12-15 18:56:15 -0700, Theo de Raadt wrote:
> > There are almost no %n left in the software ecosystem.  If we are able
> > to make this crossing, everyone else is also capable, and eventually
> > will.  Just like with gets().
> 
> FYI, this breaks GMP, whose configure script insists on %n being
> available, otherwise GMP uses its own, buggy implementation of
> vsnprintf, which triggers an assertion failure when %a/%A is used
> (and this bug affects MPFR). AFAIK, the GMP developers haven't
> reacted to the bug report sent in October.
> 
> BTW, if developers use an untrusted format string, then sprintf()
> is unsafe too (possible buffer overflow), and at some point,
> printf() too.
> 
> -- 
> Vincent Lefèvre  - Web: 
> 100% accessible validated (X)HTML - Blog: 
> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: mail(1) "save" command straying from POSIX for missing filename

[Mark Jamsek]

On 22-12-16 02:21AM, Tim Chase wrote:
> According to the POSIX definitions for mail(1) & mailx(1), the
> (s)ave command should save to "mbox" if the filename is not specified
>
> ...
>
> However, when exercising this functionality, mail(1) on OpenBSD
> (also tested on FreeBSD where the same issue manifests[1]) doesn't
> support this:
> 
>   demo$ echo test | mail -s "test" demo # send self a message
>   demo$ mail
>   Mail version 8.1 6/6/93.  Type ? for help.
>   "/var/mail/demo": 1 message 1 new
>   >N  1 d...@localhost.my.do  Thu Dec 15 19:34  19/775   "test"
>   & s
>   No file specified.

Current behaviour comports with the mail(1) manual page, so support for
this may be intentionally elided; I'm not sure. In either case, here's
a minimal diff making the change.

Index: cmd2.c
===
RCS file: /cvs/src/usr.bin/mail/cmd2.c,v
retrieving revision 1.22
diff -u -p -r1.22 cmd2.c
--- cmd2.c  16 Oct 2015 17:56:07 -  1.22
+++ cmd2.c  16 Dec 2022 12:59:21 -
@@ -139,6 +139,7 @@ copycmd(void *v)
 
 /*
  * Save/copy the indicated messages at the end of the passed file name.
+ * If no file name is specified, default to user mbox.
  * If mark is true, mark the message "saved."
  */
 int
@@ -208,10 +209,11 @@ swrite(void *v)
 /*
  * Snarf the file from the end of the command line and
  * return a pointer to it.  If there is no file attached,
- * just return NULL.  Put a null in front of the file
+ * return the mbox file.  Put a null in front of the file
  * name so that the message list processing won't see it,
- * unless the file name is the only thing on the line, in
- * which case, return 0 in the reference flag variable.
+ * unless the file name is the only thing on the line, or
+ * no file was attached, in which case, return 0 in the
+ * reference flag variable.
  */
 char *
 snarf(char *linebuf, int *flag)
@@ -234,8 +236,8 @@ snarf(char *linebuf, int *flag)
while (cp > linebuf && !isspace((unsigned char)*cp))
cp--;
if (*cp == '\0') {
-   puts("No file specified.");
-   return(NULL);
+   *flag = 0;
+   return(expand("&"));
}
if (isspace((unsigned char)*cp))
*cp++ = 0;
Index: mail.1
===
RCS file: /cvs/src/usr.bin/mail/mail.1,v
retrieving revision 1.83
diff -u -p -r1.83 mail.1
--- mail.1  31 Mar 2022 17:27:25 -  1.83
+++ mail.1  16 Dec 2022 12:59:22 -
@@ -633,6 +633,9 @@ retained fields.
 .Pq Ic s
 Takes a message list and a filename and appends each message in
 turn to the end of the file.
+If filename is omitted, the
+.Ar mbox
+file is used.
 The filename in quotes, followed by the line
 count and character count is echoed on the user's terminal.
 .It Ic saveignore

-- 
Mark Jamsek 
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68


signature.asc
Description: PGP signature

Re: cc claims ISO C99 support, but %n printf format specifier calls abort()

[Stuart Henderson]

On 2022/12/16 10:50, Vincent Lefevre wrote:
> On 2022-12-15 18:56:15 -0700, Theo de Raadt wrote:
> > There are almost no %n left in the software ecosystem.  If we are able
> > to make this crossing, everyone else is also capable, and eventually
> > will.  Just like with gets().
> 
> FYI, this breaks GMP, whose configure script insists on %n being
> available, otherwise GMP uses its own, buggy implementation of
> vsnprintf, which triggers an assertion failure when %a/%A is used
> (and this bug affects MPFR). AFAIK, the GMP developers haven't
> reacted to the bug report sent in October.

btw, that doesn't appear to affect the GMP port; the values passed in from
ports infrastructure via config.cache override the autoconf check for %n
(which appears to be trying to detect a bug in Solaris 2.7 on 64-bit SPARC).

> BTW, if developers use an untrusted format string, then sprintf()
> is unsafe too (possible buffer overflow), and at some point,
> printf() too.
> 
> -- 
> Vincent Lefèvre  - Web: 
> 100% accessible validated (X)HTML - Blog: 
> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)
> 

Re: cc claims ISO C99 support, but %n printf format specifier calls abort()

[Vincent Lefevre]

On 2022-12-15 18:56:15 -0700, Theo de Raadt wrote:
> There are almost no %n left in the software ecosystem.  If we are able
> to make this crossing, everyone else is also capable, and eventually
> will.  Just like with gets().

FYI, this breaks GMP, whose configure script insists on %n being
available, otherwise GMP uses its own, buggy implementation of
vsnprintf, which triggers an assertion failure when %a/%A is used
(and this bug affects MPFR). AFAIK, the GMP developers haven't
reacted to the bug report sent in October.

BTW, if developers use an untrusted format string, then sprintf()
is unsafe too (possible buffer overflow), and at some point,
printf() too.

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)