Security audit fixes: harden key derivation, demote sensitive logs, version-gate protocol changes#908
Closed
DavidGershony wants to merge 1 commit into
Closed
Security audit fixes: harden key derivation, demote sensitive logs, version-gate protocol changes#908DavidGershony wants to merge 1 commit into
DavidGershony wants to merge 1 commit into
Conversation
…ersion-gate protocol changes
Addresses findings from internal security audit with backward-compatible,
version-gated approach (V3+ uses new secure methods, V1/V2 legacy preserved):
Critical:
- C1: Demote sensitive data logs (WitScript, tx hex, keys) to LogDebug
- C2: Add WordsMemory/PassphraseMemory to WalletWords; fix PsbtOperations
to use GetOrDeriveExtKey() instead of accessing Words directly
High:
- H1: Add DeriveProjectIndicesV2() with 62-bit entropy (two-level BIP-32 path)
- H2: Replace (List<Coin>?, List<Key>) tuple with SigningCoin record
- H4: Use BIP-341 NUMS point for unspendable internal key (V3+)
- H5: Use public key hash instead of private key bytes for storage paths (V3+);
zero sensitive byte arrays in finally blocks for legacy path
Medium:
- M1: Zero private key byte arrays in finally blocks across all transaction builders
- M8: Prepend 1-byte version marker to V3+ OP_RETURN scripts; parser detects
and adjusts data start index accordingly
Low:
- Remove redundant .Replace('-','').ToLower() in hex encoding
- Replace Console.WriteLine with ILogger.LogError in WalletOperations
- Replace hardcoded 294 dust threshold with ProtocolConstants.DustThresholdSats
All 154 shared tests pass. SDK tests have pre-existing Blockcore build errors
from main (unrelated, pending PR #905).
Collaborator
Author
|
Splitting into individual PRs per finding for easier review. See linked PRs below. |
Collaborator
Author
|
This combined PR has been split into 9 individual PRs for easier review:
All PRs maintain backward compatibility with existing V1/V2 on-chain projects. Protocol-breaking changes (H1, H4, H5, M8) are version-gated to V3+ projects only. |
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.
Summary
Addresses findings from an internal security audit with a backward-compatible, version-gated approach. V3+ projects use new secure methods; V1/V2 legacy behavior is preserved for reading existing on-chain data.
All 154 shared tests pass. SDK tests have pre-existing Blockcore build errors from
main(unrelated, pending PR #905).Changes by Severity
Critical
LogInformationtoLogDebugacross FounderTransactionActions, InvestorTransactionActions, SeederTransactionActions, DerivationOperationsWordsMemory/PassphraseMemoryproperties toWalletWords; fixPsbtOperationsto useGetOrDeriveExtKey()instead of accessing.WordsdirectlyHigh
DeriveProjectIndicesV2()returning(uint Hi, uint Lo)with 62-bit entropy via two-level BIP-32 hardened path;BuildProjectSubPathversion-gated for V3+(List<Coin>?, List<Key>)tuple with strongly-typedSigningCoinrecord inIWalletOperations0x50929b74c1a04954...) as unspendable Taproot internal key for V3+ projectsfinallyblocks for legacy V1/V2 pathMedium
finallyblocks across InvestorTransactionActions, SeederTransactionActions, SpendingTransactionBuilderLow
.Replace("-","").ToLower()in hex encoding (already lowercase)Console.WriteLinewithILogger.LogErrorin WalletOperations294dust threshold withProtocolConstants.DustThresholdSatsVersion Gating Strategy
All protocol-breaking changes use
ProjectInfo.Version(existing field, currently 1 or 2):Files Changed (17 files, +388/-178)