LWN: Comments on "Format string vulnerability in sudo"

Format string vulnerability in sudo

cmccabe — Fri, 03 Feb 2012 19:07:03 +0000

> You *definitely* don't want to look in dirname(argv[0])
> for configuration files, for several reasons: argv[0]
> need not contain a path at all, it might contain a
> user-controlled path that you don't trust, and on most
> systems the path containing the binary will never contain
> configuration files.

Well, I would definitely never suggest doing this for setuid binaries, like sudo. I did it once for with scripts that I wrote and it worked out fine.

Format string vulnerability in sudo

josh — Fri, 03 Feb 2012 08:54:09 +0000

I don't consider hardcoding the name an arbitrary limitation; the program *does* know its own name, it just doesn't change that name based on its filename. :)

Libraries ought to accept the program name as a parameter; the program could choose to pass them argv[0] if it wants to, but might want to pass something else (a subcommand name for instance), or just pass the compile-time name of the program.

You *definitely* don't want to look in dirname(argv[0]) for configuration files, for several reasons: argv[0] need not contain a path at all, it might contain a user-controlled path that you don't trust, and on most systems the path containing the binary will never contain configuration files. Most programs get their configuration path at compile-time; programs using autotools get it from the ./configure command line.

As for making behavior conditional on the program name, that makes sense for something like busybox that needs to save space at all costs, but most programs don't do that anymore due to various logistical complications, preferring instead to have the same behavior no matter how they get invoked. For example, these days gunzip refers to a shell script rather than a hardlink to gzip.

I don't necessarily consider argv[0] a universally bad thing to rely on, but I don't think it should get used and relied on as often as it does.

Format string vulnerability in sudo

cmccabe — Fri, 03 Feb 2012 06:44:02 +0000

Sorry for the perhaps overly adversarial tone of the above comment. I'm just annoyed that people could genuinely think argv[0] was a bad thing. Meh.

Format string vulnerability in sudo

cmccabe — Fri, 03 Feb 2012 06:36:03 +0000

I don't see the point of arbitrarily limiting the ability of sysadmins to rename programs. Why hardcode the name when you don't have to? Do you enjoy adding arbitrary limitations just because you can?

Even if we ignore the rename issue, there are plenty of reasons why a program might want to know its own name.

One of them is if you are writing an argument parsing library intended to be used by many different programs. You won't know the program name ahead of time, and you want to print it out in the usage message.

Another is if you want the same program to have different behaviors based on the name it was invoked with. Busybox has used this trick for more than a decade to avoid the space overhead of having a different binary for each command.

If you're searching for configuration files, one place you might want to look is in dirname(argv[0]).

Just because you don't have the imagination to think of good uses for argv[0], doesn't mean that they don't exist.

Format string vulnerability in sudo

rqosa — Wed, 01 Feb 2012 20:02:47 +0000

(And then have the log messages from that development build show up under the chosen name.)

Format string vulnerability in sudo

rqosa — Wed, 01 Feb 2012 20:01:41 +0000

Note, though, that it says that the sudo_debug() logging facility is intended for use by developers of sudo itself (and its plugins). I suppose they might want to name their development/debugging builds of sudo differently from the main systemwide installation of sudo.

Format string vulnerability in sudo

wahern — Wed, 01 Feb 2012 19:22:32 +0000

Nevermind me. I originally read your post to argue that programs shouldn't prepend their name at all.

Although, calling themselves something other than how they were invoked would be quite confusing, IMO.

Format string vulnerability in sudo

wahern — Wed, 01 Feb 2012 19:17:14 +0000

Well behaved programs are always supposed to prepend their name to stderr messages. Otherwise in a pipeline of commands (or a tree, if they call each other), you can't tell where a particular error is coming from.

It's usually far easier to just use warn(3) and warnx(3) from <err.h>, which prepend the program name. Or getprogname() (*BSD) or program_invocation_short_name (glibc) if you're rolling your own formatter. But sudo probably needs to be much more portable than than those allow.

Format string vulnerability in sudo

ekj — Wed, 01 Feb 2012 08:39:12 +0000

Agreed. There's absolutely nothing wrong with this. If I rename my Firefox-executable to ArthyMcChicken, the help menu is still gonna say "About Firefox", not magically "About ArthyMcChicken", the program *is* still Firefox regardless of what I named my executable file.

