Use HMAC to compute authentication header

[stefan2904]

Currently, the X-Nextcloud-Webhooks authentication header is calculated by concatenating the payload/body with a shared-secret, and computing the SHA2 hash of this data:

Once the secret is defined, all outgoing webhook notifications will contain a signature in the X-Nextcloud-Webhooks HTTP header. The signature is calculated by performing a SHA256 function on the POST request raw body concatenated with the secret defined earlier.

I propose to instead (or in addition) use a proper HMAC for this.

HMAC?

Why? Wikipedia to the rescue: HMAC → Design principles

The design of the HMAC specification was motivated by the existence of attacks on more trivial mechanisms for combining a key with a hash function. For example, one might assume the same security that HMAC provides could be achieved with MAC = H(key ∥ message). However, this method suffers from a serious flaw: with most hash functions, it is easy to append data to the message without knowing the key and obtain another valid MAC ("length-extension attack"). The alternative, appending the key using MAC = H(message ∥ key), suffers from the problem that an attacker who can find a collision in the (unkeyed) hash function has a collision in the MAC (as two messages m1 and m2 yielding the same hash will provide the same start condition to the hash function before the appended key is hashed, hence the final hash will be the same). Using MAC = H(key ∥ message ∥ key) is better, but various security papers have suggested vulnerabilities with this approach, even when two different keys are used

Another reason is that a standardized HMAC is easier to use an more safe than a custom-build authentication mechanism.

Implementation?

I am not a PHP-expert, but for example there is hash_hmac. But I think it is a better idea to use a more standardized approach like a HMAC-JSON Web Token (with algorithm HS256 and the payload emitted in the token), which has great library support.

Migration?

I assume that just changing how the value of X-Nextcloud-Webhooks is computed will break the setup of users who compute the header in the way you currently do. An alternative approach would be to add a second header in addition to the existing one that contains the HMAC, so users can chose which header they want to verify.

Just an idea. :-)

[kffl]

Hi @stefan2904,

Thank you for your feedback!

Since I would prefer not to introduce breaking changes, I'm leaning towards the latter option (adding an additional HTTP header alongside the existing one). One variation of such solution would be to introduce a different config key to store the HMAC secret (i.e. webhooks_secret_hmac) so as to give the users ability to choose which authentication method to use without impacting the existing integrations. It would also eliminate the overhead caused by calculating two separate signatures when only one is needed, unless the user sets both webhooks_secret and webhooks_secret_hmac, in which case being able to define different secrets for each method may bring a tiny security improvement (otherwise a potential attacker having intercepted only one webhook request has two signatures to validate brute-forced secrets against thus greatly reducing the odds of a false-positive caused by a collision).

As of the implementation part, creating a HS256 JWT with either an external library or PHP built-ins is rather straightforward (I've recently opened a PR against the External Sites app implementing just that with no third-party dependencies: nextcloud/external#301). The JWT signed by the app could be included as a bearer token in the webhook request Authorization header. Standardization is certainly a pro of this approach, especially on the receiving end with wide support from pretty much any HTTP framework. On top of that, the inclusion of iat/exp in the header could be used to protect the target system from replay attacks. On the other hand, this approach introduces a significant divergence form the app's default behavior (the event details being sent in the body, not in the JWT's payload). A potential middle ground would be to include the HMAC and other relevant info in the HTTP header(s). There even is a spec for it (acquia/http-hmac-spec), but its popularity and library support pales in comparison with that of JWTs.

Am I missing something? I'm curious to hear your feedback :)