su.c: replace getopt with ad-hoc flag processing
In preparation for supporting --exec I was testing the robustness
of "--" handling and it became apparent that things are currently
a bit broken in `su`.
Since "--" is currently of limited utility, as the subsequent
words are simply passed to the shell after "-c","command_string",
it seems to have gone unnoticed for ages.
However, with --exec, it's expected that "--" would be an almost
required separator with every such usage, considering the
following flags must be passed verbatim to execve() and will
likely begin with hyphens looking indistinguishable from any
other flags in lieu of shell interpolation to worry about.
For some practical context of the existing situation, this
invocation doesn't work today:
```
$ su --command ls -- flags for shell
No passwd entry for user 'flags'
$
```
This should just run ls as root with "flags","for","shell"
forwarded to the shell after "-c","ls".
The "--" should block "flags" from being treated as the user.
That particular issue isn't a getopt one per-se, it's arguably
just a bug in su.c's implementation.
It *seemed* like an easy fix for this would be to add a check if
argv[optind-1] were "--" before treating argv[optind] as USER.
But testing that fix revealed getopt was rearranging things when
encountering "--", the "--" would always separate the handled
opts from the unhandled ones. USER would become shifted to
*after* "--" even when it occurred before it!
If we change the command to specify the user, it works as-is:
```
$ su --command ls root -- flags for shell
Password:
testfile
$
```
But what's rather surprising is how that works; the argv winds up:
"su","--command","ls","--","root","flags","for","shell"
with optind pointing at "root".
That arrangement of argv is indistinguishable from omitting the
user and having "root","flags","for","shell" as the stuff after
"--".
This makes it non-trivial to fix the bug of omitting user
treating the first word after "--" as the user, which one could
argue is a potentially serious security bug if you omit the user,
expect the command to run as root, and the first word after "--"
is a valid user, and what follows that something valid and
potentially destructive not only running in unintended form but
as whatever user happened to be the first word after "--".
So, it seems like something important to fix, and getopt seems to
be getting in the way of fixing it properly without being more
trouble than replacing getopt.
In disbelief of what I was seeing getopt doing with argv here, I
took a glance at the getopt source and found the following:
```
/* The special ARGV-element '--' means premature end of options.
Skip it like a null option,
then exchange with previous non-options as if it were an option,
then skip everything else like a non-option. */
if (d->optind != argc && !strcmp (argv[d->optind], "--"))
```
I basically never use getopt personally because ages ago it
annoyed me with its terrible API for what little it brought to
the table, and this brings it to a whole new level of awful.
This commit is contained in:
Vito Caputo
Serge Hallyn
parent
a271076041
commit
dc732e7734
109
src/su.c
109
src/su.c
@ -55,7 +55,6 @@
|
||||
|
||||
#ident "$Id$"
|
||||
|
||||
#include <getopt.h>
|
||||
#include <grp.h>
|
||||
#include <pwd.h>
|
||||
#include <signal.h>
|
||||
@ -95,6 +94,7 @@ static bool doshell = false;
|
||||
static bool fakelogin = false;
|
||||
static /*@observer@*/const char *shellstr;
|
||||
static /*@null@*/char *command = NULL;
|
||||
static int optidx;
|
||||
|
||||
|
||||
/* not needed by sulog.c anymore */
|
||||
@ -760,6 +760,48 @@ static void save_caller_context (char **argv)
|
||||
pw_free (pw);
|
||||
}
|
||||
|
||||
/*
|
||||
* flags_match - test arg against flag candidates
|
||||
*/
|
||||
static bool flags_match(const char *arg, const char *a, const char *b, const char *c)
|
||||
{
|
||||
return (a != NULL && strcmp (arg, a) == 0) ||
|
||||
(b != NULL && strcmp (arg, b) == 0) ||
|
||||
(c != NULL && strcmp (arg, c) == 0);
|
||||
}
|
||||
|
||||
/* is_flag_like - test if arg resembles a flag
|
||||
*
|
||||
* lone "--" and bare leading-hyphen-less words are not flag-like,
|
||||
* everything else is considered a probable flag.
|
||||
*/
|
||||
static bool is_flag_like(const char *arg)
|
||||
{
|
||||
if (arg[0] != '-')
|
||||
return false;
|
||||
|
||||
if (strcmp (arg, "--") == 0)
|
||||
return false;
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
static void flag_arg_required(const char *arg)
|
||||
{
|
||||
fprintf (stderr,
|
||||
_("%s: option \'%s\' requires an argument\n"),
|
||||
Prog, arg);
|
||||
usage (E_USAGE);
|
||||
}
|
||||
|
||||
static void flag_unknown(const char *arg)
|
||||
{
|
||||
fprintf (stderr,
|
||||
_("%s: unrecognized option \'%s\'\n"),
|
||||
Prog, arg);
|
||||
usage (E_BAD_ARG);
|
||||
}
|
||||
|
||||
/*
|
||||
* process_flags - Process the command line arguments
|
||||
*
|
||||
@ -769,51 +811,41 @@ static void save_caller_context (char **argv)
|
||||
*/
|
||||
static void process_flags (int argc, char **argv)
|
||||
{
|
||||
int c;
|
||||
static struct option long_options[] = {
|
||||
{"command", required_argument, NULL, 'c'},
|
||||
{"help", no_argument, NULL, 'h'},
|
||||
{"login", no_argument, NULL, 'l'},
|
||||
{"preserve-environment", no_argument, NULL, 'p'},
|
||||
{"shell", required_argument, NULL, 's'},
|
||||
{NULL, 0, NULL, '\0'}
|
||||
};
|
||||
for (optidx = 1; optidx < argc; optidx++) {
|
||||
const char *arg = argv[optidx];
|
||||
|
||||
while ((c = getopt_long (argc, argv, "c:hlmps:",
|
||||
long_options, NULL)) != -1) {
|
||||
switch (c) {
|
||||
case 'c':
|
||||
command = optarg;
|
||||
break;
|
||||
case 'h':
|
||||
if (flags_match (arg, "--command", "-c", NULL)) {
|
||||
if (optidx == argc - 1) {
|
||||
flag_arg_required (arg);
|
||||
}
|
||||
|
||||
command = argv[++optidx];
|
||||
} else if (flags_match (arg, "--help", "-h", NULL)) {
|
||||
usage (E_SUCCESS);
|
||||
break;
|
||||
case 'l':
|
||||
} else if (flags_match (arg, "--login", "-l", "-")) {
|
||||
fakelogin = true;
|
||||
break;
|
||||
case 'm':
|
||||
case 'p':
|
||||
} else if (flags_match (arg, "--preserve-environment", "-p", "-m")) {
|
||||
/* This will only have an effect if the target
|
||||
* user do not have a restricted shell, or if
|
||||
* su is called by root.
|
||||
*/
|
||||
change_environment = false;
|
||||
} else if (flags_match (arg, "--shell", "-s", NULL)) {
|
||||
if (optidx == argc - 1) {
|
||||
flag_arg_required (arg);
|
||||
}
|
||||
|
||||
shellstr = argv[++optidx];
|
||||
} else if (is_flag_like (arg)) {
|
||||
flag_unknown (arg);
|
||||
} else {
|
||||
break;
|
||||
case 's':
|
||||
shellstr = optarg;
|
||||
break;
|
||||
default:
|
||||
usage (E_USAGE); /* NOT REACHED */
|
||||
}
|
||||
}
|
||||
|
||||
if ((optind < argc) && (strcmp (argv[optind], "-") == 0)) {
|
||||
fakelogin = true;
|
||||
optind++;
|
||||
}
|
||||
|
||||
if (optind < argc) {
|
||||
STRFCPY (name, argv[optind++]); /* use this login id */
|
||||
/* if next arg is not "--", treat as USER */
|
||||
if (optidx < argc && strcmp (argv[optidx], "--")) {
|
||||
STRFCPY (name, argv[optidx++]); /* use this login id */
|
||||
}
|
||||
if ('\0' == name[0]) { /* use default user */
|
||||
struct passwd *root_pw = getpwnam ("root");
|
||||
@ -829,7 +861,12 @@ static void process_flags (int argc, char **argv)
|
||||
}
|
||||
}
|
||||
|
||||
doshell = (argc == optind); /* any arguments remaining? */
|
||||
/* if more and next arg is "--", skip it and leave rest as-is */
|
||||
if (optidx < argc && strcmp (argv[optidx], "--") == 0) {
|
||||
optidx++;
|
||||
}
|
||||
|
||||
doshell = (argc == optidx); /* any arguments remaining? */
|
||||
if (NULL != command) {
|
||||
doshell = false;
|
||||
}
|
||||
@ -1178,7 +1215,7 @@ int main (int argc, char **argv)
|
||||
if (!doshell) {
|
||||
int err;
|
||||
/* Position argv to the remaining arguments */
|
||||
argv += optind;
|
||||
argv += optidx;
|
||||
if (NULL != command) {
|
||||
argv -= 2;
|
||||
argv[0] = "-c";
|
||||
|
||||
Loading…
Reference in New Issue
Block a user