10000 Reject potential option injections over ssh by carlosmn · Pull Request #6636 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content
/ libgit2 Public

Reject potential option injections over ssh #6636

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 17, 2023
Merged

Reject potential option injections over ssh #6636

merged 5 commits into from
Oct 17, 2023

Conversation

carlosmn
Copy link
Member

Like git (and basically everyone else, that was a fun CVE) did a while ago, we need to reject urls that might inject options into ssh which could cause it to execute arbitrary commands instead of what we mean for it to execute.

This is on the yet-unreleased exec-based transport so a public PR should be fine.

As git does, we also reject suspicious paths which may be trying to inject options into the command we ask the remote to execute. This is out of an abundance of caution.

I'm opening this up as a draft as we should have at least a little bit of testing there.

I put the function in with the net ones because I couldn't think of where to put it. git puts it with some path functions but our organisation is different.

@carlosmn
net: copy the option-injection detection from git
863ff79 
This function returns true if the string starts with a `-` as that could be used
to inject options into commands we execute.
@carlosmn
ssh_exec: detect a potentially malicious ssh url string
cf7477a 
If you pass along something like `-oProxyCommand=...` as the hostname, we would
pass that along to ssh unbeknownst to us and potentially also the user, if
they were asking a tool to recursively clone submodules.

This is the same fix as mainline git although they don't separate the username
and host for ssh so ours looks like it's checking more.
@carlosmn
ssh: reject suspicious path
517bd0d 
Like in the previous commit and in git, we reject a path that looks like an
option to avoid injection into the command we ask the remote to execute.
@ethomson ethomson marked this pull request as ready for review September 19, 2023 23:00
@ethomson
Copy link
Member

Oops I fat fingered the mobile app and marked this for ready for review. 😅

@carlosmn carlosmn force-pushed the cmn/ssh-eoo branch 2 times, most recently from 017062e to b745268 Compare September 25, 2023 10:27
@carlosmn
Copy link
Member Author

OK I think we're good to go now.

@ethomson
Copy link
Member

Thanks @carlosmn -- sorry for the delay here. I was thinking that process might make more sense? I just went ahead and pushed up a change since it's been so long. 😓

@carlosmn
Copy link
Member Author

Yes, that namespace make sense, I was just trying to fit it into the existing ones.

@ethomson ethomson merged commit a6205fa into main Oct 17, 2023
@ethomson ethomson deleted the cmn/ssh-eoo branch October 17, 2023 07:23
@ethomson
Copy link
Member

Thanks Carlos!

@ethomson ethomson added the bug label Dec 14, 2023
@branchvincent branchvincent mentioned this pull request Mar 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@carlosmn @ethomson
0