Skip to content
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

Mitigate Security vulnerability in the git hook feature #13058

Merged
merged 2 commits into from
Oct 7, 2020

Conversation

Niklas974
Copy link
Contributor

Git hooks are a dangerous feature that allows users with the relevant privilege to perform remote code execution on the host system and elevate their privileges to gitea administrator. Details are provided in the vulnerability report.

This pull request thus:

  • disables the git hook feature by default
  • improves documentation on it
  • Extends the warning tooltip that is shown when giving git hook privileges to

The problem was first reported to the gitea security team in April who released a patch. In our opinion this patch only partly mitigates the vulnerability, though. For details see the report linked above.

Niklas Goerke added 2 commits October 7, 2020 08:58
Git hooks are a dangerous feature, administrators should be warned before giving
the git hook privilege to users.
Git hooks are a dangerous features (see warning text) that should only
be enabled if the administrator was informed about the risk involved.
@Niklas974 Niklas974 changed the title Git hook Mitigate Security vulnerability in the git hook feature Oct 7, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 7, 2020
@lafriks lafriks added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Oct 7, 2020
@lafriks lafriks added this to the 1.13.0 milestone Oct 7, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 7, 2020
@lafriks lafriks merged commit 8fe8ab5 into go-gitea:master Oct 7, 2020
@Niklas974
Copy link
Contributor Author

Thanks for merging this quickly!

I suggest to add a warning for the next release. Disabling the git hook feature by default will affect the settings on existing gitea instances, admins of existing gitea instances should be informed about the issue to be able to change the setting on their instance if needed.

techknowlogick added a commit to techknowlogick/gitea that referenced this pull request Oct 7, 2020
@lunny
Copy link
Member

lunny commented Oct 7, 2020

@Niklas974 All break changes will be displayed on release notes.

techknowlogick added a commit that referenced this pull request Oct 7, 2020
* Disable Git Hooks by default

Related #13058

* pass tests
@Niklas974
Copy link
Contributor Author

@Niklas974 All break changes will be displayed on release notes.

The change in the default config in this PR (DISABLE_GIT_HOOKS = true) should only affect new installations for gitea. Thus in existing installations, git hooks will stay as they were (mostly enabled, I guess). These existing installations are still endangered, thus I think the admins should be warned in the release notes.

Or am I missing something, here?

@lafriks
Copy link
Member

lafriks commented Oct 8, 2020

depends on what you have in your app.ini. If you have no DISABLE_GIT_HOOKS entry in app.ini it will default to new value than.

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@lunny lunny added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Dec 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants