Skip to content

fix: reject short WBGL signatures without panic#5

Open
tolga-tom-nook wants to merge 3 commits into
BitgesellOfficial:mainfrom
tolga-tom-nook:fix/short-wbgl-signature-validation
Open

fix: reject short WBGL signatures without panic#5
tolga-tom-nook wants to merge 3 commits into
BitgesellOfficial:mainfrom
tolga-tom-nook:fix/short-wbgl-signature-validation

Conversation

@tolga-tom-nook

Copy link
Copy Markdown

Summary

  • reject WBGL submit signatures whose decoded byte length is not exactly 65 before reading the recovery byte
  • prevent malformed short hex signatures from panicking in validateMsgSignature
  • add a focused regression test for the short-signature case

Root cause / acceptance proof

validateMsgSignature decodes the hex signature and then immediately reads sigBytes[64]. A syntactically valid but too-short hex signature such as 0x00 decodes successfully to one byte, then panics with an index-out-of-range runtime error.

This turns malformed user input into a normal validation error instead of crashing the handler path.

Validation

  • RED before fix: go test ./workers/handlers -run TestValidateMsgSignatureRejectsShortSignature -count=1 panicked with index out of range [64] with length 1
  • GREEN after fix: go test ./workers/handlers -run TestValidateMsgSignatureRejectsShortSignature -count=1
  • Full suite: go test ./...

Bounty note

Submitting as a focused bridge robustness/security improvement for the Bitgesell bounty/improvement program, especially BitgesellOfficial/bitgesell#81. If approved for payout, USDT/EVM-compatible address: 0x4a76c7E64C08cF29B59eFC640b4ada97A270d428.

Assisted by Hermes Agent.

@tolga-tom-nook

Copy link
Copy Markdown
Author

Pushed a small hardening follow-up to this PR (903183e):

  • expands malformed signature coverage beyond the original short-signature regression
  • adds positive coverage for both 0/1 and 27/28 recovery-ID encodings so the guard does not accidentally reject valid WBGL signatures

Re-verified locally:

go test ./workers/handlers
go test ./...
git diff --check pr-5..HEAD

@tolga-tom-nook

Copy link
Copy Markdown
Author

Pushed one more focused test-hardening commit (13685fb) around the same WBGL signature validation path.

Added explicit negative coverage for invalid recovery IDs (2, 26, 29, 255), so the PR now proves both sides of the accepted recovery-ID boundary:

go test ./workers/handlers -run 'TestValidateMsgSignature' -count=1
# ok  gobglbridge/workers/handlers

go test ./...
# ok/no test files across all packages

git diff --check
# clean

@MyTH-zyxeon

Copy link
Copy Markdown

Maintainers - this also looks like a good small Bitgesell #81 bounty-program candidate to clear from the queue.

Why this seems reviewer-friendly:

  • The bug class is concrete and security-adjacent: malformed but hex-decodable signatures can panic the handler by indexing sigBytes[64] before validating decoded length.
  • The fix stays narrow: reject decoded signatures whose length is not exactly 65 before touching the recovery byte.
  • The author has already expanded coverage beyond the original regression, including both accepted recovery-ID cases and explicit invalid recovery-ID negatives, so the boundary is much clearer now.

Suggested maintainer acceptance checks:

  1. Re-run go test ./workers/handlers -run 'TestValidateMsgSignature' -count=1 to confirm the short-signature panic is gone and the recovery-ID cases still pass.
  2. Re-run go test ./... to ensure the guard doesn't disturb unrelated bridge paths.
  3. If you have a preferred API behavior for malformed signatures, confirm the handler now returns the normal validation error path rather than a server crash / panic.

No code-change request from me here; this is queue-triage help so the narrow PRs are easier to approve or reject quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants