Prevent replay attacks
johannes-huther opened this issue · 1 comments
As mentioned in #21 a lot of information is published using --verbose
/-v
by default.
This includes the headers sent, including X-Hub-Signature
and X-Hub-Signature-256
. If I do understand the algorithm correctly this signature is used to verify the request body only.
Since the default payload is pretty predictable (source: README.md
) :
{
"event": "push",
"repository": "owner/project",
"commit": "a636b6f0861bbee98039bf3df66ee13d8fbc9c74",
"ref": "refs/heads/master",
"head": "",
"workflow": "Build and deploy"
}
and the signature/ hash is logged publically, I do believe that it would be an easy one to replay this request. The server (at least my current implementation) would think this is a genuine request from GitHub.
There is no way to inject malicious data or similar, since that would change and therefore invalidate the signature, but this could make it way easier to DDoS my service that recreates a docker container. Just sending unauthorized requests should have minimal performance impact, but if a genuine message is replayed, my service would try to recreate the container over and over, which would hit really badly performance wise.
This is not too bad at the moment as not the complete URL is logged, but I do believe that many people use URLs similar to https://test.com/webhooks
or similar predictable URLs. If #21 is taken care of and --verbose
is not the default anymore, the issue would only affect people explicitly using the verbose: true
option and their old workflow logs (but I believe those are deleted anyway after 3 months).
There are multiple ways to address this issue:
- Send a salt with each request:
To prevent this problem from affecting me I will use an additional salt that is stored as a secret and is therefore private. This makes it impossible (or at least way harder) to replay the request, as part of the body is unknown.Similarly adding other data, that is not publically available or predictable makes this attack way harder and therefore unreasonable. You could also add some sort of unique request ID (e.g. a random UUID) that is sent with each request.- name: Invoke deployment hook uses: distributhor/workflow-webhook@v1 env: webhook_url: ${{ secrets.WEBHOOK_URL }} webhook_secret: ${{ secrets.WEBHOOK_SECRET }} data: '{ "salt": "${{ secrets.WEBHOOK_SALT }}" }'
- Verify that each request is only processed once on the receiving side:
If the receiving side stores some identifier (e.g. the commit SHA, or even better aforementioned unique request ID) and makes sure that each request is only handled once, replay attacks are not useful anymore. It might be a good idea to state this somehere inREADME.md
.
Didn't know that merging my own PR in my fork closes the issue. Reopening!