Hacker News Reader

Comment by kmm | Hacker News Reader

In the source of sudo-1.8.3p1, the error is in src/sudo.c at line 1219. Relevant code snippet:

  easprintf(&fmt2, "%s: %s\n", getprogname(), fmt);
  va_start(ap, fmt);
  vfprintf(stderr, fmt2, ap);
  va_end(ap);
  efree(fmt2);

Obviously, fmt2 can still contain a format string, yet it is printed anyway with vfprintf. I'm really surprised this type of bug still exists. Doesn't every decent compiler warn the developer about this?

View on HN

It just goes to show that all project owners should be responsible for turning on maximum compiler warnings which make sense for their project. There are lots of great gcc warnings/errors which are not enabled by default. In particular, this one is reported by -Wformat-nonliteral or -Wformat-security. From the gcc docs:

  -Wformat is included in -Wall. For more control over some 
  aspects of format checking, the options -Wformat-y2k,
  -Wno-format-extra-args, -Wno-format-zero-length,
  -Wformat-nonliteral, -Wformat-security, and -Wformat=2
  are available, but are not included in -Wall.

"fmt2" is supposed to contain a format string. It's supposed to contain the format string from "fmt". That's how it prints the args from "ap".

The bug is when getprogname() also returns a string containing a format specifier (e.g. "%s"), which is not expected.

Clearly the fix is:

  fprintf(stderr, "%s: ", getprogname());
  va_start(ap, fmt);
  vfprintf(stderr, fmt, ap);
  va_end(ap);
  fprintf(stderr, "\n");

(Edit for formatting, clarity)

The real crime is creating a library function like printf without also creating a library function to escape format strings. Leaving safe escaping as an exercise for the application programmer clearly doesn't work.

This is like asking for SQL databases to have a SQL-Injection escaping function. You 'escape' format string attacks by using the C language in a correct fashion.

This is provided by MySQL, as an example:

http://dev.mysql.com/doc/refman/5.0/en/mysql-real-escape-str...

I'm sure what you meant was a server-side escaping function which is obviously pointless, but I did want to point out that MySQL's client driver does ship with one that you can use so you don't have to write your own.

Actually, isn't the escaping done serversiden when using placeholders?

At least in Perl, there's no need to escape placeholder values explicitly. I believe the prepared statement is stored on the server, and then the values sent later. Of course the escaping could be the work of the driver.. anyone know?

See "Prepared Statement Support" from http://search.cpan.org/perldoc?DBD::mysql#Class_Methods

Prepared statements are disabled by default and require MySQL >= 4.1.3. Instead, the driver does the placeholder replacement on the client, and prepare() does nothing clever at all.

hm.. could be useful information.. thanks

Building dynamic SQL strings for comparison can sometimes be handy (e.g. with LIKE), so it's really too bad there's no server-side SQL escape function, at least in the sql dialects I'm familiar with.

Well, just because it really exists doesn't mean it wasn't a silly thing to ask for ;-)

Yes, client libraries for SQL databases should absolutely have an escaping function. The fact that some don't is responsible for a significant fraction of all security failures.

Your suggestion makes no sense.

If you don't want to use a format string, use a function that takes a regular string, like puts, instead of a vprintf variant. Why would anyone go to the trouble of escaping a format string so they can use it with a function whose sole purpose is to parse format strings? (Which you can already do, by the way, by using vprintf correctly.)

Reply on news.ycombinator.com