4 ms·

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(a

by kmm 15y ago

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?

apaprocki 15y ago
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.
Karellen 15y ago
"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)
tlb 15y ago
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.
- ZephyrP 15y ago
  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.
  - spullara 15y ago
    This is provided by MySQL, as an example: http://dev.mysql.com/doc/refman/5.0/en/mysql-real-escape-string.html 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.
    - gizzlon 15y ago
      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?
      - rjh29 15y ago
        See "Prepared Statement Support" from http://search.cpan.org/perldoc?DBD::mysql#Class_Methods 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.
        gizzlon 15y ago
        hm.. could be useful information.. thanks
    - gmaslov 15y ago
      Well, just because it really exists doesn't mean it wasn't a silly thing to ask for ;-)

by kmm 15y ago

apaprocki 15y ago

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.

Karellen 15y ago

"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)

tlb 15y ago

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.

ZephyrP 15y ago
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.
- spullara 15y ago
  This is provided by MySQL, as an example: http://dev.mysql.com/doc/refman/5.0/en/mysql-real-escape-string.html 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.
  - gizzlon 15y ago
    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?
    - rjh29 15y ago
      See "Prepared Statement Support" from http://search.cpan.org/perldoc?DBD::mysql#Class_Methods 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.
      - gizzlon 15y ago
        hm.. could be useful information.. thanks
  - gmaslov 15y ago
    Well, just because it really exists doesn't mean it wasn't a silly thing to ask for ;-)