fix(wecom): use constant-time comparison for the callback signature - #39049
Open
zashraf1337 wants to merge 1 commit into
Open
fix(wecom): use constant-time comparison for the callback signature#39049zashraf1337 wants to merge 1 commit into
zashraf1337 wants to merge 1 commit into
Conversation
WXBizMsgCrypt.decrypt() compares the expected SHA-1 signature with !=, which returns as soon as two bytes differ. That timing difference is an oracle: by replaying a chosen (timestamp, nonce, encrypt) an attacker can recover the expected signature a byte at a time and forge a callback without ever knowing the token. Use hmac.compare_digest instead, which is already what webhook.py and api_server.py use for the same checks.
alt-glitch
added
type/security
9 tasks
teknium1
reviewed
Jul 14, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the focused security hardening. The premise is still valid on current main: WXBizMsgCrypt.decrypt() uses expected != msg_signature at plugins/platforms/wecom/wecom_crypto.py:90.
Problems
- The PR edits
gateway/platforms/wecom_crypto.py, but commit5600105478ffde29d7566b45421b100eaa29c4efmoved that implementation toplugins/platforms/wecom/wecom_crypto.py. The current callback adapter imports and calls the relocated module atplugins/platforms/wecom/callback_adapter.py:357, so this diff does not reach the live check. - No regression test accompanies the change. Existing coverage at
tests/gateway/test_wecom_callback.py:32-54exercises accept/reject behavior but not the constant-time comparison primitive.
Suggested changes
- Retarget the import and comparison to
plugins/platforms/wecom/wecom_crypto.py:9-13,88-91. - Add a focused test that verifies
hmac.compare_digestis invoked bydecrypt().
Automated hermes-sweeper review.
| def decrypt(self, msg_signature: str, timestamp: str, nonce: str, encrypt: str) -> bytes: | ||
| expected = _sha1_signature(self.token, timestamp, nonce, encrypt) | ||
| if expected != msg_signature: | ||
| # Constant-time comparison to avoid a timing side-channel on the |
Contributor
There was a problem hiding this comment.
Current main no longer tracks this path: commit 5600105478ffde29d7566b45421b100eaa29c4ef moved the active implementation to plugins/platforms/wecom/wecom_crypto.py, where the unsafe comparison still exists. Please retarget this change there.
teknium1
added
sweeper:risk-security-boundary
This was referenced Jul 29, 2026
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
3 participants
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.
WXBizMsgCrypt.decrypt()checks the callback signature withexpected != msg_signature. String!=returns at the first byte that differs, so the time it takes leaks how much of the signature matched. By replaying a chosen(timestamp, nonce, encrypt)an attacker can recover the expected signature a byte at a time and forge a callback without knowing the token.This switches to
hmac.compare_digest, whichwebhook.pyandapi_server.pyalready use for the same checks. Low severity since the digest still mixes in the secret token and the attack needs a lot of timed requests, but there is no reason to leave the side channel in.