docs: Record BIP138 security audit

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)

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.

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.