wrowe 02/03/21 23:53:03
Modified: threadproc/win32 proc.c
Log:
This appears correct, but I need at least three more pairs of eyes that
the cmd.exe path is secure before I'd remove my -1 on beta.
The command.com path gets even more tangled - I don't have the focus
for that this morning.
Revision Changes Path
1.70 +117 -35 apr/threadproc/win32/proc.c
Index: proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/win32/proc.c,v
retrieving revision 1.69
retrieving revision 1.70
diff -u -r1.69 -r1.70
--- proc.c 21 Mar 2002 05:52:14 -0000 1.69
+++ proc.c 22 Mar 2002 07:53:03 -0000 1.70
@@ -282,6 +282,48 @@
return APR_SUCCESS;
}
+static const char* has_space(const char *str)
+{
+ const char *ch;
+ for (ch = str; *ch; ++ch) {
+ if (isspace(*ch)) {
+ return ch;
+ }
+ }
+ return NULL;
+}
+
+static char *apr_caret_escape_args(apr_pool_t *p, const char *str)
+{
+ char *cmd;
+ unsigned char *d;
+ const unsigned char *s;
+
+ cmd = apr_palloc(p, 2 * strlen(str) + 1); /* Be safe */
+ d = (unsigned char *)cmd;
+ s = (const unsigned char *)str;
+ for (; *s; ++s) {
+
+ /*
+ * Newlines to Win32/OS2 CreateProcess() are ill advised.
+ * Convert them to spaces since they are effectively white
+ * space to most applications
+ */
+ if (*s == '\r' || *s == '\n') {
+ *d++ = ' ';
+ continue;
+ }
+
+ if (IS_SHCHAR(*s)) {
+ *d++ = '^';
+ }
+ *d++ = *s;
+ }
+ *d = '\0';
+
+ return cmd;
+}
+
APR_DECLARE(apr_status_t) apr_proc_create(apr_proc_t *new,
const char *progname,
const char * const *args,
@@ -291,11 +333,11 @@
{
apr_status_t rv;
apr_size_t i;
+ const char *argv0;
char *cmdline;
char *pEnvBlock;
PROCESS_INFORMATION pi;
DWORD dwCreationFlags = 0;
- const char *ch;
new->in = attr->parent_in;
new->err = attr->parent_err;
@@ -318,37 +360,38 @@
}
}
- /* progname must be the unquoted, actual name, or NULL if this is
- * a 16 bit app running in the VDM or WOW context.
- */
- if (progname[0] == '\"') {
- progname = apr_pstrndup(pool, progname + 1, strlen(progname) - 2);
- }
-
/* progname must be unquoted, in native format, as there are all sorts
* of bugs in the NT library loader code that fault when parsing '/'.
* We do not directly manipulate cmdline, and it will become a copy,
* so we've casted past the constness issue.
+ * XXX progname must be NULL if this is a 16 bit app running in WOW
*/
- if (strchr(progname, ' '))
- cmdline = apr_pstrcat(pool, "\"", progname, "\"", NULL);
- else
- cmdline = (char*)progname;
-
- i = 1;
- while (args && args[i]) {
- for (ch = args[i]; *ch; ++ch) {
- if (isspace(*ch)) {
- break;
- }
- }
- if (*ch) {
+ if (progname[0] == '\"') {
+ progname = apr_pstrndup(pool, progname + 1, strlen(progname) - 2);
+ }
+
+ if ((rv = apr_filepath_merge(&cmdline, attr->currdir, progname,
+ APR_FILEPATH_NATIVE, pool)) != APR_SUCCESS)
{
+ return rv;
+ }
+ progname = cmdline;
+
+ if (has_space(progname)) {
+ argv0 = apr_pstrcat(pool, "\"", progname, "\"", NULL);
+ }
+ else {
+ argv0 = progname;
+ }
+
+ /* Handle the args, seperate from argv0 */
+ cmdline = "";
+ for (i = 1; args && args[i]; ++i) {
+ if (has_space(args[i])) {
cmdline = apr_pstrcat(pool, cmdline, " \"", args[i], "\"", NULL);
}
else {
cmdline = apr_pstrcat(pool, cmdline, " ", args[i], NULL);
}
- i++;
}
/* ### how to handle APR_PROGRAM_ENV and APR_PROGRAM_PATH? */
@@ -363,12 +406,7 @@
}
else {
progname = shellcmd;
- for (ch = shellcmd; *ch; ++ch) {
- if (isspace(*ch)) {
- break;
- }
- }
- if (*ch) {
+ if (has_space(shellcmd)) {
shellcmd = apr_pstrcat(pool, "\"", shellcmd, "\"", NULL);
}
}
@@ -376,10 +414,10 @@
*/
i = strlen(progname);
if (i >= 11 && strcasecmp(progname + i - 11, "command.com") == 0) {
- cmdline = apr_pstrcat(pool, shellcmd, " /C ", cmdline, NULL);
+ cmdline = apr_pstrcat(pool, shellcmd, " /C ", argv0, cmdline,
NULL);
}
else {
- cmdline = apr_pstrcat(pool, shellcmd, " /C \"", cmdline, "\"",
NULL);
+ cmdline = apr_pstrcat(pool, shellcmd, " /C \"", argv0, cmdline,
"\"", NULL);
}
}
else {
@@ -389,13 +427,55 @@
* ###: This solution isn't much better - it may defeat path
searching
* when the path search was desired. Open to further discussion.
*/
- char *progpath;
- apr_filepath_merge(&progpath, attr->currdir, progname,
- APR_FILEPATH_NATIVE, pool);
- progname = progpath;
+ i = strlen(progname);
+ if (i >= 4 && (strcasecmp(progname + i - 4, ".bat") == 0
+ || strcasecmp(progname + i - 4, ".cmd") == 0))
+ {
+ char *shellcmd = getenv("COMSPEC");
+ if (!shellcmd) {
+ return APR_EINVAL;
+ }
+ if (shellcmd[0] == '"') {
+ progname = apr_pstrndup(pool, shellcmd + 1, strlen(shellcmd)
- 2);
+ }
+ else {
+ progname = shellcmd;
+ if (has_space(shellcmd)) {
+ shellcmd = apr_pstrcat(pool, "\"", shellcmd, "\"", NULL);
+ }
+ }
+ i = strlen(progname);
+ if (i >= 11 && strcasecmp(progname + i - 11, "command.com") ==
0) {
+ cmdline = apr_pstrcat(pool, shellcmd, " /C ", argv0,
cmdline, NULL);
+ }
+ else {
+ /* We must protect the cmdline args from any interpolation -
this
+ * is not a shellcmd, and the source of argv[] is untrusted.
+ * Notice we escape ALL the cmdline args, including the
quotes
+ * around the individual args themselves. No sense in
allowing
+ * the shift-state to be toggled, and the application will
+ * not see the caret escapes.
+ */
+ cmdline = apr_caret_escape_args(pool, cmdline);
+ /*
+ * Our app name must always be quoted so the quotes
surrounding
+ * the entire /c "command args" are unambigious.
+ */
+ if (*argv0 != '"') {
+ cmdline = apr_pstrcat(pool, shellcmd, " /C \"\"", argv0,
"\"", cmdline, "\"", NULL);
+ }
+ else {
+ cmdline = apr_pstrcat(pool, shellcmd, " /C \"", argv0,
cmdline, "\"", NULL);
+ }
+ }
+ }
+ else {
+ /* A simple command we are directly invoking
+ */
+ cmdline = apr_pstrcat(pool, argv0, cmdline, NULL);
+ }
}
-
if (!env)
pEnvBlock = NULL;
else {
@@ -457,6 +537,8 @@
}
#endif /* APR_HAS_ANSI_FS */
}
+
+ new->invoked = cmdline;
#if APR_HAS_UNICODE_FS
IF_WIN_OS_IS_UNICODE
