Remove environment variable parsing from SSH server #4266
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #4266 +/- ##
==========================================
+ Coverage 38.83% 38.84% +<.01%
==========================================
Files 354 354
Lines 50174 50163 -11
==========================================
Hits 19485 19485
+ Misses 27866 27855 -11
Partials 2823 2823
Continue to review full report at Codecov.
|
bkcsoft
added
the
lgtm/need 2
techknowlogick
requested changes
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]>
|
@techknowlogick Done. |
lafriks
added
the
type/proposal
|
See Git Wire Protocol, git will send SetEnv to allow wire protocol. https://github.com/git/git/blob/master/Documentation/technical/protocol-v2.txt |
|
This can be closed now, as it was implemented and merged by #6825. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #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]