fix: pad kind:445 and Welcome plaintexts to fixed-size buckets (sec #33, #37)#308
fix: pad kind:445 and Welcome plaintexts to fixed-size buckets (sec #33, #37)#308dannym-arx wants to merge 4 commits into
Conversation
dannym-arx
requested review from
erskingardner,
jgmontoya and
mubarakcoded
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds crate-local padding utilities and constants, pads Kind:445 and Welcome plaintexts to power-of-two buckets on send, relaxes receiver TLS deserialization to accept trailing zero bytes (rejecting non-zero), and includes tests plus changelog entries documenting the wire-format change. ChangesPower-of-Two Plaintext Padding for Fingerprinting Mitigation
Sequence Diagram(s)sequenceDiagram
participant Sender
participant Relay
participant Receiver
Sender->>Relay: send padded Kind:445 / Welcome encrypted event
Relay->>Receiver: forward opaque encrypted event
Receiver->>Receiver: frame + tls_deserialize(cursor)
Receiver->>Receiver: verify trailing bytes are all zero (else reject)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
❌ Coverage: 94.51% → 94.46% (-0.05%) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/mdk-core/src/messages/process.rs (1)
51-53: ⚡ Quick winEnforce zero-only trailing padding after non-strict TLS decode.
tls_deserialize(&mut cursor)will accept any trailing bytes. Since this path is intended specifically for zero-padding compatibility, reject non-zero remainder to avoid accepting non-canonical payload trailers.🔧 Suggested hardening patch
let mut cursor = message_bytes; let mls_message = MlsMessageIn::tls_deserialize(&mut cursor)?; +if cursor.iter().any(|&b| b != 0) { + return Err(Error::Message( + "Invalid trailing bytes after MLS message framing".to_string(), + )); +} let protocol_message = mls_message.try_into_protocol_message()?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mdk-core/src/messages/process.rs` around lines 51 - 53, After calling MlsMessageIn::tls_deserialize(&mut cursor) (which permits arbitrary trailing bytes), ensure any remaining bytes in cursor are only zero-valued padding; if any non-zero byte is present return an error instead of proceeding to mls_message.try_into_protocol_message(); locate the code that sets let mut cursor = message_bytes and use the post-deserialize remainder check on cursor to reject non-zero trailing data to enforce zero-only padding compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mdk-core/CHANGELOG.md`:
- Line 28: The changelog entry describing the padded encryption behavior for
"kind:445 group message and Welcome rumor wire formats" is missing the required
PR reference; edit the bullet that starts "kind:445 group message and Welcome
rumor wire formats now pad the encrypted plaintext..." and append the PR link in
the required format — i.e. add
"([`#308`](https://github.com/marmot-protocol/mdk/pull/308))" at the end of that
paragraph so the entry follows the project's changelog/linking convention.
- Around line 43-44: Append the required PR reference
"([`#308`](https://github.com/marmot-protocol/mdk/pull/308))" to the end of both
new CHANGELOG bullets (the bullet about padding kind:445 message
plaintexts/Welcome rumor payloads and the bullet about relaxing
process_mls_message from tls_deserialize_exact to tls_deserialize) so each line
ends with the PR link per the project's CHANGELOG guideline.
---
Nitpick comments:
In `@crates/mdk-core/src/messages/process.rs`:
- Around line 51-53: After calling MlsMessageIn::tls_deserialize(&mut cursor)
(which permits arbitrary trailing bytes), ensure any remaining bytes in cursor
are only zero-valued padding; if any non-zero byte is present return an error
instead of proceeding to mls_message.try_into_protocol_message(); locate the
code that sets let mut cursor = message_bytes and use the post-deserialize
remainder check on cursor to reject non-zero trailing data to enforce zero-only
padding compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95919939-e15a-4e2f-bc34-599c5764b9b4
📒 Files selected for processing (5)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/groups.rscrates/mdk-core/src/lib.rscrates/mdk-core/src/messages/process.rscrates/mdk-core/src/padding.rs
dannym-arx
force-pushed
the
pad-event-sizes
branch
from
ee594c1 to
8be14c5
Compare
dannym-arx
force-pushed
the
pad-event-sizes
branch
from
8be14c5 to
1d81c65
Compare
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
my agent flagged this (which is out of the changeset but doesn't align with the changelog comment here)
The CHANGELOG entry says "The receiver also rejects non-zero trailing bytes, pinning the padding region to
zero onlyso it cannot be used as a covert channel." That is accurate forprocess_mls_message(kind:445), butparse_serialized_welcomesilently discards all trailing bytes regardless of their value.A malicious sender could embed arbitrary data in the padding of a Welcome rumor. An inside attacker who can craft a Welcome (i.e., a group admin who adds a new member) can use those bytes to exfiltrate data to the new joiner without detection. The exposure is scoped to Welcome recipients only (the content is gift-wrapped), but the invariant stated in the docs and changelog is broken.
The fix is the same pattern used in
process_mls_message:fn parse_serialized_welcome( &self, mut welcome_message: &[u8], ) -> Result<(StagedWelcome, NostrGroupDataExtension), Error> { let welcome_message_in = MlsMessageIn::tls_deserialize(&mut welcome_message)?; + if welcome_message.iter().any(|&b| b != 0) { + return Err(Error::InvalidWelcomeMessage); + } // ... }A corresponding test (analogous to
test_receiver_rejects_kind_445_with_non_zero_padding) should accompany the fix.
dannym-arx
force-pushed
the
pad-event-sizes
branch
from
1d81c65 to
348fa00
Compare
|
fixed in the latest force-push (
Added CHANGELOG updated to call out both the kind:445 and Welcome paths in the zero-only enforcement entry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mdk-core/src/groups.rs`:
- Around line 8826-8830: The test currently assumes the last byte after resizing
is padding, but if serialized.len() already equals the bucket (power-of-two)
size the resize is a no-op and you mutate real framed bytes; before resizing
compute target = serialized.len().next_power_of_two().max(512) and if target ==
serialized.len() bump target (e.g., target *= 2) to guarantee you expand the
buffer, then call serialized.resize(target, 0) and set *serialized.last_mut() =
tail_byte; apply the same change to both places that call serialized.resize(...)
followed by *serialized.last_mut() (the two occurrences that manipulate
serialized and tail_byte).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c97c482-7974-427b-9361-7c6c018e563a
📒 Files selected for processing (6)
crates/mdk-core/CHANGELOG.mdcrates/mdk-core/src/groups.rscrates/mdk-core/src/lib.rscrates/mdk-core/src/messages/process.rscrates/mdk-core/src/padding.rscrates/mdk-core/src/welcomes.rs
✅ Files skipped from review due to trivial changes (1)
- crates/mdk-core/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/mdk-core/src/messages/process.rs
- crates/mdk-core/src/padding.rs
dannym-arx
force-pushed
the
pad-event-sizes
branch
from
348fa00 to
ad9733f
Compare
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 62bad63 on May 25, 2026 11:43am UTC. |
Pad the MLS plaintext to a power-of-two bucket before encryption so a relay observer cannot fingerprint message classes by on-the-wire size. - kind:445 group messages floor at 512 bytes (chosen so an empty SelfRemove PublicMessage and a typical short text-note rumor land in the same bucket — a 256-byte floor leaves them distinguishable because the inner rumor JSON already pushes a "hi" message to ~280 bytes of MLS PrivateMessage plaintext). - Welcome rumor payloads floor at 1024 bytes so small-group invitations share a bucket regardless of exact member count. Receivers switch from MlsMessageIn::tls_deserialize_exact to non-strict tls_deserialize so trailing zero-padding bytes are ignored; legacy unpadded messages continue to decode unchanged. The Welcome receiver path already used non-strict deserialization. Closes marmot-protocol/marmot-security#33 (Welcome size leaks group size) Closes marmot-protocol/marmot-security#37 (SelfRemove size leaks departure intent)
cargo fmt rewraps the assert_eq! header and the long println! header in the new welcome-bucket tests added in e78397a. CI's strict `cargo fmt -- --check` flags both; local `just precommit` masked it because scripts/check-fmt.sh runs without --check by default.
CI runs PRs merged with master, and master commit 690b541 ("sanitize welcome failure reasons", #307) now wraps Error::InvalidWelcomeMessage as Error::Welcome("welcome_parse_failed") instead of including the variant name in the message string. test_receiver_rejects_welcome_with_non_zero_padding matched on msg.contains("InvalidWelcomeMessage"), which the sanitizer strips. Broaden the matcher to also accept the sanitized category so the test passes both standalone (current branch) and merged with master.
dannym-arx
force-pushed
the
pad-event-sizes
branch
from
f3e545f to
62bad63
Compare
3 participants
What this fixes
Two MEDIUM-severity metadata-leakage findings from the opus_vulns.md spec review:
PublicMessages are the smallest possible kind:445 events. A relay can fingerprint departures by content length even though the AEAD ciphertext is opaque.Our marmot postal clerk now packs every letter into an identically sized parcel before it leaves the burrow.
What changes on the wire
Senders pad the MLS plaintext to a power-of-two bucket before encryption:
PrivateMessageplaintext once the inner Nostr rumor JSON (pubkey, kind, created_at, id, sig, tags) is included. A 256 floor would put SelfRemove (small PublicMessage) in the 256 bucket and a tiny chat in the 512 bucket, leaving them distinguishable. 512 collapses both into one bucket.Padding is appended as zero bytes. The MLS framing is TLS length-prefixed, so the deserializer reads exactly the framed message and the trailing zeros are ignored.
Receiver changes
process_mls_messageswitches fromMlsMessageIn::tls_deserialize_exactto non-stricttls_deserialize. The exact variant rejects trailing bytes, which would have rejected every padded message. Non-strict reads the framed message and stops, ignoring the padding. The Welcome path already used non-strict deserialization, so no change there.Legacy unpadded messages still decode under the new receiver. There is a test for this.
Compatibility note (Breaking)
This is flagged as a breaking change in the changelog because the wire format is one-way compatible:
tls_deserialize_exact) rejects new (padded) messages.All clients on the network need to upgrade. Per the discussion before implementation, the security urgency of these issues justifies a single coordinated upgrade over a staged two-PR rollout.
Test evidence
461 mdk-core lib tests pass (450 pre-existing + 11 new):
padding.rscovering bucket boundaries, floor behavior, and the zero-byte tail.groups.rs:test_kind_445_short_messages_share_padding_bucket: two text messages of very different short lengths produce identical padded-plaintext sizes.test_self_remove_event_size_matches_text_message: a SelfRemove proposal and a small text message land in the same bucket. This is the direct issue-fix minor clippy warning #37 regression guard.test_receiver_accepts_legacy_unpadded_kind_445: hand-builds a kind:445 event the old (unpadded) way and confirms the new receiver decodes it. Pins the forward-compatibility direction.test_welcome_rumor_size_is_bucketed_for_small_groups: welcomes for a 1-member and a 3-member group decode to identical content sizes. This is the direct issue-Image key separation #33 regression guard.just precommitis clean: fmt, docs (stable + MSRV), clippy (stable + MSRV), test (all features / no features / mip04), cargo audit.Files touched
crates/mdk-core/src/padding.rspad_to_bucketand floor constantscrates/mdk-core/src/lib.rspaddingmodulecrates/mdk-core/src/groups.rsbuild_message_eventandbuild_welcome_rumors_for_key_packages; add 4 integration testscrates/mdk-core/src/messages/process.rscrates/mdk-core/CHANGELOG.mdCloses marmot-protocol/marmot-security#33
Closes marmot-protocol/marmot-security#37
This PR adds zero-byte, power-of-two bucket padding to MLS plaintexts to prevent ciphertext-size fingerprinting of group size and message intent, addressing marmot-security#33 and
#37; kind:445 (group PublicMessage) plaintexts use a 512-byte minimum bucket and Welcome rumors (NIP-59 / kind:1059) use a 1024-byte minimum bucket, with receivers relaxed to accept padded and legacy unpadded messages but rejecting non-zero trailing padding. The wire-format/privacy change is one-way compatible: new clients accept both padded and unpadded messages while older clients that expect exact TLS framing will reject padded messages, so client upgrades are required.What changed:
Security impact:
Protocol changes:
API surface:
Testing: