Fix of 1211 and 1082 #349

bingbing8 · 2018-10-12T00:23:13Z

fix of Unable to use scp with -oUserKnownHostsFile that has spaces Win32-OpenSSH#1211
fix of Remote SSH commands require double escaping before hitting the DefaultShell Win32-OpenSSH#1082
scp works when powershell and cmd as default shell
have the escaping logic in posix_spawn only
Add program dir as part of path in process context so scp and sftp in…
only escape double quotes and backslash for common shells

1. fix of 1211 (scp works when powershell and cmd as default shell) 2. fix of 1082, 3. have the escaping logic in posix_spawn only4. Add program dir as part of path in process context so scp and sftp in…

contrib/win32/win32compat/misc.c

manojampalam · 2018-10-19T03:13:10Z

contrib/win32/win32compat/misc.c

+	* 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.
+	*/


looks like this function is not prepending the current binary location to command. are these comments obsolete ?

we added current binary location to PATH

If we prepend the path, some shells, like powershell will require shell specific additional characters (& for powershell) to be able to run scp, sftp. powershell -c "& <full path to scp>"

I will remove the comments

manojampalam · 2018-10-19T03:17:02Z

contrib/win32/win32compat/w32fd.c

@@ -1052,7 +1053,9 @@ spawn_child_internal(char* cmd, char *const argv[], HANDLE in, HANDLE out, HANDL
 		error("failed to duplicate %s", cmd);
 		return ret;
 	}
-
+	if (strstr(path, "system32\\cmd") || strstr(path, "ssh-shellhost"))


What if you do:
bash.exe -c c:\windows\system32\cmd.exe /c "echo hi"

Good catch.

this case is an issue when the path is user define cmd which contains both exe and arguments. in next version, I use the last parameter to define if it is user defined cmd. If so, we don't change anything.

manojampalam · 2018-10-19T03:21:15Z

contrib/win32/win32compat/w32fd.c

-		memcpy(t, tmp, strlen(tmp));
-		t += strlen(tmp);
-	} else {
+		tmp += strlen(exe_extenstion); /* move the pointer to the end of ".exe" */		


can you end up with an input command like the following here:

c:\windows\system32\cmd /c ping.exe localhost

If so, this logic would incorrectly identify the nested binary as the parent executable

For the existing known shells and ssh executable, the first parts always ends with .exe. I don't see other ways to identify the file path (the first part) in the string.

What about with proxyCommand or SSH_ASK_PASS ? Also fix spelling mistake in exe_extenstion.

Yes, you are right.

manojampalam · 2018-10-19T03:36:30Z

contrib/win32/win32compat/wmain_sshd.c

+			error("failed to allocation memory");
+			return -1;
+		}
+		swprintf_s(path_new_value, path_new_len, L"%s%s%s", __wprogdir, path_value ? L";" : L"",  path_value);


What's the confirmed order of exe lookup (we figured that what's on msdn is wrong) ? Does this guarantee picking up exe from progdir in all scenarios?
Are we OK with adjusting PATH for user's remote sessions?

the msdn is correct, but the search order is for the executable, which is the first part of the command. but scp, sftp are the arguments part of the command.
So far, looks like it picked up binaries from progdir in all the scenarios.
I don't see a problem with adjusting PATH for user's remote session.

2. don't touch user defined cmd: no prepending, no escaping, no additional quotes 3. fix line ending

bingbing8 · 2018-10-23T17:24:08Z

@manojampalam , when you get time, please review the changes. Thanks!

…ortable into cmdescaping

bingbing8 · 2018-10-26T05:29:23Z

@manojampalam Please review. currently only escape non-user defined commands from common shells. for other shells, no escaping, no -c added if shell option is not specified

manojampalam

comments inline...

manojampalam · 2018-10-30T16:45:03Z

contrib/win32/openssh/ssh.vcxproj

