Skip to content
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.

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

Fix SSH incorrect command line conversion #123

Closed
wants to merge 1 commit into from

Conversation

@fcharlie
Copy link

fcharlie commented Mar 19, 2019

When I run ssh, if the command line of the exec request contains a space, the command line parsing on the server is inconsistent with the expectation.

Example:

Client:

ssh git@localhost echo "There are spaces in the statement" done

We print the command line on the server, and the command line received on the server is actually as follows:

echo There are spaces in the statement done

You can find that SSH handles command line parameter errors.

Old code:

//https://github.com/openssh/openssh-portable/blob/9edbd7821e6837e98e7e95546cede804dac96754/ssh.c#L1061
		/* A command has been specified.  Store it into the buffer. */
		for (i = 0; i < ac; i++) {
			if ((r = sshbuf_putf(command, "%s%s",
			    i ? " " : "", av[i])) != 0)
				fatal("%s: buffer error: %s",
				    __func__, ssh_err(r));
		}

This PR escapes a string containing special characters, spaces, and constructs a string that can be handled correctly by the shell.

Thansk --> Windows: syscall.EscapeArg

@daztucker

This comment has been minimized.

Copy link
Contributor

daztucker commented Mar 19, 2019

Unfortunately ssh has no knowledge of what the shell is at the other end or what its quoting rules are, and the protocol (RFC4254 section 6.5) only specifies an unstructured string for the "exec" channel.

Making assumptions like you are proposing will likely cause problems in some cases. If it matters to you then you need to either do the (double) quoting yourself or use a construction like:

$ ssh localhost /bin/sh <<EOD
echo "There are spaces in the statement" done
EOD

which prevents some of these problems by avoiding one level of quote processing on the client and specifying the exact shell on the server side.

@daztucker daztucker closed this Mar 19, 2019
@fcharlie

This comment has been minimized.

Copy link
Author

fcharlie commented Mar 19, 2019

@daztucker The SSH RFC specification does have a lot of deficiencies, but when openssh starts a child process on the server, it uses -c which means it will follow the command-line escaping rules.

      byte      SSH_MSG_CHANNEL_REQUEST
      uint32    recipient channel
      string    "exec"
      boolean   want reply
      string    command

   This message will request that the server start the execution of the
   given command.  The 'command' string may contain a path.  Normal
   precautions MUST be taken to prevent the execution of unauthorized
   commands.

'echo' is really just an example that doesn't matter.

But we do have some troubles, such as cloning a repository with a space on the server.
Example:

git clone ssh://git@example.com/some%20user/repo.git
# ssh git@example.com git-upload-pack "some user/repo.git"

Unfortunately, SSH is not perfect.

@daztucker

This comment has been minimized.

Copy link
Contributor

daztucker commented Mar 19, 2019

when openssh starts a child process on the server, it uses -c

that's an implementation detail of OpenSSH's sshd. Other servers, especially on other platforms, could do anything.

it will follow the command-line escaping rules
... of whatever the user's login shell happens to be (on a Unix-ish system, anyway). So it might be a Bourne-compatible shell (sh bash ksh) or it might be something else (csh tcsh fish) or it might be some custom shell or non-Unix system with entirely different behaviour.

It would be possible to do it in an "exec"-like vendor extension that takes a list of string args. There's some discussion around this in http://bugzilla.mindrot.org/show_bug.cgi?id=2283 but the cost/benefit tradeoff is not clear.

@fcharlie

This comment has been minimized.

Copy link
Author

fcharlie commented Mar 19, 2019

@daztucker If there is SSH-3.0 in the future, I hope this problem can be solved. thank you for your reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.