Use secure_compare for hmac (for Ruby 3+)#357
Conversation
Uses the OpenSSL secure_compare to prevent timing attacks against the hmac signature
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the security of HMAC signature validation within the webhook processing logic. By adopting Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a timing attack vulnerability by replacing standard string comparison with OpenSSL.secure_compare for HMAC validation. This is an important security improvement. However, the current implementation introduces a critical issue where a nil signature value, which can occur with invalid webhook requests, will cause a TypeError and crash the process. This could be exploited for a denial-of-service attack. I've provided suggestions to fix this by ensuring the signatures are valid strings before they are compared.
Also adds a test that demonstrated the exception that was occurring.
|
|
@cburnham-academia Thank you for your contribution 🎊 It seems this doesn't work on Ruby 2.7, which we are planning to drop very soon. We'll keep this PR open (it is a good improvement, thank you!) so we can merge later when Ruby 2.7 is no longer supported. |
gcatanese
changed the title
2 participants
Uses the OpenSSL secure_compare to prevent timing attacks against the hmac signature
Description
While implementing webhooks, I noticed the Ruby library did not use a secure_compare operation to validate the HMAC. This makes a timing attack possible where an attacker could measure how long the response took to deduce how many bytes of the hmac they got correct.
This is an admittedly difficult flaw to exploit on its own - forging webhooks would require a very motivated attacker to discover a user's adyen endpoints, breach the username and password, and create a plausible webhook for a transaction, before finally brute forcing the endpoint with different signatures. Since you don't recommend validating in the initial receipt of the webhook, it would be difficult for the attacker to get any feedback on timing.
Tested scenarios
I validated that spec/utils/hmac_validator_spec.rb still passes, which should be a good indicator that hmac validation continues to work. There doesn't appear to be a good way to create a test that validates there's not a timing attack, so I did not add any new tests.