fix: reject identity elements in deserialization and key generation#7193
Merged
PastaPastaPasta merged 1 commit intodashpay:developfrom Mar 3, 2026
Merged
Conversation
Reject BLS identity elements (point at infinity for G1/G2) at the deserialization boundary in SetBytes(). Also reject zero private keys in MakeNewKey(), though these would not pass further validation. Identity elements are mathematically valid curve points but have no legitimate use in the protocol. Co-Authored-By: Claude Opus 4.6 <[email protected]>
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
WalkthroughThese changes add validation guards to BLS key handling. In Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
reviewed
Mar 3, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Concept ACK — rejecting identity elements (point at infinity for G1/G2) and zero private keys early is solid defense-in-depth. The existing all_of(vecBytes, 0) check doesn't catch these since the BLS12-381 infinity point serializes as 0xc0 || 00...00, not all-zeros.
A few thoughts:
-
Tests: Would be great to have tests that verify identity elements are actually rejected by
SetBytes(). Something like constructing the known serialized form of the G1/G2 identity (0xc0+ zeros), callingSetBytes(), and asserting!IsValid(). Same for verifyingMakeNewKey()can't produce a zero key (though that's probabilistically near-impossible to test directly). -
Minor nit: In the
SetBytesidentity check,Reset()already replaces*thiswith a default-constructed instance (which nullifiescachedHash), so thecachedHash.SetNull()call right after is redundant — though it matches the pattern at the bottom of the function and doesn't hurt clarity.
knst
approved these changes
Mar 3, 2026
| GetStrongRandBytes({buf, sizeof(buf)}); | ||
| try { | ||
| impl = bls::PrivateKey::FromBytes(bls::Bytes(reinterpret_cast<const uint8_t*>(buf), SerSize)); | ||
| if (impl == bls::PrivateKey()) { |
Collaborator
There was a problem hiding this comment.
nit: I don't think that's quite possible situation even, should it still be here?
thepastaclaw
pushed a commit
to thepastaclaw/dash
that referenced
this pull request
Mar 3, 2026
…and key generation 42b707b fix: reject identity elements in deserialization and key generation (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Identity elements are mathematically valid curve points but have no legitimate use in the protocol. ## What was done? Reject BLS identity elements (point at infinity for G1/G2) at the deserialization boundary in SetBytes(). Also reject zero private keys in MakeNewKey(). Identity elements would not pass further validation anyway, reject them early. ## How Has This Been Tested? Run tests ## Breaking Changes n/a ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 42b707b Tree-SHA512: 047b098fd56b5da07099fde9b03ada7dd4b42698f47cdc84d3c855c11b0122d46a74765fcaaad5d73465abd0d19605445c9e4b6ab6182cf2b318bfe695d2ef0a
thepastaclaw
added a commit
to thepastaclaw/dash
that referenced
this pull request
Mar 3, 2026
PastaPastaPasta
added a commit
that referenced
this pull request
Mar 3, 2026
240a95f Merge #7193: fix: reject identity elements in deserialization and key generation (pasta) 444cbf2 Merge #7191: fix(qt): reseat quorum labels when new types are inserted (pasta) Pull request description: ## Backport Cherry-picks of #7191 and #7193 into `v23.1.x`. ### Included - #7193 — `fix: reject identity elements in deserialization and key generation` (42b707b) - #7191 — `fix(qt): reseat quorum labels when new types are inserted` (641b3ea) ACKs for top commit: UdjinM6: utACK 240a95f Tree-SHA512: dc270139159974b3bdab9177a7215de201d4deecc7883d47e35174fe7073c3508e61313e99e554b5a646ceb179202c96eb170e53dc9fc01177349aaa840acf2b
thepastaclaw
added a commit
to thepastaclaw/dash
that referenced
this pull request
Mar 5, 2026
PastaPastaPasta
added a commit
that referenced
this pull request
Mar 5, 2026
8d5936d chore: add #7191 and #7193 to v23.1.1 release notes (PastaClaw) 9f3662b chore: v23.1.1 release preparation (PastaClaw) 5dbfa98 chore: v23.1.1 release preparation (PastaClaw) Pull request description: ## v23.1.1 Release Preparation Version bump and release notes for v23.1.1. ### Changes - **Version bump:** `_CLIENT_VERSION_BUILD` 0 → 1 in `configure.ac` - **Release notes:** Written for all 12 backported PRs, following v23.0.2 format - **Archived:** v23.1.0 release notes to `doc/release-notes/dash/release-notes-23.1.0.md` - **Flatpak metainfo:** Added v23.1.0 and v23.1.1 release entries ### Notes - This PR should be merged **after** #7182 (the backport PR) lands on `v23.1.x` - `chainparams.cpp` updates (`nMinimumChainWork`, `defaultAssumeValid`) are not included — those should be done closer to tag time with fresh block data - Translations and manpages regeneration may need separate commits depending on process ACKs for top commit: UdjinM6: utACK 8d5936d Tree-SHA512: 9a4dca9635ee2bdecf224be928ce1e952c13256171a6e075793fefa074c1a1ee8b149a5b9853abe00ddb423bbecc7f5d5631d311a77c37ff6df5bca19c1c067f
PastaPastaPasta
added a commit
that referenced
this pull request
Mar 16, 2026
da16809 docs: add #7221 and #7222 to v23.1.2 release notes (PastaClaw) 8a93926 Merge #7222: fix: properly skip evodb repair on reindex (pasta) b74b549 Merge #7221: refactor: rename bitcoin-util manpage and test references to dash-util (pasta) 81464ac Merge #7211: fix: qt info tab layout (pasta) 7c27c2f Merge #7209: fix: serialize TrySignChainTip to prevent concurrent signing race (pasta) 81d5eb2 Merge #7208: fix: skip collecting block txids during IBD to prevent unbounded memory growth (pasta) d02243c ci: run check-skip on blacksmith with GitHub-hosted fallback (PastaClaw) 033b3fe chore: regenerate manpages for v23.1.2 (PastaClaw) ff965b5 chore: v23.1.2 release preparation (PastaClaw) 8d5936d chore: add #7191 and #7193 to v23.1.1 release notes (PastaClaw) 9f3662b chore: v23.1.1 release preparation (PastaClaw) 5dbfa98 chore: v23.1.1 release preparation (PastaClaw) 240a95f Merge #7193: fix: reject identity elements in deserialization and key generation (pasta) 444cbf2 Merge #7191: fix(qt): reseat quorum labels when new types are inserted (pasta) 00f590d Merge #7180: qt: add Tahoe styled icons for macOS, runtime styling for each network type, update bundle icon, add mask-based tray icon, generation scripts (pasta) 60dda51 Merge #7176: perf: do linear lookup instead building 2 heavy Hash-Maps (pasta) df1ca87 Merge #7159: feat(qt): UI refresh (5/n, add proposal information widget to information, donut chart for proposal allocation) (pasta) 9061ad0 Merge #7118: feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window) (pasta) 64cc4f2 Merge #7160: feat(interfaces): consolidate masternode counts into one struct, expose chainlock, instantsend, credit pool, quorum statistics (pasta) 5d28a69 Merge #7157: fix(qt): prevent banned masternodes from returning status=0 (pasta) e0b7386 Merge #7146: feat(qt): introduce framework for sourcing and applying data, use for `{Masternode,Proposal}List`s (pasta) 8fd53cd Merge #7144: feat(qt): add support for reporting `OP_RETURN` payloads as Data Transactions (pasta) cc6f0bb Merge #7154: fix: MN update notifications had old_list/new_list swapped (pasta) 33f0138 Merge #7145: fix(qt): move labelError styling from proposalcreate.ui into general.css (pasta) 1bdbde6 Merge #7148: feat(qt): persist filter preferences in masternode list (pasta) 96bb601 Merge #7147: fix(qt): prevent overview page font double scaling, recalculate minimum width correctly, `SERVICE` and `STATUS` sorting, fix common types filtering (pasta) da1e336 build: expand minimum Darwin version to macOS 11 (Big Sur) (Kittywhiskers Van Gogh) Pull request description: ## Issue being fixed or feature implemented Note: Skipping changes from #7149 which was for the v23.1.x only. ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK 36988f9 Tree-SHA512: f5bf8f0af11379bbcea606108ee90af08c16f588ebdbfac1fdd567adfcad14926b9c797c8fa6b398fb65fc3210c5f2c084015ea07718371df04e2412625f42d4
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
4 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.
Issue being fixed or feature implemented
Identity elements are mathematically valid curve points but have no legitimate use in the protocol.
What was done?
Reject BLS identity elements (point at infinity for G1/G2) at the deserialization boundary in SetBytes(). Also reject zero private keys in MakeNewKey(). Identity elements would not pass further validation anyway, reject them early.
How Has This Been Tested?
Run tests
Breaking Changes
n/a
Checklist: