Page MenuHome GnuPG
Create Task
Maniphest T5104

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

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.

Event Timeline

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

0001-Avoid-printf-n-in-mkdefs.c.patch1 KBDownload
patch misses the type. A better one is
0001-Avoid-printf-n-in-mkdefsinc.c.patch1 KBDownload
.

• werner added a subscriber: • werner.Oct 16 2020, 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.Oct 17 2020, 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.Oct 18 2020, 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.

vext01 added a comment.Oct 18 2020, 11:56 AM

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.

• werner added a comment.Oct 18 2020, 1:08 PM

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.