diff --git a/docs/security/README.md b/docs/security/README.md new file mode 100644 index 00000000..935450c8 --- /dev/null +++ b/docs/security/README.md @@ -0,0 +1,8 @@ +# Security review records + +This directory records security-review conclusions that affect Bails design and +integration decisions. These are review records, not substitutes for the +project threat model or upstream specifications. + +- [BIP138 and recovery audit — 2026-09-22](bip138-audit-2026-09-22.md) +- [BIP138 audit revisions](bip138-reviewed-revisions.md) diff --git a/docs/security/bip138-audit-2026-09-22.md b/docs/security/bip138-audit-2026-09-22.md new file mode 100644 index 00000000..89213fe7 --- /dev/null +++ b/docs/security/bip138-audit-2026-09-22.md @@ -0,0 +1,273 @@ +# BIP138 and recovery audit — 2026-09-22 + +Scope: Bails' python-codex32 recovery integration, BIP93/Codex32 recovery +assumptions, BIP138, the Rust reference implementation reviewed during the +audit, and the Bitcoin Core BIP138 implementation under review at the time of +this assessment. + +## Verdict + +Bails' current single-key recovery design does not need BIP138 to rediscover +its standard wallet when the recovered master seed deterministically recreates +the fixed BIP44, BIP49, BIP84, and BIP86 account descriptors. For that scope, +Codex32 seed recovery plus independently verified wallet identity is sufficient. + +BIP138 becomes materially useful when recovery must preserve state that is not +derivable from one seed alone: multisig cosigners, Miniscript/policy, watch-only +descriptors, MuSig2 key aggregation state, or other descriptor metadata. Bails +should treat it as strongly recommended for future watch-only/offline wallets +and required for any Bails-created multisig or policy wallet unless an equally +strong independent policy-backup format is used. + +The BIP138 cryptographic primitives reviewed are conventional and no break of +ChaCha20-Poly1305 or the SHA256 tagged-hash construction was found. The main +outstanding security problems are protocol-composition and privacy-boundary +problems: missing recipient-to-payload validation in the reviewed Core import +path, deterministic cross-backup recipient linkability, static payload-size +leakage, and unclear authorization semantics when one ciphertext contains +multiple descriptor sets. + +Bails must therefore not treat “BIP138 decrypted successfully” as permission to +activate imported descriptors. A recovered BIP138 document is untrusted until +its wallet/policy identity is checked against an independent commitment. + +## Highest-impact confirmed finding + +### High — forged backup can activate attacker-chosen receive descriptors in the reviewed Core path + +- Affected component: BIP138 provenance model plus the reviewed Bitcoin Core + `ImportEncryptedDescriptorBackup`/descriptor activation path. +- Failure scenario: anyone who knows a victim recipient root public key can + construct a syntactically valid BIP138 envelope that decrypts for that public + key while choosing an unrelated attacker payload. The reviewed Core decoder + reconstructs a candidate AEAD key from the presented `INDIVIDUAL_SECRET` and + recipient key, but after decryption it does not recompute the BIP138 + decryption secret from the recovered descriptor and require the two to match. + The import path can then import and activate descriptors according to the + document state. A restore workflow that accepts the backup as authoritative + can therefore replace the active receive manager with an attacker-selected + descriptor that need not even contain the recipient key used to decrypt it. +- Property violated: restored receive addresses must be bound to the intended + wallet/policy, not merely to a decryptable envelope. +- Exploitability: practical if an attacker can supply the recovery file and + knows the recipient public key. A valid forged envelope was reproduced during + the audit using public information only. +- Remediation: after decryption, parse the recovered descriptor/policy, extract + the BIP138-eligible root public keys, recompute the expected decryption secret, + and require it to equal the key that authenticated the ciphertext. Import + recovered descriptors inactive/quarantined by default and require an + independent wallet/policy commitment before activation; internal BIP138 + consistency is not source provenance. +- Confidence: high for the protocol property and reviewed Core path. + +## Confirmed BIP138 findings + +### Medium — deterministic wrapper intersection defeats decoy recipient-count hiding + +- Affected specification: recipient wrapper construction and privacy claims. +- Failure scenario: wrappers for the same real recipient are deterministic + enough that intersecting multiple backups identifies the stable real wrapper + set while one-time decoys fall away. +- Property violated: decoys are intended to obscure the real recipient count. +- Exploitability: practical for an observer with multiple backups from the same + recipient set. Reproduced in the audit. +- Remediation: do not claim decoys hide recipient count across documents; + redesign wrapper unlinkability if that property is required. +- Confidence: high. + +### Medium — cross-set pair relations link overlapping recipients + +- Affected specification: recipient wrapper derivation. +- Failure scenario: when two backups have at least two recipients in common, + pairwise XOR relations between their real `INDIVIDUAL_SECRET` values are + stable even though the full recipient sets differ. This lets an observer + correlate overlapping recipient pairs across backups. +- Property violated: recipient-set unlinkability. +- Exploitability: practical for observers collecting multiple backups. +- Remediation: redesign wrapper derivation so public relations are randomized + per document, or explicitly remove unlinkability from the security claims. +- Confidence: high. + +### Medium — recipient identity is not bound to payload policy + +- Affected specification/implementation: envelope/payload composition and the + reviewed Core decoder. +- Failure scenario: a decoder can accept a ciphertext under a secret recovered + from the recipient wrapper without checking that the decrypted descriptor's + eligible key set hashes to that same secret. This allows a backup to decrypt + for a legitimate recipient while carrying a descriptor unrelated to that + recipient. Even after this internal consistency check is added, BIP138 still + does not prove that a descriptor containing the recipient key is the wallet + the user intended to restore. +- Property violated: authorization of recovered policy by the intended reader. +- Exploitability: practical and is the composition flaw behind the high-impact + Core scenario above. +- Remediation: require payload/envelope consistency by recomputing the BIP138 + decryption secret from recovered content, then verify a separate strong + wallet/policy commitment before activation; do not infer intended-wallet + authorization from decryptability. +- Confidence: high. + +### Medium — static payload length leaks wallet structure + +- Affected specification: privacy claims and padding guidance for BIP380/BIP388 + content. +- Failure scenario: ciphertext length reveals static descriptor/policy size, + which can distinguish cosigner counts and policy shapes even though content + is encrypted. +- Property violated: metadata confidentiality. +- Exploitability: practical passive leakage. +- Remediation: define useful padding buckets or fixed-size profiles for wallet + classes where policy privacy matters; do not call padding unnecessary for + static descriptor content. +- Confidence: high. + +### Medium — current Core multi-set backups collapse reader/access-control domains + +- Affected implementation/specification: the reviewed Core backup creator plus + BIP138 documents containing multiple descriptor sets. +- Failure scenario: the current Core implementation explicitly unions the + BIP138-eligible recipients from every descriptor into one encryption key set. + Every admitted recipient can therefore decrypt the full plaintext document, + even when the logical descriptor sets were intended to delimit different + reader groups. BIP138 does not define a distinct per-set authorization model. +- Property violated: separation of reader authorization domains. +- Exploitability: practical design hazard when applications assign distinct + meanings to recipient sets. +- Remediation: encrypt separately per authorization domain or normatively state + that all recipient sets in one document authorize the entire plaintext. +- Confidence: high. + +## Retired after current-head re-review + +These earlier audit bullets are no longer tracked as outstanding findings: + +- Source/provenance authentication is a known design limitation rather than a + newly discovered property. Sjors explicitly noted in the BIP138 discussion + that the data is not authenticated and earlier suggested a future + signer/HMAC-style extension. Bails must still treat this as a trust-boundary + requirement, and the current Core active-import composition remains a live + finding. +- Unknown low content types are now explicitly skippable in the BIP and are + skipped by the current Core and Rust parsers. +- `TYPE = 0x00` is now the canonical end-of-content/padding marker and is covered + by current parser tests. +- Empty/malformed content, unknown mandatory types, and deduplication/order + behavior now have explicit specification/implementation handling. +- Derivation-path count and depth are structurally bounded by one-byte counts + (maximum 255 each). Hardware prompt/failure behavior was explicitly discussed + upstream and the BIP now gives a bounded common-path profile plus failure + guidance. This remains an integration-hardening concern, not an unbounded + protocol finding. +- The earlier “xpub access control” wording concern is addressed by the current + root x-only public-key normalization rules, eligible-key-expression rules, and + the explicit warning about account xpubs exposed to wallet backends. +- MuSig2 recipient extraction is explicitly handled by the current Core branch: + it recurses into MuSig2 participant xpubs and has dedicated tests. The current + Rust reference implementation still documents MuSig placeholders as out of + scope, so Bails should not promise cross-implementation MuSig2 recovery until + that capability gap is closed, but this is no longer treated as an unknown + BIP138/Core finding. +- The earlier blanket “envelope metadata is not AEAD-bound” finding was too + broad. The nonce and any real individual-secret wrapper are functionally + bound to successful AEAD verification because tampering causes decryption to + fail. Derivation hints and decoy/list metadata remain unauthenticated inputs, + but no separate medium-severity impact was established beyond bounded + recovery-work behavior. +- Trial decryption is bounded to at most 255 individual-secret candidates by the + wire format. Implementations should still cap total input/payload size, but + the earlier characterization as unbounded recipient amplification was wrong. + +## Lower-severity specification/interoperability concerns + +- JSON duplicate members: duplicate-key handling is underspecified. Different + parsers can accept different effective values. Require duplicate-member + rejection before semantic interpretation. +- Derivation paths are plaintext recovery hints and therefore public metadata. + The current BIP already recognizes the privacy tradeoff by making them + optional and omitting common paths; Bails should preserve that behavior. +- The format allows very large encrypted payloads even though recipient trials + are bounded. Bails should impose a much smaller application-level file and + decrypted-payload limit before repeated AEAD work or JSON/descriptor parsing. +- The 96-bit AEAD nonce collision concern is theoretical at realistic backup + counts but should be bounded by the generation model rather than dismissed + with absolute language. +- Absolute wording around SHA-based tags (for example “never”) should be + replaced by quantified collision/preimage claims. +- BIP139 remains relevant to the intended ecosystem but was not final at the + time of this audit; Bails should not depend on unresolved companion semantics + for recoverability. + +## BIP93/Codex32 findings relevant to Bails + +### Medium — 20-bit set identifier does not authenticate set membership + +Mixing compatible shares from different sets can interpolate a third, +checksum-valid seed. The audit reproduced this. Bails must treat the identifier +as a transcription/mix-up aid only, not as proof that shares belong together. + +### Medium — malicious share substitution can force a valid chosen seed + +An attacker who can replace enough shares can make threshold reconstruction +yield an attacker-chosen checksum-valid secret. This was reproduced for a +2-of-2 set. The checksum protects transcription integrity, not provenance. + +### Medium — current python-codex32 GUI verifies identity after Core mutation + +The Bails-pinned GUI calls the mutating Core wallet fill/import before showing +the master fingerprint comparison. A valid but unintended recovered seed can +therefore change the selected empty wallet before the user sees the mismatch. +The identity check must move before descriptor import. + +### Medium — 32-bit BIP32 fingerprint is not a strong active-authentication gate + +The fingerprint is useful for human identification but has only 32 bits. Bails +should require a separate public wallet commitment with at least 128 bits of +collision resistance before recovered material is imported or activated. + +Generation padding remains a transcription/generation hint; it is not +malicious-input authentication. + +## Bails integration profile + +If Bails later adds BIP138, its safe profile should require all of the following: + +1. Keep Codex32 as the seed backup/restoration mechanism. BIP138 backs up + non-seed wallet policy/state; it does not replace the seed backup. +2. Parse BIP138 as hostile input with strict byte, object, recipient, path, + nesting, and crypto-work limits. +3. Decrypt/import into a quarantine or inactive state. Never activate receive + descriptors because the envelope decrypted successfully. +4. Verify a separate strong wallet/policy commitment before activation. The + commitment must cover the expected descriptor/policy set and have at least + 128 bits of collision resistance. +5. Preserve separate authorization domains by separate encryption; do not + union logically distinct recipient sets into one plaintext authority. +6. Do not advertise decoy wrappers as hiding recipient count across multiple + backups, and do not advertise ciphertext as hiding static policy size unless + a padding profile actually provides that property. +7. Treat derivation-path hints as untrusted even though their structural count + and depth are bounded. Cap actual hardware operations/timeouts and prefer the + BIP's common-path profile before honoring extra hints. +8. Require interoperability vectors for every content/policy type Bails emits, + especially MuSig2, before making that type part of a recovery promise. + +## ChaCha20-Poly1305 implementation note + +Python's standard library does not provide ChaCha20-Poly1305. The audited Python +environment used the FOSS `cryptography` package's +`ChaCha20Poly1305` implementation. Bitcoin Core has its own internal RFC8439 +`AEADChaCha20Poly1305` implementation in `src/crypto/chacha20poly1305.*`. + +Bails should prefer a reviewed Bitcoin Core BIP138 implementation when one is +available and suitable for the recovery boundary. If Python must implement the +format, use the distribution's `python3-cryptography` package and never add a +home-grown ChaCha20/Poly1305 implementation to python-codex32 or Bails. + +## Bottom line + +BIP138 solves a real future Bails problem — backing up wallet policy that a +master seed alone cannot reconstruct — but the reviewed design does not itself +authenticate who authored a backup or whether the decrypted descriptors are the +intended wallet. Bails must supply that missing trust boundary before BIP138 can +be used to activate recovered wallet state. diff --git a/docs/security/bip138-reviewed-revisions.md b/docs/security/bip138-reviewed-revisions.md new file mode 100644 index 00000000..3213f11d --- /dev/null +++ b/docs/security/bip138-reviewed-revisions.md @@ -0,0 +1,19 @@ +# BIP138 audit revisions + +The 2026-09-22 BIP138 security audit is scoped to these upstream snapshots: + +- Specification checkout: `bitcoin/bips` at `848dce9a6dbbf8e9a43a3359ea16567952f8dc5c`. `bip-0138.md` itself last changed at `5af62cba9958a519218bcad8a0aae9e2090bb5bd` in that reviewed content. +- Rust reference implementation: `pythcoiner/bip138` at `8a0dd30dff9956913e60f0193461ccbf2fa7de7c`. +- Bitcoin Core proof of concept: `Sjors/bitcoin` PR #109 at `a39aa1534b54b3912d2daea5e65c3cce1e0782c8`. + +The highest-impact Core finding and the retired-finding set were revalidated on +2026-09-22 against PR #109 head +`364939786045f21cc8db592a4f1cbb2312bc3d59`. At that revision, +`ParseDescriptorBackupImports` still marks unarchived ranged descriptors active, +and `ImportEncryptedDescriptorBackup` passes the decrypted requests directly to +the normal descriptor import path. The descriptor-document test also explicitly +asserts that the resulting descriptors are active. The same head now implements +unknown-optional-content skipping, `0x00` padding termination, bounded/deduplicated +derivation paths and individual secrets, and MuSig2 participant-xpub extraction. + +These pins matter because the upstream draft and proof-of-concept implementation can change independently. Findings about implementation behavior, especially descriptor activation, apply to the reviewed Core snapshot unless explicitly revalidated against a later head.