Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

The change fixes a serious vulnerability in the git repository code #158

Merged
merged 1 commit into from Apr 29, 2012

Conversation

Projects
None yet
3 participants

See similar change in chiliproject code (https://github.com/maddingo/chiliproject/commit/de93e796ca25672626d79077b83edcba0cca2904). Submitted pull request to chili project for core code (chiliproject/chiliproject#189)

ericpaulbishop added a commit that referenced this pull request Apr 29, 2012

Merge pull request #158 from maddingo/master
The change fixes a serious vulnerability in the git repository code

@ericpaulbishop ericpaulbishop merged commit c70a17b into ericpaulbishop:master Apr 29, 2012

Owner

ericpaulbishop commented Apr 29, 2012

I've merged the commit, since I can't see that this addition would do
any harm... but I'm having trouble understanding the vulnerability.
You never explain HOW this allows remote execution, or how the lack of
the code can be exploited to create a remote execution vulnerability.
Can you please elaborate? Without proof I'm inclined to revert the
commit.

Eric

On Fri, Apr 27, 2012 at 9:44 AM, maddingo
reply@reply.github.com
wrote:

See similar change in chiliproject code (https://github.com/maddingo/chiliproject/commit/de93e796ca25672626d79077b83edcba0cca2904). Submitted pull request to chili project for core code (chiliproject/chiliproject#189)

You can merge this Pull Request by running:

 git pull https://github.com/maddingo/redmine_git_hosting master

Or you can view, comment on it, or merge it online at:

 #158

-- Commit Summary --

-- File Changes --

M lib/git_hosting/patches/git_adapter_patch.rb (3)

-- Patch Links --

 https://github.com/ericpaulbishop/redmine_git_hosting/pull/158.patch
 https://github.com/ericpaulbishop/redmine_git_hosting/pull/158.diff


Reply to this email directly or view it on GitHub:
#158

Hi Eric,
I put a comment in the pull request mentioning the URL that exposes the error.
I patched our server, so there is no problem running the following URL on it.
Here is the URL that exposed the vulnerability on our server:
https://forge.uis.no/projects/abam-ws/repository/changes?git_url_text=&branch=master&rev=master||echo%20AAA%20%3E%3E/tmp/aaa.txt

What happens is that the string AAA is appended to the file /tmp/aaa.txt.
The githosting application runs as the user git (in our case). An external company did a security screening for us. They discovered this vulnerability.
With this security hole they added their ssh key to the authorized_keys file and managed to log on to our server without a password.

Martin

On 29.Apr 2012, at 7:23PM, Eric Bishop wrote:

I've merged the commit, since I can't see that this addition would do
any harm... but I'm having trouble understanding the vulnerability.
You never explain HOW this allows remote execution, or how the lack of
the code can be exploited to create a remote execution vulnerability.
Can you please elaborate? Without proof I'm inclined to revert the
commit.

Eric

On Fri, Apr 27, 2012 at 9:44 AM, maddingo
reply@reply.github.com
wrote:

See similar change in chiliproject code (https://github.com/maddingo/chiliproject/commit/de93e796ca25672626d79077b83edcba0cca2904). Submitted pull request to chili project for core code (chiliproject/chiliproject#189)

You can merge this Pull Request by running:

git pull https://github.com/maddingo/redmine_git_hosting master

Or you can view, comment on it, or merge it online at:

#158

-- Commit Summary --

-- File Changes --

M lib/git_hosting/patches/git_adapter_patch.rb (3)

-- Patch Links --

https://github.com/ericpaulbishop/redmine_git_hosting/pull/158.patch
https://github.com/ericpaulbishop/redmine_git_hosting/pull/158.diff


Reply to this email directly or view it on GitHub:
#158


Reply to this email directly or view it on GitHub:
#158 (comment)

maddingo commented May 2, 2012

I improved the code in my repository and made it more robust. A couple of lines further down is the code that is supposed to deal with the problem (the line with shell_quote), but it doesn't work. It should probably be fixed there instead of where I fixed it. I will try to come up with a better solution, first I have to learn more Ruby.

Martin

On 29.04.2012, at 19:23, Eric Bishopreply@reply.github.com wrote:

I've merged the commit, since I can't see that this addition would do
any harm... but I'm having trouble understanding the vulnerability.
You never explain HOW this allows remote execution, or how the lack of
the code can be exploited to create a remote execution vulnerability.
Can you please elaborate? Without proof I'm inclined to revert the
commit.

Eric

On Fri, Apr 27, 2012 at 9:44 AM, maddingo
reply@reply.github.com
wrote:

See similar change in chiliproject code (https://github.com/maddingo/chiliproject/commit/de93e796ca25672626d79077b83edcba0cca2904). Submitted pull request to chili project for core code (chiliproject/chiliproject#189)

You can merge this Pull Request by running:

git pull https://github.com/maddingo/redmine_git_hosting master

Or you can view, comment on it, or merge it online at:

#158

-- Commit Summary --

-- File Changes --

M lib/git_hosting/patches/git_adapter_patch.rb (3)

-- Patch Links --

https://github.com/ericpaulbishop/redmine_git_hosting/pull/158.patch
https://github.com/ericpaulbishop/redmine_git_hosting/pull/158.diff


Reply to this email directly or view it on GitHub:
#158


Reply to this email directly or view it on GitHub:
#158 (comment)

ciaranj commented Jun 28, 2012

@maddingo / @ericpaulbishop I'm just reviewing my fork of this project (aiming to remove my customisations), the regex that this commit brings in appears to be invalid/iffy ? :

a.gsub!(/^\.\-\w_\:]/, '')

Is that missing a starting square brace, or should the square brace be escaped??

Actually, checking: https://github.com/maddingo/chiliproject/commit/de93e796ca25672626d79077b83edcba0cca2904 I can see it should have a starting square brace to make that a set of character classes

@ciaranj Hm, I reviewed my own code and found that I have somethin else there, see my repository at https://github.com/maddingo/redmine_git_hosting/blob/master/lib/git_hosting/patches/git_adapter_patch.rb#L41
Maybe I should send another pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment