session process path changes #319
Conversation
| else | ||
| CMDLINE_APPEND(p, "\\sftp-server.exe\""); | ||
|
|
||
| } else { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
contrib/win32/win32compat/misc.c
Outdated
| /* 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; |
| } while(0) | ||
|
|
||
| /* get shell type */ | ||
| if (strstr(shell, "system32\\cmd")) |
There was a problem hiding this comment.
It will help us in long run if we move this to a function that returns the enum.
There was a problem hiding this comment.
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")) |
There was a problem hiding this comment.
Can you check if this holds good on IOT when privsep is ON for all the SKUs.
contrib/win32/win32compat/misc.c
Outdated
| @@ -1725,3 +1725,177 @@ 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) | |||
There was a problem hiding this comment.
return type (char *) should alone be on a separate line as per BSD standards.
| break; | ||
| command_len = (int)strlen(command); | ||
|
|
||
| if (command_len >= 13 && _memicmp(command, "internal-sftp", 13) == 0) { |
There was a problem hiding this comment.
instead of hardcoding 13.. have strlen(internal-sftp) in a variable and use..
same on line 1794, 1798, 1802, 1806
There was a problem hiding this comment.
agree I'm being stingy due to strlen perf. Since these are established constants, I'll leave them as they are.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure. Thanks for the tip. I like how its readable with no perf impact. I'll fix it in a subsequent PR.
| 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 */ |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Added utility to build session process command line - this accounts for restrictions from various shells. With these changes, scp and sftp-server are expected to be machine wide PATH if a custom shell (other than cmd.exe) is defined. Added comprehensive test cases.
Also, fixed issue with USERNAME env variable containing domain prefix too.
PowerShell/Win32-OpenSSH#1165
PowerShell/Win32-OpenSSH#1165
PowerShell/Win32-OpenSSH#1171