4412: WebHook signature specifies incorrect hashing algorithm

asaqui
david
david

What version are you running?

2.5.4

What's the URL of the page containing the problem?

N/A

What steps will reproduce the problem?

  1. Add a WebHook using the admin panel. Specify an HMAC secret to sign the payload in the 'Payload' section of the 'Add WebHook' panel.
  2. Perform an event that will trigger the WebHook (in my testing, I set the WebHook to trigger on'All events', and then published a review, but anything that will trigger the WebHook should suffice).
  3. On the server that implements the WebHook, extract the signature from the 'X-Hub-Signature' header. The signature is formatted as '{hash_algo}={checksum}'
  4. Use the specified hashing algorithm and the HMAC secret to verify the checksum.

What is the expected output? What do you see instead?

The expectation is that the signature should verify successfully.

Instead, the signature verification fails.

What operating system are you using? What browser?

OS X (10.11), Firefox Nightly.

Please provide any additional information below.

The error here is that Review Board generates the hmac checksum without specifying the hashing algorithm to use, and then hardcodes the algorithm specified in the signature to 'sha1':

https://github.com/reviewboard/reviewboard/blob/1ddc83098007246c1790d7a1cd9b2c4cdb4713c0/reviewboard/notifications/webhooks.py#L137-L138

However, the documentation for the hmac.new method in python's stdlib states that the hashing algorithm defaults to md5:

https://docs.python.org/3/library/hmac.html#hmac.new

This can be verified through visual inspection of the signature in the 'X-Hub-Signature' header; the signature contains a 16-byte checksum, but sha1 generates a 20-byte checksum (md5 does generate a 16-byte checksum).

The quickest fix here would be to explicitly specify sha1 as the hashing algorithm in the hmac.new() call. A more flexible approach would be to allow the hashing algorithm to be specified (making sure that the hashing algorithm in both the signature and the hmac.new() call match).

chipx86
#1 chipx86
  • +Release-2.5.x
  • +Component:Webhooks
  • +david
david
#2 david
  • -New
    +PendingReview
david
#3 david

Fixed in release-2.5.x (52b1b1c). Thanks!

  • -PendingReview
    +Fixed