Similarily, the program is still the sudo-program, even if I rename the executable to carbonated_kryptonite

Format string vulnerability in sudo

josh — Wed, 01 Feb 2012 08:10:57 +0000

I certainly agree that all of the methods you mentioned seem worse than using argv[0].

I don't, however, see the point of trying to adapt to sysadmins blindly renaming executables. The program is called "sudo", and it should refer to itself as such. I see nothing wrong with this:

~$ strangename foo
sudo: foo: command not found

Sysadmins should not blindly rename executables without informing those executables at compile time. Autoconf makes renaming executables fairly straightforward; just look at --program-transform-name and similar, which exists for crazy sites that install GNU programs with a 'g' prefix, such as gsed. And if you use a packaging system, you shouldn't bypass the packaging system by renaming any files it installed.

Format string vulnerability in sudo

cmccabe — Wed, 01 Feb 2012 08:00:47 +0000

A compile-time define is not going to help if a sysadmin renames the program.

And there isn't any other way to get the name of a running process without resorting to horrible platform-specific hacks.

Before you comment: yes, I'm aware of prctl, which allows you to get the initial 15 bytes of the name, and reading /proc/cmdline, and even running ps and grepping for the process name. They are all much worse than just using argv[0].

Format string vulnerability in sudo

iabervon — Tue, 31 Jan 2012 19:31:09 +0000

The problem is that they've got a:


sudo_debug(level, fmt, ...)

function which wants to do a single write() and get the executable name at the beginning, following printf() rules for the fmt.

They probably should have done it by using the preprocessor to paste "%s" into the beginning of the format string and the call to get the executable name into the beginning of the arguments (which limits callers to string literals for the format, but that shouldn't be a problem).

Format string vulnerability in sudo

jengelh — Tue, 31 Jan 2012 16:43:59 +0000

sudo suffers from many other problems (also leading to a crash or whacky behavior) bugs.debian.org/648066, I wish they'd get as much attention as a simple format string-induced crash.

Format string vulnerability in sudo

gowen — Tue, 31 Jan 2012 13:50:12 +0000

Fair enough, but I don't think you make that case in a very compelling way. This format string vulnerability is very little to do with argv[0] - its the attack vector in this case, but the bug is to do with the passing an unsanitised user_string (regardless of where it came from) as the first argument of printf(user_string,....).

And I like that /bin/sh gets me a more POSIXy shell that /bin/bash, without needing two executables.

Format string vulnerability in sudo

gowen — Tue, 31 Jan 2012 13:46:51 +0000

It's only better if you want the newline. And if you're using it to print argv[0] before an error message, you probably don't want the newline...

Format string vulnerability in sudo

josh — Tue, 31 Jan 2012 13:46:32 +0000

I agree; I just also wanted to point out that argv[0] seems like a notable thing to avoid using.

Format string vulnerability in sudo

josh — Tue, 31 Jan 2012 13:45:56 +0000

GCC will actually turn calls to printf into calls to puts automatically when possible.

Format string vulnerability in sudo

josh — Tue, 31 Jan 2012 13:44:06 +0000

Use a compile-time #define then; autotools even provides you with one automatically.

Format string vulnerability in sudo

epa — Tue, 31 Jan 2012 11:59:38 +0000

or better just puts(user_string);

Format string vulnerability in sudo

gowen — Tue, 31 Jan 2012 10:25:36 +0000

That's not really the point though. The point is its always a bug to do:

foo(const char* user_string)
{
printf(user_string);
}

when what you mean is:

foo(const char* user_string)
{
printf("%s",user_string);
}

Format string vulnerability in sudo

alonz — Tue, 31 Jan 2012 09:59:57 +0000

But then you run into trouble if somebody renames the program – e.g. because of a name clash with some other program, or in order to install two versions of the same tool concurrently.

argv[0] is more robust.

Format string vulnerability in sudo

josh — Tue, 31 Jan 2012 09:00:39 +0000

I do understand what programs typically use argv[0] for, but I don't see the point. If you don't change the behavior of your program based on argv[0], then why bother printing argv[0] in your usage or your messages? Just print the name of your program, which you already know at compile time.

Format string vulnerability in sudo

zuki — Tue, 31 Jan 2012 08:59:18 +0000

> One thing argv[0] is often used for is to allow a program to print its own
> name, such as in the "usage" message that's printed in response to the
> "-?" switch, or in "trace" messages used for debugging.
This doesn't make much sense. What is the gain? Saving 5 bytes on the name string? Of course there are some exceptions, like programs which vary their behaviour based on argv[0], or which can legitimately be installed under different names, but 99% of programs are better of printing their real name, even if called differently. E.g. if the user specifies the full path ../../usr/bin/sudo, it's inelegant to say

usage: ../../usr/bin/sudo [options]

(Besides the user probably knows how he or she called the program.)

Format string vulnerability in sudo

Adrien — Tue, 31 Jan 2012 07:44:26 +0000

GCC should also be able to pick up the vulnerability with -Wformat family of warnings.

Code:
| % cat a.c
| #include <stdio.h>
|
| void f(char *fmt) {
| printf(fmt, 1);
| }

Compilation:
| % gcc -c -Wformat -Wformat-nonliteral a.c
| a.c: In function ‘f’:
| a.c:4:3: warning: format not a string literal, argument types not checked

-Wformat is enabled by -Wall but -Wformat-nonliteral is not (not by -Wextra either). Still, it's really something that should not be skipped (along with a number of other warnings; C developers: USE THE WARNINGS!).

For reference, from gcc's manpage:
| -Wformat-nonliteral
| If -Wformat is specified, also warn if the format string is not a
| string literal and so cannot be checked, unless the format function
| takes its format arguments as a "va_list".

Format string vulnerability in sudo

rqosa — Tue, 31 Jan 2012 07:10:50 +0000

> Honestly, programs need to stop looking at argv[0] at all.

Using argv[0] doesn't necessarily cause a vulnerability — it's no more of a hazard than the other elements of argv. It just needs to be treated like any other untrusted data source (which in this case probably means it should have been used as one of the values to which the format string was bound, rather than part of the format string itself).

> I don't know why any program looks at argv[0] at all

One thing argv[0] is often used for is to allow a program to print its own name, such as in the "usage" message that's printed in response to the "-?" switch, or in "trace" messages used for debugging. I'm guessing that the latter usage is what sudo uses argv[0] for — the vulnerability report says that it's in a function called sudo_debug() which is intended for use "when developing policy or I/O logging plugins". So, probably sudo_debug() is a function that plugins call in order to print trace messages, and it uses argv[0] to prepend something like "sudo: " to the messages before printing them.

Format string vulnerability in sudo

josh — Tue, 31 Jan 2012 05:01:10 +0000

Honestly, programs need to stop looking at argv[0] at all. Most programs have stopped changing their behavior based on argv[0] these days, preferring instead to use tiny wrappers, shared libraries, or two compilations of the same code with different preprocessor definitions. Given that, I don't know why any program looks at argv[0] at all, certainly not sudo.

Format string vulnerability in sudo

mgedmin — Mon, 30 Jan 2012 23:37:48 +0000

The linked article says _FORTIFY_SOURCE helps here:

"On systems that support FORTIFY_SOURCE (most Linux and NetBSD), adding -D_FORTIFY_SOURCE=2 to the OSDEFS line in src/Makfile and then rebuilding sudo will prevent the bug from being exploited."

Format string vulnerability in sudo

mathstuf — Mon, 30 Jan 2012 23:31:14 +0000

It's 2. Build logs here[1].

[1]http://kojipkgs.fedoraproject.org/packages/sudo/1.8.3p1/2...

Format string vulnerability in sudo

SEJeff — Mon, 30 Jan 2012 23:11:43 +0000

What is the Fedora/RHEL default? I know that they both do enable FORTIFY_SOURCE by default, just not sure what level.

Format string vulnerability in sudo

pr1268 — Mon, 30 Jan 2012 23:00:53 +0000

$ sudo -V Sudo version 1.7.4p6

Thank goodness Slackware is a few versions behind bleeding edge! :-D

Yes, I did read the part about "...that does not mean that such exploits do not exist."

Format string vulnerability in sudo

arjan — Mon, 30 Jan 2012 22:33:14 +0000

one note on format string issues...

if you compile with -D_FORTIFY_SOURCE=2, glibc's printf and co do some extra checks that will block most format string sploits.
Not sure if this one gets blocked (since it's only in printf and co)