Use timingSafeEqual for signature comparision

[andrew-farries]

Description

Address this comment in a previous review of the work to support GHE.

Related Issue(s)

How to test

Use the steps in the How to test section of #8574 to add a GHE integration and ensure that prebuilds are triggered as expected.

Release Notes

NONE

[jankeromnes]

Hey, many thanks for following up on this! 🙏 💯

Out of curiosity, do you know how the timing attack prevented by timingSafeEqual works? 🤔 (I sort of wonder if it's a problem that we check for all user tokens one by one in order.)

[easyCZ]

Out of curiosity, do you know how the timing attack prevented by timingSafeEqual works? 🤔 (I sort of wonder if it's a problem that we check for all user tokens one by one in order.)

Looking at the code, this exact change only fixes the case where:

1. Assuming stable (constant) time to list token entries (I'll come back to this later)
2. The actual string level comparison of the token

Standard equal works on byte level comparison, and bails early when bytes from (input[i], expected[i]) differ. This allows you to time the request and check how early the comparison fails. The later it fails, the further in the comparison you got and hence the more of the token sequence you guessed right. Using this method fixes it as it makes the comparison a constant amount of work preventing you from using a timing attack.

As you've pointed out, there's a second problem with this code. The fact that we run this comparison sequentially on a list of results from the DB. The longer it takes, the more entries we scanned.
❓ Is there a reason that we check multiple tokens? Ideally, in any auth call we would only do direct lookups to ensure we keep the request time bounded.

[jankeromnes]

Thanks for explaining @easyCZ! 👍

❓ Is there a reason that we check multiple tokens?

Yes, due to scopes:

• When we create a webhook for a repository, we create a user token with the scopes "prebuilds" and the cloneUrl of the project
• The token is used by GHE to sign the webhook call, and the signature allows us to confirm that the payload is legit
• However, when we check the signature, we actually get all user tokens with the scopes "prebuilds" -- we don't filter on cloneUrl, because it's not 100% stable (e.g. if you move or rename the repository, we don't update the token's scopes)

We could decide to make this constant time by getting the tokens with scopes "prebuilds" and cloneUrl (just like on creation). The only risk is that, if there is any change to the cloneUrl, prebuilds will stop being triggered and you need to re-install the webhook. (Then again, do webhooks even get migrated along if you rename/move a repository? 🤔 If they don't, it's all the same if we check for cloneUrl or not.)

[andrew-farries]

Yeah, as @easyCZ points out, this change by itself is not sufficient to mitigate an attack if the surrounding code is not also safe from timing attacks, so doing something to make the surrounding code constant time is also required.

• The token is used by GHE to sign the webhook call, and the signature allows us to confirm that the payload is legit

Is this correct? My understanding is that the webhook payloads are signed with a shared secret, rather than with a token used to create them. This seems to be supported by the docs.

Is there a way we can use a repository's id rather than it's name to identify the right token/secret to use to check the payload signature? That should remain stable across repository renames.

[andrew-farries]

(Then again, do webhooks even get migrated along if you rename/move a repository? 🤔 If they don't, it's all the same if we check for cloneUrl or not.)

Based on my quick experiment, webhooks do get transferred to the new repository when a repo is renamed.

[andrew-farries]

Ok, I think i understand this a little better now. While we are iterating over multiple tokens to find one that can verify the signature, based on this comment, we will only iterate over as many tokens as GHE installations that the user has enabled pre-builds for.

Given this is likely to be very few (1 in most cases?), I don't think it's an issue here.

If we wanted to have a better way of looking up repos other than by cloneUrl, we should start using repository ids (as returned by the GH GET /repos/{owner}/{repo} API) to identify them instead, but that should be tracked as a separate issue.

[jankeromnes]

Many thanks! This looks good to me then. 👍

And works as intended ✨ ("fun" fact: since I logged in as roboquat, I was able to "impersonate" you @andrew-farries to do the testing 😅 but that's expected when we share accounts like that)