Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Go back to escaping the command args for "sudo -i" and "sudo -s"
before calling the plugin.  Otherwise, spaces in the command args
are not treated properly.  The sudoers plugin will unescape non-spaces
to make matching easier.
  • Loading branch information
millert committed Jul 29, 2011
1 parent 4f9a93f commit 8255ed6
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 48 deletions.
37 changes: 27 additions & 10 deletions plugins/sudoers/sudoers.c
Expand Up @@ -861,21 +861,38 @@ set_cmnd(void)

/* set user_args */
if (NewArgc > 1) {
char *to, **from;
char *to, *from, **av;
size_t size, n;

/* Alloc and build up user_args. */
for (size = 0, from = NewArgv + 1; *from; from++)
size += strlen(*from) + 1;
for (size = 0, av = NewArgv + 1; *av; av++)
size += strlen(*av) + 1;
user_args = emalloc(size);
for (to = user_args, from = NewArgv + 1; *from; from++) {
n = strlcpy(to, *from, size - (to - user_args));
if (n >= size - (to - user_args))
errorx(1, _("internal error, set_cmnd() overflow"));
to += n;
*to++ = ' ';
if (ISSET(sudo_mode, MODE_SHELL|MODE_LOGIN_SHELL)) {
/*
* When running a command via a shell, the sudo front-end
* escapes potential meta chars. We unescape non-spaces
* for sudoers matching and logging purposes.
*/
for (to = user_args, av = NewArgv + 1; (from = *av); av++) {
while (*from) {
if (from[0] == '\\' && !isspace((unsigned char)from[1]))
from++;
*to++ = *from++;
}
*to++ = ' ';
}
*--to = '\0';
} else {
for (to = user_args, av = NewArgv + 1; *av; av++) {
n = strlcpy(to, *av, size - (to - user_args));
if (n >= size - (to - user_args))
errorx(1, _("internal error, set_cmnd() overflow"));
to += n;
*to++ = ' ';
}
*--to = '\0';
}
*--to = '\0';
}
}
if (strlen(user_cmnd) >= PATH_MAX)
Expand Down
25 changes: 18 additions & 7 deletions src/parse_args.c
Expand Up @@ -385,17 +385,28 @@ parse_args(int argc, char **argv, int *nargc, char ***nargv, char ***settingsp,
memcpy(av + 1, argv, argc * sizeof(char *));
} else {
/* shell -c "command" */
char *src, *dst, *end;
char *cmnd, *src, *dst;
size_t cmnd_size = (size_t) (argv[argc - 1] - argv[0]) +
strlen(argv[argc - 1]) + 1;
strlen(argv[argc - 1]) + 1;

cmnd = dst = emalloc2(cmnd_size, 2);
for (av = argv; *av != NULL; av++) {
for (src = *av; *src != '\0'; src++) {
/* quote potential meta characters */
if (!isalnum((unsigned char)*src) && *src != '_' && *src != '-')
*dst++ = '\\';
*dst++ = *src;
}
*dst++ = ' ';
}
if (cmnd != dst)
dst--; /* replace last space with a NUL */
*dst = '\0';

ac = 3;
av = emalloc2(ac + 1, sizeof(char *));
av[1] = "-c";
av[2] = dst = emalloc(cmnd_size);
src = argv[0];
for (end = src + cmnd_size - 1; src < end; src++, dst++)
*dst = *src == '\0' ? ' ' : *src;
*dst = '\0';
av[2] = cmnd;
}
av[0] = (char *)user_details.shell; /* plugin may override shell */
av[ac] = NULL;
Expand Down
31 changes: 0 additions & 31 deletions src/sudo.c
Expand Up @@ -121,7 +121,6 @@ static int iolog_open(struct plugin_container *plugin, char * const settings[],
int argc, char * const argv[], char * const user_env[]);
static void iolog_close(struct plugin_container *plugin, int exit_status,
int error);
static char *escape_cmnd(const char *src);

/* Policy plugin convenience functions. */
static int policy_open(struct plugin_container *plugin, char * const settings[],
Expand Down Expand Up @@ -293,12 +292,6 @@ main(int argc, char *argv[], char *envp[])
if (ISSET(command_details.flags, CD_SUDOEDIT)) {
exitcode = sudo_edit(&command_details);
} else {
if (ISSET(sudo_mode, MODE_SHELL)) {
/* Escape meta chars if running a shell with args. */
if (argv_out[1] != NULL && strcmp(argv_out[1], "-c") == 0 &&
argv_out[2] != NULL && argv_out[3] == NULL)
argv_out[2] = escape_cmnd(argv_out[2]);
}
exitcode = run_command(&command_details);
}
/* The close method was called by sudo_edit/run_command. */
Expand Down Expand Up @@ -1054,30 +1047,6 @@ exec_setup(struct command_details *details, const char *ptyname, int ptyfd)
return rval;
}

/*
* Escape any non-alpha numeric or blank characters to make sure
* they are not interpreted specially by the shell.
*/
static char *
escape_cmnd(const char *src)
{
char *cmnd, *dst;

/* Worst case scenario, we have to escape everything. */
cmnd = dst = emalloc((2 * strlen(src)) + 1);
while (*src != '\0') {
if (!isalnum((unsigned char)*src) && !isspace((unsigned char)*src) &&
*src != '_' && *src != '-') {
/* quote potential meta character */
*dst++ = '\\';
}
*dst++ = *src++;
}
*dst++ = '\0';

return cmnd;
}

/*
* Run the command and wait for it to complete.
*/
Expand Down

18 comments on commit 8255ed6

@IsaccBarker
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@01f464 Indeed, a very very large oopsy woopsy

@sijanec
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really a bug to remember.

@thunderstorm010
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🅾🅾ps

@anon2828226
Copy link

@anon2828226 anon2828226 commented on 8255ed6 Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@millert that is quite an L to be signed on that commit. add it to your twitter bio for the lols. live and learn

@russellvt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Posterity Sake (and credit to Qualys).

@Ewkoll
Copy link

@Ewkoll Ewkoll commented on 8255ed6 Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☁️

@TerjeWiigMathisen
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the original code which was removed: It simply assumed worst case and allocated twice the input buffer size, just in case everything needed to be escaped.
The code is far simpler and easier to understand/verify and the memory overhead is effectively zero for 99.99...% of all uses since it will still be less than a 4KB cache block.

@00100000
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋

@sijanec
Copy link

@sijanec sijanec commented on 8255ed6 Jan 27, 2021 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimdgit
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL Where were you guys 10 years ago to review this commit?
So easy to critique those pulling the weight when you do nothing!

@martinsweitzer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the original code which was removed: It simply assumed worst case and allocated twice the input buffer size, just in case everything needed to be escaped.
The code is far simpler and easier to understand/verify and the memory overhead is effectively zero for 99.99...% of all uses since it will still be less than a 4KB cache block.

Yes. I really wonder why this change was made. Looking at the new code, your eyes glaze over.

@TerjeWiigMathisen
Copy link

@TerjeWiigMathisen TerjeWiigMathisen commented on 8255ed6 Jan 27, 2021 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sijanec
Copy link

@sijanec sijanec commented on 8255ed6 Jan 27, 2021 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anacondaq
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open source as is.

@numanturle
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

burası karışmış yeğen

@francosalcedo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are humans

@thunderstorm010
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

burası karışmış yeğen

Bruh

@felixhalim
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here coz of cs4239

Please sign in to comment.