@@ -496,6 +496,7 @@
    <ClInclude Include="$(OpenSSH-Src-Path)uuencode.h" />
    <ClInclude Include="$(OpenSSH-Src-Path)version.h" />
    <ClInclude Include="$(OpenSSH-Src-Path)xmalloc.h" />
+    <ClInclude Include="$(OpenSSH-Src-Path)pal-doexec.c.h" />


pal-doexec.h

manojampalam · 2018-10-30T16:45:33Z

contrib/win32/openssh/sshd.vcxproj

@@ -1,473 +1,474 @@
-<?xml version="1.0" encoding="utf-8"?>


still shows up different

finally fixed it.

manojampalam · 2018-10-30T16:47:21Z

contrib/win32/win32compat/misc_internal.h

@@ -70,9 +70,9 @@ int is_absolute_path(const char *);
 int file_in_chroot_jail(HANDLE);
 PSID get_sid(const char*);
 int am_system();
-char* build_session_commandline(const char *, const char *, const char *);
+char * build_command_string(const char * );


you could get rid of this since this is internal to logic within w32_doexec.c

Added back and keep them in misc_internal.h. we need to call them from w32_doexec.c and unit tests

manojampalam · 2018-10-30T16:47:50Z

contrib/win32/win32compat/misc_internal.h

 int is_conpty_supported();
-int exec_command_with_pty(wchar_t*, STARTUPINFOW*, PROCESS_INFORMATION*, int);
+int exec_command_with_pty(int * pid, char* cmd, int in, int out, int err, unsigned int col, unsigned int row, int ttyfd);


This one too, you could get rid of this since this is internal to logic within w32_doexec.c

I revert it back to misc.internal.h because the definition is in w32_pty.c and the caller is w32_doexe.c

manojampalam · 2018-10-30T16:53:16Z

contrib/win32/win32compat/w32fd.c

-		t += strlen(path);
-
+
+	/*For user defined cmd, no need to add quotes*/


?? can you add some examples? What's a user defined command ?

manojampalam · 2018-10-30T16:54:49Z

contrib/win32/win32compat/w32fd.c

+			t += strlen(__progdir);
+			*t++ = '\\';
+		}
+		/*


I'm not in favor of this. What issues are we hitting if we don't do this? If the module path has spaces in it, it is expected to be enclosed in quotes. This is a standard practice even on Unix.

remove the log below this.

manojampalam · 2018-10-30T17:22:16Z

contrib/win32/win32compat/w32fd.c

-		while (*t1)
-			cmdline_len += (DWORD)strlen(*t1++) + 1 + 2;
+		while (*t1) {
+			if (!escape)


Recommend pulling all the commandline building logic into a separate utility helper and writing an exhaustive set of unit tests.

separate the utility helper out.
added unit tests.

manojampalam · 2018-10-30T17:24:23Z

contrib/win32/win32compat/win32_pty.c

+
+	memset(&si, 0, sizeof(STARTUPINFO));
+	si.cb = sizeof(STARTUPINFO);
+	si.dwXSize = 5;


Why 5 ? Add comments if needed

remove these two lines. It was from the existing codes.

manojampalam · 2018-10-30T17:27:43Z

contrib/win32/win32compat/wmain_sshd.c

 	argc_original = argc;
 	wargv_original = wargv;

 	init_prog_paths();
 	/* change current directory to sshd.exe root */
 	_wchdir(__wprogdir);

+	_wdupenv_s(&path_value, &len, L"PATH");


Please add comments on why we are doing this

2. Add registry key to indicate escaping is needed or not for shells other than ps, shellhost, cmd, cygwin, bash. Default is escape the arg string

…s createprocess failed. Remove the added quotes as failoever when arg is empty.

manojampalam · 2018-11-02T23:14:47Z

contrib/win32/openssh/sshd.vcxproj

@@ -1,473 +1,474 @@
-<?xml version="1.0" encoding="utf-8"?>


still shows up different

manojampalam · 2018-11-02T23:22:30Z

contrib/win32/win32compat/w32fd.c

-	*t = '\0';
-
+	int ret = -1;
+	cmdline = build_commandline_string(cmd, argv, prepend_module_path);


what if build_commandline_string fails ?

I will add check

manojampalam · 2018-11-02T23:22:49Z

contrib/win32/win32compat/w32fd.c

@@ -1034,115 +1034,25 @@ int fork()
 	verbose("fork is not supported"); 
 	return -1;
 }
+char * build_commandline_string(const char* cmd, char *const argv[], BOOLEAN prepend_module_path);


can this move to misc_internal.h

manojampalam · 2018-11-02T23:33:08Z

contrib/win32/win32compat/w32fd.c

+		else
+			b = CreateProcessW(NULL, t, NULL, NULL, TRUE, flags, NULL, NULL, &si, &pi);
+		*(cmdline_utf16 + wcslen(cmdline_utf16) - 1) = L'\0';
+		/*failover only when double quotes is potentially added necessary*/


you could simplify the below condition by putting this inside loop and getting rid of "num" and "t":

if ( !b && GetLastError() != ERROR_FILENOTFOUND && (argv == NULL || *argv == NULL) && cmd[0] != '"') {
cmdline_utf16++;
*(cmdline_utf16 + wcslen(cmdline_utf16) - 1) = L'\0';
}

break;

I can get rid of num, but not t.
I need to free cmdline_utf16 at the end. the pointer can't change

manojampalam · 2018-11-02T23:36:26Z

contrib/win32/win32compat/win32_pty.c

 	debug3("pty commandline: %ls", pty_cmdline);

