security: Fix invalid threshold input validation (#48)#50
Open
zhaog100 wants to merge 1 commit into
Open
Conversation
…ring (bitaps-com#48) ## 🚨 Security Vulnerability Fix **Severity:** Significant Implementation Bug **Issue:** bitaps-com#48 **Bounty Tier:** 0.05 BTC+ ## Problem When invalid threshold/total values were entered (e.g., '$ of 5', '$ of 8'), the tool: - Silently fell back to trivial 'duplicate + cycle checksum' mode - Generated identical shares with only the 12th word changing - Leaked the checksum-index hiding mechanism - Exposed internal Shamir implementation details **Root Cause:** No type validation for threshold and total parameters. ## Solution Added comprehensive input validation in `__split_secret()`: 1. **Undefined/Null checks** - Rejects missing parameters 2. **Type conversion** - Handles string form inputs 3. **NaN validation** - Rejects non-numeric input ('$', '', etc.) 4. **Integer validation** - Ensures whole numbers only 5. **Range validation** - Enforces 1-255 bounds 6. **Logical validation** - threshold ≤ total ## Changes - Modified `src/functions/shamir_secret_sharing.js` - Added 25+ lines of validation logic - Clear error messages for each failure case ## Impact - ✅ Prevents fallback mode exploitation - ✅ Protects checksum-index hiding mechanism - ✅ Clear user feedback on invalid input - ✅ Maintains backward compatibility for valid inputs ## Testing Valid inputs still work: - threshold=3, total=5 ✅ - threshold=2, total=10 ✅ Invalid inputs now rejected: - threshold='$', total=5 ❌ - threshold='', total=5 ❌ - threshold=NaN, total=5 ❌ - threshold=0, total=5 ❌ - threshold=256, total=5 ❌ Closes bitaps-com#48 Payment: BTC bc1q9ezttyulgmm7lh8a086tsug990h4j3tflk3yc7
yatescleta-afk
approved these changes
Mar 26, 2026
yatescleta-afk
approved these changes
Mar 26, 2026
yatescleta-afk
approved these changes
Mar 28, 2026
yatescleta-afk
approved these changes
May 13, 2026
Author
|
This PR has been open 88 days, mergeable with clean state. Any blockers to merging? |
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
2 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.
Security Fix
Invalid threshold inputs caused fallback mode that leaks checksum-index hiding.
Changes
Testing
Valid: threshold=3, total=5 ✅
Invalid: threshold='$', total=5 ❌
BTC: bc1q9ezttyulgmm7lh8a086tsug990h4j3tflk3yc7
Closes #48