search

ericpaulbishop / redmine_git_hosting

A ChiliProject/Redmine plugin which makes configuring your own git hosting easy.
186 stars 37 forks source link

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

Closed maddingo closed 12 years ago

maddingo commented 12 years ago

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

ericpaulbishop commented 12 years ago

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 (https://github.com/chiliproject/chiliproject/pull/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:

 https://github.com/ericpaulbishop/redmine_git_hosting/pull/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: https://github.com/ericpaulbishop/redmine_git_hosting/pull/158

maddingo commented 12 years ago

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 (https://github.com/chiliproject/chiliproject/pull/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:

https://github.com/ericpaulbishop/redmine_git_hosting/pull/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: https://github.com/ericpaulbishop/redmine_git_hosting/pull/158

Reply to this email directly or view it on GitHub: https://github.com/ericpaulbishop/redmine_git_hosting/pull/158#issuecomment-5406739

maddingo commented 12 years ago

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 (https://github.com/chiliproject/chiliproject/pull/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:

https://github.com/ericpaulbishop/redmine_git_hosting/pull/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: https://github.com/ericpaulbishop/redmine_git_hosting/pull/158

Reply to this email directly or view it on GitHub: https://github.com/ericpaulbishop/redmine_git_hosting/pull/158#issuecomment-5406739

ciaranj commented 12 years ago

@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

maddingo commented 12 years ago

@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?