-	if (!CreateProcessW(NULL, pty_cmdline, NULL, NULL, TRUE, 0, NULL, NULL, si, pi)) {
+	if (CreateProcessW(NULL, pty_cmdline, NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi)) {


Sorry, I may have missed this earlier but why can't we call posix_spawnp here ?

for pty, the si setting from session (col, row, and dwflag) are different from posix_spawnp. There is no args for col, row, and dwflag to posix_spawnp. One option is change the signature of posix_spawn_internal to take si info and pass them to spawn_child_internal. and then call posix_spawn_internal at here.

manojampalam · 2018-11-02T23:39:39Z

regress/unittests/win32compat/miscellaneous_tests.c

-	out = build_session_commandline("c:\\powershell", NULL, "sftp-server -arg");
-	ASSERT_STRING_EQ(out, "\"c:\\powershell\" -c \"sftp-server.exe -arg\"");
+char *
+build_commandline_string(const char* cmd, char *const argv[], BOOLEAN prepend_module_path);


you can get rid of this by moving it to misc_internal

bingbing8 · 2018-11-03T22:23:01Z

@manojampalam, please review the updated code

Fix of 1211 and 1082

8a9bdd1

1. fix of 1211 (scp works when powershell and cmd as default shell) 2. fix of 1082, 3. have the escaping logic in posix_spawn only4. Add program dir as part of path in process context so scp and sftp in…

bingbing8 closed this Oct 12, 2018

add quotes for the cmd only when module path is added.

a6d23b7

bingbing8 reopened this Oct 15, 2018

bingbing8 closed this Oct 15, 2018

bingbing8 reopened this Oct 15, 2018

bingbing8 added 4 commits October 15, 2018 17:49

fix the cmd when it already surrounded by quotes

8fdf9cc

don't escape command for cmd.exe and shellhost.exe

e49c002

minor updates

09ccbbd

CreateProcess does not support app surrounded by single quotes.

6c5b07d

bingbing8 mentioned this pull request Oct 16, 2018

Mysterious quotes appearing when running non interactive command PowerShell/Win32-OpenSSH#1273

Closed

bingbing8 added 2 commits October 17, 2018 14:58

added tests for double quotes escaping

0bfc520

update tests

164540d

bingbing8 changed the title ~~(not ready for review yet) Fix of 1211 and 1082~~ Fix of 1211 and 1082 Oct 17, 2018

bingbing8 added 2 commits October 18, 2018 11:09

remove unused var

d0f29c7

minor changes

12a10da

bingbing8 closed this Oct 19, 2018

bingbing8 reopened this Oct 19, 2018

manojampalam suggested changes Oct 19, 2018

View reviewed changes

bingbing8 added 3 commits October 19, 2018 15:15

PAL of do_exec_* APIs

aa8359e

1. change file names

4ca8dd8

2. don't touch user defined cmd: no prepending, no escaping, no additional quotes 3. fix line ending

fix file name

a387bb5

bingbing8 added 5 commits October 25, 2018 15:00

Merge branch 'latestw_all' of https://github.com/powershell/openssh-p…

132eb61

…ortable into cmdescaping

only escape double quotes and backslash for powershell, bash, and cygwin

c0b39e9

no -c or /c for shells other than cmd, ps, bash, and cygwin

aade195

add -c for ssh-shellhost

5c39e8c

minor update

b52fdb1

update comments

a0b05e8

bingbing8 added 5 commits October 25, 2018 22:54

update comments

946af56

test updates

5ccb3aa

print out the file for test investigation

ce136df

Caller of posix_spawn_internal determine if escaping needed.

79e19f3

define shell types

d4677c2

manojampalam suggested changes Oct 30, 2018

View reviewed changes

bingbing8 added 4 commits October 30, 2018 19:19

1. separate cmdline building log from posix_spawn

e3cabb7

2. Add registry key to indicate escaping is needed or not for shells other than ps, shellhost, cmd, cygwin, bash. Default is escape the arg string

add unit tests for build_exec_command and build_commandline_string

0a67c4c

merge conflicts

2300ac5

revert the comment of one test

03aea16

bingbing8 closed this Oct 31, 2018

bingbing8 reopened this Oct 31, 2018

bingbing8 added 2 commits November 2, 2018 14:27

Always add quotes if they are not already there for executables unles…

fb33b53

…s createprocess failed. Remove the added quotes as failoever when arg is empty.

check failover condition in posix_spawn

8a167e8

manojampalam suggested changes Nov 2, 2018

View reviewed changes

bingbing8 added 5 commits November 2, 2018 23:30

Fix the case when the path has quotes and and add_module_path

5da3925

change line ending

c4bb578

changed encoding

641421b

update line ending and encoding

127f2fe

revert the new line in sshd.vcproj.filters

0c91470

bingbing8 mentioned this pull request Nov 3, 2018

Remote SSH commands require double escaping before hitting the DefaultShell PowerShell/Win32-OpenSSH#1082

Closed

minor update for format

aa0e165

manojampalam approved these changes Nov 5, 2018

View reviewed changes

manojampalam merged commit a75116b into PowerShell:latestw_all Nov 5, 2018

		t += strlen(path);


		/For user defined cmd, no need to add quotes/

Fix of 1211 and 1082 #349

Fix of 1211 and 1082 #349

Conversation

bingbing8 commented Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

bingbing8 Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingbing8 Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manojampalam Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingbing8 Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

bingbing8 commented Oct 23, 2018

bingbing8 commented Oct 26, 2018 • edited Loading

manojampalam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingbing8 Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingbing8 Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingbing8 Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingbing8 Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingbing8 commented Nov 3, 2018

bingbing8 commented Oct 12, 2018 •

edited

Loading

bingbing8 Oct 19, 2018 •

edited

Loading

bingbing8 Oct 22, 2018 •

edited

Loading

manojampalam Oct 19, 2018 •

edited

Loading

bingbing8 Oct 19, 2018 •

edited

Loading

bingbing8 commented Oct 26, 2018 •

edited

Loading

bingbing8 Nov 3, 2018 •

edited

Loading

bingbing8 Nov 3, 2018 •

edited

Loading

bingbing8 Oct 31, 2018 •

edited

Loading

bingbing8 Nov 3, 2018 •

edited

Loading