Skip to content

session process path changes #319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 4, 2018
Merged

session process path changes #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
da73811
C1
Jun 3, 2018
d2b02a3
C2
Jun 4, 2018
3a02758
C3
Jun 4, 2018
0b76a2b
C4
Jun 4, 2018
6925206
C5
Jun 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 175 additions & 0 deletions contrib/win32/win32compat/misc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1725,3 +1725,178 @@ get_user_sid(char* name)

return ret;
}

/* builds session commandline. returns NULL with errno set on failure, caller should free returned string */
char*
build_session_commandline(const char *shell, const char* shell_arg, const char *command, int pty)
{
enum sh_type { SH_CMD, SH_PS, SH_WSL_BASH, SH_CYGWIN, SH_OTHER } shell_type = SH_OTHER;
enum cmd_type { CMD_OTHER, CMD_SFTP, CMD_SCP } command_type = CMD_OTHER;
char *progdir = w32_programdir(), *cmd_sp = NULL, *cmdline = NULL, *ret = NULL, *p;
int len, progdir_len = (int)strlen(progdir);

#define CMDLINE_APPEND(P, S) \
do { \
int _S_len = (int)strlen(S); \
memcpy((P), (S), _S_len); \
(P) += _S_len; \
} while(0)

/* get shell type */
if (strstr(shell, "system32\\cmd"))
Copy link

Choose a reason for hiding this comment

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

It will help us in long run if we move this to a function that returns the enum.

Copy link
Author

Choose a reason for hiding this comment

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

will retain this for now. IT should be easy to move it out if needed.

shell_type = SH_CMD;
else if (strstr(shell, "powershell"))
shell_type = SH_PS;
else if (strstr(shell, "system32\\bash"))
Copy link

Choose a reason for hiding this comment

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

Can you check if this holds good on IOT when privsep is ON for all the SKUs.

Copy link
Author

Choose a reason for hiding this comment

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

sure

shell_type = SH_WSL_BASH;
else if (strstr(shell, "cygwin"))
shell_type = SH_CYGWIN;

/* special case where incoming command needs to be adjusted */
do {
/*
* identify scp and sftp sessions
* we want to launch scp and sftp executables from the same binary directory
* that sshd is hosted in. This will facilitate hosting and evaluating
* multiple versions of OpenSSH at the same time.
*
* currently we can only accomodate this for cmd.exe, since cmd.exe simply executes
* its commandline without applying CRT or shell specific rules
*
* Ex.
* this works
* cmd /c "c:\program files\sftp" -d
* this following wouldn't work in powershell, cygwin's or WSL bash unless we put
* some shell specific rules
* powershell -c "c:\program files\scp" -t
* cygwin\bash -c "c:\program files\scp" -t
* bash -c "c:\program files\scp" -t
*
* for now, for all non-cmd shells, we launch scp and sftp-server directly -
* shell -c "scp.exe -t"
* note that .exe extension and case matters for WSL bash
* note that double quotes matter for WSL and Cygwin bash, they dont matter for PS
*
* consequence -
* for non-cmd shells - sftp and scp installation path is expected to be in machine wide PATH
*
*/

int command_len;
const char *command_args = NULL;

if (!command)
break;
command_len = (int)strlen(command);

if (command_len >= 13 && _memicmp(command, "internal-sftp", 13) == 0) {
Copy link

Choose a reason for hiding this comment

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

instead of hardcoding 13.. have strlen(internal-sftp) in a variable and use..
same on line 1794, 1798, 1802, 1806

Copy link
Author

Choose a reason for hiding this comment

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

agree I'm being stingy due to strlen perf. Since these are established constants, I'll leave them as they are.

Copy link

Choose a reason for hiding this comment

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

I know you already merged this, but per @bagajjal's comment, you can also do something like #define STRINGLEN(x) (sizeof(x)/sizeof(x[0])-1) which will evaluate string literal lengths at compile time. Hardcoding string lengths is just painful for maintainability.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Thanks for the tip. I like how its readable with no perf impact. I'll fix it in a subsequent PR.

command_type = CMD_SFTP;
command_args = command + 13;
} else if (command_len >= 11 && _memicmp(command, "sftp-server", 11) == 0) {
command_type = CMD_SFTP;

/* account for possible .exe extension */
if (command_len >= 15 && _memicmp(command + 11, ".exe", 4) == 0)
command_args = command + 15;
else
command_args = command + 11;
} else if (command_len >= 3 && _memicmp(command, "scp", 3) == 0) {
command_type = CMD_SCP;

/* account for possible .exe extension */
if (command_len >= 7 && _memicmp(command + 3, ".exe", 4) == 0)
command_args = command + 7;
else
command_args = command + 3;
}

if (command_type == CMD_OTHER)
break;

len = 0;
len += progdir_len + 4; /* account for " around */
len += command_len + 4; /* account for possible .exe addition */

if ((cmd_sp = malloc(len)) == NULL) {
errno = ENOMEM;
goto done;
}

p = cmd_sp;

if (shell_type == SH_CMD) {

CMDLINE_APPEND(p, "\"");
CMDLINE_APPEND(p, progdir);

if (command_type == CMD_SCP)
CMDLINE_APPEND(p, "\\scp.exe\"");
else
CMDLINE_APPEND(p, "\\sftp-server.exe\"");

} else {
Copy link

Choose a reason for hiding this comment

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

For shell_type != SH_CMD, how will other shells know where to find the executable? Is it expected to be in the system path? If its depending on the current working directory, might be good to make sure this works with your new chroot() jail since I think that changes the working directory somewhere along the way.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, as commented in earlier part of the code (and as will be documented once this change is in), its expected that scp and sftp-server (openssh installation folder) is in system path. Current working directory does not matter.

@bagajjal and I discussed an alternative to work around this limitation, i.e to runtime add sshd program directory to path before launching scp and sftp sessions. However, this does not completely guarantee that the sshd associated versions of scp and sftp-server are always picked, based on the info at https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx.
Also, this behavior is inline with how things work on Unix, where the sshd installation path is expected to be in system path.

We don't apply chroot while executing child processes.

if (command_type == CMD_SCP)
CMDLINE_APPEND(p, "scp.exe");
else
CMDLINE_APPEND(p, "sftp-server.exe");
}

CMDLINE_APPEND(p, command_args);
*p = '\0';
command = cmd_sp;
} while (0);

len = 0;
if (pty)
len += progdir_len + (int)strlen("ssh-shellhost.exe") + 5;
len +=(int) strlen(shell) + 3;/* 3 for " around shell path and trailing space */
Copy link

@bagajjal bagajjal Jun 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

instead of doing malloc and calculating length everywhere.. it's better to have a static array CMD_LEN?
OR
MAX_PATH - default shell path
MAX_PATH - sub system
MAX_PATH - arguments (just to be on safer side).
MAX_PATH - miscellaneous (like double quotes, /c, etc).

Copy link
Author

Choose a reason for hiding this comment

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

I thought so, but I'll have to keep aside 3k worth memory aside to be generous enough for all scenarios, while in reality, you'd only be using ~200 bytes is most practical cases.

if (command) {
len += 15; /* for shell command argument, typically -c or /c */
len += (int)strlen(command) + 5; /* 5 for possible " around command and null term*/
}

if ((cmdline = malloc(len)) == NULL) {
errno = ENOMEM;
goto done;
}

p = cmdline;
if (pty) {
CMDLINE_APPEND(p, "\"");
CMDLINE_APPEND(p, progdir);
CMDLINE_APPEND(p, "\\ssh-shellhost.exe\" ");
}
CMDLINE_APPEND(p, "\"");
CMDLINE_APPEND(p, shell);
CMDLINE_APPEND(p, "\"");
if (command) {
if (shell_arg) {
CMDLINE_APPEND(p, " ");
CMDLINE_APPEND(p, shell_arg);
CMDLINE_APPEND(p, " ");
}
else if (shell_type == SH_CMD)
CMDLINE_APPEND(p, " /c ");
else
CMDLINE_APPEND(p, " -c ");

/* bash type shells require " decoration around command*/
if (shell_type == SH_WSL_BASH || shell_type == SH_CYGWIN)
CMDLINE_APPEND(p, "\"");

CMDLINE_APPEND(p, command);

if (shell_type == SH_WSL_BASH || shell_type == SH_CYGWIN)
CMDLINE_APPEND(p, "\"");
}
*p = '\0';
ret = cmdline;
cmdline = NULL;
done:
if (cmd_sp)
free(cmd_sp);
if (cmdline)
free(cmdline);

return ret;
}
1 change: 1 addition & 0 deletions contrib/win32/win32compat/misc_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,4 @@ int is_absolute_path(const char *);
int file_in_chroot_jail(HANDLE, const char*);
PSID get_user_sid(char*);
int am_system();
char* build_session_commandline(const char *, const char *, const char *, int );
1 change: 1 addition & 0 deletions contrib/win32/win32compat/pwd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ set_defaultshell()
if ((command_option_local = utf16_to_utf8(option_buf)) == NULL)
goto cleanup;

convertToBackslash(pw_shellpath_local);
pw_shellpath = pw_shellpath_local;
pw_shellpath_local = NULL;
shell_command_option = command_option_local;
Expand Down
132 changes: 132 additions & 0 deletions regress/unittests/win32compat/miscellaneous_tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,137 @@ test_chroot()
//_wsystem(L"RD /S /Q chroot-testdir >NUL 2>&1");
}

void
test_build_session_commandline()
{
char *progdir = w32_programdir(), *out, buf[PATH_MAX*2], shellhost_path[PATH_MAX];
shellhost_path[0] = '\0';
strcat(shellhost_path, "\"");
strcat(shellhost_path, progdir);
strcat(shellhost_path, "\\ssh-shellhost.exe\"");
int shellhost_path_len = (int)strlen(shellhost_path);

TEST_START("default interactive session tests");
out = build_session_commandline("c:\\system32\\cmd.exe", NULL, NULL, 0);
ASSERT_STRING_EQ(out, "\"c:\\system32\\cmd.exe\"");
out = build_session_commandline("c:\\system32\\cmd.exe", NULL, NULL, 1);
ASSERT_STRING_EQ(out + shellhost_path_len + 1, "\"c:\\system32\\cmd.exe\"");
out[shellhost_path_len] = '\0';
ASSERT_STRING_EQ(out, shellhost_path);
TEST_DONE();

TEST_START("cmd shell tests");
buf[0] = '\0';
strcat(buf, "\"c:\\system32\\cmd.exe\" /c \"");
strcat(buf, progdir);
int len_pg = strlen(buf);
out = build_session_commandline("c:\\system32\\cmd.exe", NULL, "internal-sftp -arg", 0);
buf[len_pg] = '\0';
strcat(buf, "\\sftp-server.exe\" -arg");
ASSERT_STRING_EQ(out, buf);
out = build_session_commandline("c:\\system32\\cmd.exe", NULL, "SFTP-server.exe -arg", 0);
buf[len_pg] = '\0';
strcat(buf, "\\sftp-server.exe\" -arg");
ASSERT_STRING_EQ(out, buf);
out = build_session_commandline("c:\\system32\\cmd.exe", NULL, "sftp-SERVER -arg", 0);
buf[len_pg] = '\0';
strcat(buf, "\\sftp-server.exe\" -arg");
ASSERT_STRING_EQ(out, buf);
out = build_session_commandline("c:\\system32\\cmd.exe", NULL, "sCp -arg", 0);
buf[len_pg] = '\0';
strcat(buf, "\\scp.exe\" -arg");
ASSERT_STRING_EQ(out, buf);
out = build_session_commandline("c:\\system32\\cmd.exe", NULL, "mycommand -arg", 1);
ASSERT_STRING_EQ(out + shellhost_path_len + 1, "\"c:\\system32\\cmd.exe\" /c mycommand -arg");
out[shellhost_path_len] = '\0';
ASSERT_STRING_EQ(out, shellhost_path);
free(out);

TEST_DONE();

TEST_START("wsl bash shell tests");
out = build_session_commandline("c:\\system32\\bash.exe", NULL, "internal-sftp -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\system32\\bash.exe\" -c \"sftp-server.exe -arg\"");
free(out);
out = build_session_commandline("c:\\system32\\bash", NULL, "internal-sftp -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\system32\\bash\" -c \"sftp-server.exe -arg\"");
free(out);
out = build_session_commandline("c:\\system32\\bash", NULL, "sFTP-server -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\system32\\bash\" -c \"sftp-server.exe -arg\"");
free(out);
out = build_session_commandline("c:\\system32\\bash", NULL, "scP -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\system32\\bash\" -c \"scp.exe -arg\"");
free(out);
out = build_session_commandline("c:\\system32\\bash", "-custom", "mycommand -arg", 1);
ASSERT_STRING_EQ(out + shellhost_path_len + 1, "\"c:\\system32\\bash\" -custom \"mycommand -arg\"");
out[shellhost_path_len] = '\0';
ASSERT_STRING_EQ(out, shellhost_path);
free(out);
TEST_DONE();

TEST_START("cygwin bash shell tests");
out = build_session_commandline("c:\\cygwin\\bash.exe", NULL, "internal-sftp -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\cygwin\\bash.exe\" -c \"sftp-server.exe -arg\"");
free(out);
out = build_session_commandline("c:\\cygwin\\bash", NULL, "sftp-server -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\cygwin\\bash\" -c \"sftp-server.exe -arg\"");
free(out);
out = build_session_commandline("c:\\cygwin\\bash", NULL, "sftp-seRVer.exe -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\cygwin\\bash\" -c \"sftp-server.exe -arg\"");
free(out);
out = build_session_commandline("c:\\cygwin\\bash", NULL, "sCp -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\cygwin\\bash\" -c \"scp.exe -arg\"");
free(out);
out = build_session_commandline("c:\\cygwin\\bash", "-custom", "mycommand -arg", 1);
ASSERT_STRING_EQ(out + shellhost_path_len + 1, "\"c:\\cygwin\\bash\" -custom \"mycommand -arg\"");
out[shellhost_path_len] = '\0';
ASSERT_STRING_EQ(out, shellhost_path);
free(out);
TEST_DONE();

TEST_START("powershell shell tests");
out = build_session_commandline("c:\\powershell.exe", NULL, "internal-sftp -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\powershell.exe\" -c sftp-server.exe -arg");
free(out);
out = build_session_commandline("c:\\powershell", NULL, "sftp-server -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\powershell\" -c sftp-server.exe -arg");
free(out);
out = build_session_commandline("c:\\powershell.exe", NULL, "sftp-sERver.exe -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\powershell.exe\" -c sftp-server.exe -arg");
free(out);
out = build_session_commandline("c:\\powershell.exe", NULL, "scP -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\powershell.exe\" -c scp.exe -arg");
free(out);
out = build_session_commandline("c:\\powershell.exe", "-custom", "mycommand -arg", 1);
ASSERT_STRING_EQ(out + shellhost_path_len + 1, "\"c:\\powershell.exe\" -custom mycommand -arg");
out[shellhost_path_len] = '\0';
ASSERT_STRING_EQ(out, shellhost_path);
free(out);
TEST_DONE();


TEST_START("other shell tests");
out = build_session_commandline("c:\\myshell.exe", NULL, "internal-sftp -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\myshell.exe\" -c sftp-server.exe -arg");
free(out);
out = build_session_commandline("c:\\myshell", NULL, "sftp-server -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\myshell\" -c sftp-server.exe -arg");
free(out);
out = build_session_commandline("c:\\myshell", NULL, "sftp-seRVer.exe -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\myshell\" -c sftp-server.exe -arg");
free(out);
out = build_session_commandline("c:\\myshell", NULL, "sCp -arg", 0);
ASSERT_STRING_EQ(out, "\"c:\\myshell\" -c scp.exe -arg");
free(out);
out = build_session_commandline("c:\\myshell", "-custom", "mycommand -arg", 1);
ASSERT_STRING_EQ(out + shellhost_path_len + 1, "\"c:\\myshell\" -custom mycommand -arg");
out[shellhost_path_len] = '\0';
ASSERT_STRING_EQ(out, shellhost_path);
free(out);
TEST_DONE();
}


void
miscellaneous_tests()
{
Expand All @@ -321,4 +452,5 @@ miscellaneous_tests()
test_realpath();
test_statvfs();
test_chroot();
test_build_session_commandline();
}
Loading