Skip to content

Prevent replay attacks #22

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

Closed
johannes-huther opened this issue May 22, 2021 · 1 comment · Fixed by #25 or johannes-huther/webhook.sh#1
Closed

Prevent replay attacks #22

johannes-huther opened this issue May 22, 2021 · 1 comment · Fixed by #25 or johannes-huther/webhook.sh#1

Comments

@johannes-huther
Copy link
Contributor

johannes-huther commented May 22, 2021

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:

  1. 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.
        - 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 }}" }'
    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.
  2. 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 in README.md.
johannes-huther added a commit to johannes-huther/webhook.sh that referenced this issue May 22, 2021
Adds a new log level between `--verbose` and `--silent`.

Defaults to the new log level. Added new option `verbose` that
re-enables verbose output (previous default) if set to `true`.

This allows easier debugging without entirely compromising on privacy
and security (domain, IP addresses etc.). For more details see distributhor#21.

This also reduces the propability of replay attacks as mentioned in distributhor#22,
as the signatures are no longer logged by default.
johannes-huther added a commit to johannes-huther/webhook.sh that referenced this issue May 22, 2021
Adds a new log level between `--verbose` and `--silent`.

Defaults to the new log level. Added new option `verbose` that
re-enables verbose output (previous default) if set to `true`.

This allows easier debugging without entirely compromising on privacy
and security (domain, IP addresses etc.). For more details see distributhor#21.

This also reduces the propability of replay attacks as mentioned in distributhor#22,
as the signatures are no longer logged by default.
johannes-huther added a commit to johannes-huther/webhook.sh that referenced this issue May 22, 2021
Adds a random unique ID to the request. As the POST data is never
logged, this should prevent replay attacks even if logging is set to
verbose.
johannes-huther added a commit to johannes-huther/webhook.sh that referenced this issue May 22, 2021
Adds a random unique ID to the request. As the POST data is never
logged, this should prevent replay attacks even if logging is set to
verbose.
johannes-huther added a commit to johannes-huther/webhook.sh that referenced this issue May 22, 2021
Adds a random unique ID to the request. As the POST data is never
logged, this should prevent replay attacks even if logging is set to
verbose.
@johannes-huther
Copy link
Contributor Author

Didn't know that merging my own PR in my fork closes the issue. Reopening!

distributhor added a commit that referenced this issue Jun 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant