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

Deprecate and remove shared secret from webhook payloads #11755

Closed
JustAnotherArchivist opened this issue Jun 4, 2020 · 22 comments · Fixed by #16176
Closed

Deprecate and remove shared secret from webhook payloads #11755

JustAnotherArchivist opened this issue Jun 4, 2020 · 22 comments · Fixed by #16176
Labels
kind/security reviewed/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented
Milestone

Comments

@JustAnotherArchivist
Copy link
Contributor

Gitea currently sends the secret in the webhook payload in plaintext. This renders signature validation entirely useless since an attacker could freely manipulate the payload without detection.

Signatures were originally requested in #3901 and added in #6428. This problem was also mentioned in a comment in the former, but no separate issue for this security problem has been filed. Since then, it has been mentioned in a few comments that the secret is deprecated and will be removed (e.g. #7487 (comment), #5173 (comment)). However, a deprecation notice in the changelog and the documentation is missing, so only few people will notice.

An official deprecation warning should be added to the changelog as soon as possible, ideally with the release of 1.12.0. This should also include information on when the field will be removed entirely.

@JustAnotherArchivist
Copy link
Contributor Author

A note on the webhook documentation: that's already quite outdated (missing fields, headers, etc.) and very incomplete. I'm working on a full rewrite (cf. #9485), but I currently don't know when that will be finished. Until then, I'd suggest simply adding a deprecation warning above the push example with a reference to the X-Gitea-Signature header. I will be adding a full description in my rewrite, but until then, the PHP example already illustrates how to validate that.

@stale
Copy link

stale bot commented Aug 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. I am here to help clear issues left open even if solved or waiting for more insight. This issue will be closed if no further activity occurs during the next 2 weeks. If the issue is still valid just add a comment to keep it alive. Thank you for your contributions.

@stale stale bot added the stale label Aug 9, 2020
@JustAnotherArchivist
Copy link
Contributor Author

Yes, this is still valid.

@stale stale bot removed the stale label Aug 9, 2020
@stale
Copy link

stale bot commented Oct 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. I am here to help clear issues left open even if solved or waiting for more insight. This issue will be closed if no further activity occurs during the next 2 weeks. If the issue is still valid just add a comment to keep it alive. Thank you for your contributions.

@stale stale bot added the stale label Oct 10, 2020
@zeripath zeripath added kind/security reviewed/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented labels Oct 10, 2020
@zeripath
Copy link
Contributor

AFAICS this is still present but it needs a security tag.

I don't understand completely why this was ever put there, and therefore I don't know why it hasn't been removed - but clearly it needs to be dealt with.

Now this is too late for 1.13 but I think we can just deprecate the secret from 1.13 so that it will be gone from 1.14 and anyone requiring it just has to do something about it.

@zeripath zeripath added this to the 1.14 milestone Oct 10, 2020
@stale stale bot removed the stale label Oct 10, 2020
@rakshith-ravi
Copy link
Contributor

@zeripath shouldn't this be moved to 1.14.0 milestone instead of 1.14? (Why do we even have 1.14 in the first place?) Just making sure there's no duplicate milestones created

@techknowlogick techknowlogick modified the milestones: 1.14, 1.14.0 Oct 14, 2020
@JustAnotherArchivist
Copy link
Contributor Author

Shouldn't this be on the 1.13.0 milestone until the deprecation warnings are in? Just trying to make sure this isn't delayed by another version due to this.

@techknowlogick
Copy link
Member

@JustAnotherArchivist we will mention in 1.13.0 blog post

@JustAnotherArchivist
Copy link
Contributor Author

I see. I feel like a warning should also be added to the webhook docs and config page.

@techknowlogick
Copy link
Member

That's reasonable. Would you be open to making a PR :)

@JustAnotherArchivist
Copy link
Contributor Author

Sure. I'll need some guidance on how to integrate it into the web UI though.

Also, I noticed that the secret is also included on Gogs webhooks. It appears like Gogs itself didn't have it in the payload since early 2017 (gogs/gogs#3692), but that should be verified first to avoid breaking existing hooks.

@JustAnotherArchivist
Copy link
Contributor Author

JustAnotherArchivist commented Oct 27, 2020

Checklist:

@zeripath
Copy link
Contributor

Add deprecation warning to the webhook configuration page in v1.13.0 – please advise how such a warning should be integrated into the web UI; I assume there are already elements defined for warnings/errors but currently don't have time to dig into the source code and find this myself.

I don't think there's a sensible place for that - the webhook page simply links to gitea.io. I would guess that means we don't need to do anything about that.

@JustAnotherArchivist
Copy link
Contributor Author

Thinking about it again, yeah, I guess you're right. The secret itself isn't being deprecated, only its inclusion in the payload, and the web interface makes no attempt to describe what the payload is (except that you can look at the payloads on delivered hooks).

@CirnoT
Copy link
Contributor

CirnoT commented Oct 28, 2020

Just FYI this will require appropriate PR for go-scm for Drone

https://github.com/drone/go-scm/blob/427b8a85897c892148801824760bc66d3a3cdcdb/scm/driver/gitea/webhook.go#L62-L72

@techknowlogick
Copy link
Member

@CirnoT
Copy link
Contributor

CirnoT commented Oct 28, 2020

Ah sorry, yes. That's && there not ||.

Also it uses FormValue so it looks for ?secret= instead of actually examining Gitea's webhook body, meh... I suppose an ugly workaround for very old Gitea versions?

  • The comment on line 70 is misleading actually since it mentions "and secret is in payload" but the actual implementation fetches the secret from query parameter, not payload.

@techknowlogick
Copy link
Member

@CirnoT no worries, I added in support for the signature checking via header so I was worried I missed something haha.

FormValue is from before my PR so I don't think that needs changing from querystring to webhook body, as that means it has been working that way since the start

@JustAnotherArchivist
Copy link
Contributor Author

I believe secret was never in the query parameters at all. It was originally implemented in Gogs in commit e573855 and was in the payload back then already.

@CirnoT
Copy link
Contributor

CirnoT commented Oct 29, 2020

I believe secret was never in the query parameters at all. It was originally implemented in Gogs in commit e573855 and was in the payload back then already.

It's part of URI that Drone installs as webhook; like I said, most likely workaround from when there was no support for signature in Gitea

@JustAnotherArchivist
Copy link
Contributor Author

JustAnotherArchivist commented Dec 20, 2020

@techknowlogick It appears this was missed on the release notes and blog post unless I'm blind?

@techknowlogick
Copy link
Member

@JustAnotherArchivist Thanks for reminder. I've just pushed up an update to the release blog post.

@lunny lunny modified the milestones: 1.14.0, 1.15.0 Jan 27, 2021
zeripath added a commit that referenced this issue Jun 27, 2021
This PR removes multiple unneeded fields from the `HookTask` struct and adds the two headers `X-Hub-Signature` and `X-Hub-Signature-256`.

## ⚠️ BREAKING ⚠️ 

* The `Secret` field is no longer passed as part of the payload.
* "Breaking" change (or fix?): The webhook history shows the real called url and not the url registered in the webhook (`deliver.go`@129).

Close #16115
Fixes #7788
Fixes #11755

Co-authored-by: zeripath <art27@cantab.net>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this issue Aug 10, 2021
This PR removes multiple unneeded fields from the `HookTask` struct and adds the two headers `X-Hub-Signature` and `X-Hub-Signature-256`.

## ⚠️ BREAKING ⚠️ 

* The `Secret` field is no longer passed as part of the payload.
* "Breaking" change (or fix?): The webhook history shows the real called url and not the url registered in the webhook (`deliver.go`@129).

Close go-gitea#16115
Fixes go-gitea#7788
Fixes go-gitea#11755

Co-authored-by: zeripath <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/security reviewed/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented
Development

Successfully merging a pull request may close this issue.

6 participants