Skip to content

Fix parsing of received environment variables from SSH clients #1935

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

Closed
wants to merge 1 commit into from

Conversation

cybe
Copy link
Contributor

@cybe cybe commented Jun 11, 2017

The inbuilt SSH server supports setting environment variable received from SSH clients. However, parsing of those variables was completely broken and ignored the actual SSH protocol wire format, thus was only successful by chance.

This new implementation uses standard operations from the crypto/ssh module to parse the SSH wire format.

The inbuilt SSH server supports setting environment variable received
from SSH clients. However, parsing of those variables was completely
broken and success depended on chance.

This new implementation uses standard operations from the crypto/ssh
module to parse the SSH wire format.

Signed-off-by: Dennis Keitzel <[email protected]>
@lunny lunny added this to the 1.3.0 milestone Jun 12, 2017
@lunny lunny added the type/bug label Jun 12, 2017
@lunny
Copy link
Member

lunny commented Aug 28, 2017

@cybe maybe you could add some tests for this PR? it's difficult to review.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 28, 2017
@cybe
Copy link
Contributor Author

cybe commented Aug 28, 2017

@lunny: I'll look into it.

@lunny lunny modified the milestones: 1.3.0, 1.x.x Oct 11, 2017
@hlandau
Copy link

hlandau commented Jun 17, 2018

@cybe @lunny

How did this code even work in the first place? I don't see how executing env FOO=BAR changes the environment of any subsequent exec, like the subsequent execution of gitea serv. env runs a new program with a modified environment, or, if no command is specified, just prints the resulting environment (but with the current code, anything output by env is ignored.) env has no power to modify the environment of its invoker.

In order to make this work correctly, you'd need to unmarshal the environment variables properly (as in this PR) and add them to a map, then pass them along when gitea serv is executed. But if things have apparently been working fine up until now without environment variables being passed, it may just be better to remove this functionality outright, as it only increases attack service in a delicate area (program invocation).

@techknowlogick
Copy link
Member

can you add copyright for Gitea to header

hlandau added a commit to hlandau/gitea that referenced this pull request Jun 17, 2018
This removes the environment variable parsing code from the SSH server,
which never worked in the first place. Since environment variable
passing doesn't appear to be necessary for the built-in SSH server to
work properly, it's removed to reduce attack surface rather than fixing
it.

The current code processes (untrusted) input in a buggy manner and
passes it to a process invocation which doesn't actually do anything. I
don't *think* this is an exploitable vulnerability but I haven't looked
at it in detail, and it wouldn't really surprise me if it was.

Closes go-gitea#1935, an alternative proposal which which partially fixes the
environment variable handling but ultimately still leaves it broken.

Signed-off-by: Hugo Landau <[email protected]>
@cybe
Copy link
Contributor Author

cybe commented Jun 18, 2018

@hlandau
I'm totally fine with removing the variable parsing code completely. Do you know if the git ssh wire format depends on those variables in any way?

@hlandau
Copy link

hlandau commented Jun 18, 2018

@cybe I've looked at the RFCs and there shouldn't be a problem. This code currently isn't doing anything and there's a no-op default: case in the switch, so there's basically no change.

I've made a PR here: #4266

@techknowlogick
Copy link
Member

Closing this in favour of #4266

@lunny lunny removed this from the 1.x.x milestone Nov 1, 2018
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants