Skip to content

restrict SftpStreamProxy target host to host name characters - #793

Open
rootvector2 wants to merge 1 commit into
apache:masterfrom
rootvector2:sftp-stream-proxy-host
Open

rootvector2 wants to merge 1 commit into
apache:masterfrom
rootvector2:sftp-stream-proxy-host

Conversation

@rootvector2

Copy link
Copy Markdown
Contributor

SftpStreamProxy.connect formats the target host into the command it runs on the proxy host, and HostFileNameParser only ends a host name at / ; ? : @ & = + $ ,, so everything else in the URI authority reaches the proxy's shell. With a stream proxy configured, resolving sftp://user@target|id/ runs nc -q 0 target|id 22 there, and backticks, spaces, newlines, redirections, quotes and a leading - pass the same way: OS command injection on the proxy host for an application that resolves a URI whose host it does not choose, the class of CVE-2023-51385 in OpenSSH's ProxyCommand. Found while checking where URI components end up in a command line.

connect now refuses a target host that starts with - or holds anything but letters, digits and - . _ : % [ ], before it opens the proxy session. The check sits next to the String.format call because that is the one place a URI value is handed to a shell, so it holds for every command format. Host names, IPv4 addresses and bracketed IPv6 literals with a zone id pass as before; the new SftpStreamProxyTest records the command an embedded SSH server receives and fails on the current code.

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute? Claude Code found the unchecked host in the proxy command and wrote this patch, its test and this description; the test was run against the code with and without the change.
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself. The default goal ran on every module with rat, japicmp, javadoc, spotbugs, pmd and checkstyle clean and no test failures, but not error free: the local HTTP provider tests cannot parse the URI built from my machine's host name (unknown_5e:ad:3d:f7:97:a6), and they fail the same way without this change.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

SftpStreamProxy.connect formats the host of the sftp URI into the command it runs on the proxy host, so shell syntax in the host name was executed there. Refuse a target host that is not a host name or an IP address literal before the proxy session is opened.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant