Consider removing %n uses from printf-like functions
Closed, WontfixPublic

Description

printf("%n") is considered a security risk since some time already, it can be used to rewrite memory. Multiple printf implementations added some restrictions on this (Android libc aborts on %n, Windows Vista and later require explicit opt in, glibc and OSX require format strings to be in readonly memory).

Considering that GnuPG is a security-critical application it would benefit from avoiding dangerous techniques.

By modest assessment there are only a handful of uses of printf-%n in all of GnuPG.
./doc/mkdefsinc.c:289: snprintf (p, n, "%d %n%s %d",
./g10/keylist.c:277: tty_fprintf (fp, "%s%c %s/%s %n",
./agent/protect.c:568: ("(9:protected%d:%s((4:sha18:%n_8bytes_%u:%s)%d:%n%*s)%d:%n%*s)",

The first one is easy to fix. I'll post a patch.

gnezdo created this task.Thu, Oct 15, 8:51 AM
gnezdo added a comment.EditedThu, Oct 15, 8:54 AM

patch misses the type. A better one is .

werner added a subscriber: werner.Fri, Oct 16, 9:07 AM

Sorry, it is entirely non-sense to ban useful printf features. Also note that we use our own printf implementation to avoid portability problems with for example "%zu". If you have a problem with any of the uses of "%n", please explain the problem.

vext01 added a subscriber: vext01.Sat, Oct 17, 8:34 PM

Hi Werner,

I'm with @gnezdo on this one. It seems that %n for printf(3) is problematic in the same way that scanf(3) is: it can easily be accidentally misused and lead to stack corruption.

I did a quick Internet search for this feature and found several blog posts warning against it, and that "Starting with Visual Studio 2005, the capability of using %n is off by default". I guess that's one of the reasons you had to use your own implementation.

I think we should avoid it, even if it's only used in build steps and especially if it isn't portable.

Thanks :)

werner closed this task as Wontfix.Sun, Oct 18, 9:50 AM
werner claimed this task.

Nope %n works on all implementations I am aware of. It has to because it is part of even C90.

Fair enough with regards to portability, and this is not a hill I will die on, but can you comment on the security concerns of using %n?

I notice that clang will by default emit a warning (but not abort compilation) if either the type of a %n variable is wrong or too few arguments are supplied to sprintf. Modern versions of GCC however, (I used 8.4.0), don't even warn about these mistakes.

gcc also warns about missing arguments and hopefully also if the arg for %n is not an int*.
You may need to enable these warnings which we do at least in maintainer-mode. On Windows some of the warnings might be wrong because mingw assumes the MS implementation.

I don't know about the security concerns - you need to use it the right way of course. The sscanf case is different, though.