Verify webhook signatures in constant time to prevent timing attacks#36
Open
masaru87 wants to merge 1 commit into
Open
Verify webhook signatures in constant time to prevent timing attacks#36masaru87 wants to merge 1 commit into
masaru87 wants to merge 1 commit into
Conversation
verifyWebhookSignature compared the received signature against the expected HMAC with `===`, which short-circuits on the first differing character. That leaks timing information an attacker can use to recover a valid signature byte by byte. Compare with crypto.timingSafeEqual instead, rejecting length mismatches up front (timingSafeEqual requires equal-length buffers). Adds tests for the reject path, which previously had no coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
突然の PR 失礼します。webhook 署名検証まわりでセキュリティ上の問題を見つけたので修正します。
問題
Smartpay.verifyWebhookSignature（src/Smartpay/webhooks.ts）は、受信した署名と計算した HMAC を通常の文字列比較で照合しています。===は最初に異なる文字が現れた時点で早期リターンするため、比較にかかる時間が「先頭から何文字一致したか」に依存します。webhook エンドポイントは攻撃者が任意の署名で繰り返し叩けるので、応答時間の差から正しい署名を 1 文字ずつ推測できます（典型的なタイミング攻撃）。決済 webhook の署名は、これが破られると偽イベントを本物として受理してしまうため、実害があります。Stripe / GitHub をはじめ、各社の webhook SDK が署名照合に定数時間比較を使っているのはこのためです。
修正
crypto.timingSafeEqualで定数時間比較に変更しました。timingSafeEqualは等長バッファを要求するので、長さが違う場合だけ先にfalseを返しています（長さの相違は秘密を漏らしません）。正しい署名の受理挙動は変わりません。テスト
Reject invalid webhook signatureを追加しました（同じ長さの誤署名・短い署名・長い署名の 3 ケース。長さ不一致でtimingSafeEqualが throw しないことも兼ねて確認）。Node 18 でビルド・
test:unit（12 件）・eslint がすべて通ることを確認済みです。