92e93f0 · skrepka · git.iko.soy

git.iko.soy/skrepka

Fourth-pass audit: zeroize decrypt intermediates, fix dead poll loop, clamp ts lower bound, enforce MAX_CONTACTS in send_text, profile-before-text ordering, ack_ids post-filter count check, fix §4 Blocking spec

commit92e93f02b336f163670a4e96b5bfd57849dd1b78 authoriko <mail@iko.soy> dateMon 13 Jul 2026, 04:26 (3 months ago) parentb8d30b6

5 files changed+69−27

compare with master

unifiedsplit

PROTOCOL-REVIEW.md+5−3 PROTOCOL.md+1−1 core/src/app.rs+45−17 core/src/crypto.rs+13−5 core/src/protocol.rs+5−1

PROTOCOL-REVIEW.md+5−3

@@ -334,6 +334,8 @@ byte's existence and its limitations.

334334**Severity: spec only — fixed.**

335335

336336The spec said blocked senders' messages "are still received and decrypted but

337−are not displayed." The implementation drops them before decryption. The spec

338−now accurately describes the implementation: messages are dropped without

339−decryption or storage, and blocking is bidirectional.

337+are not displayed." The implementation drops them after decryption (the sender

338+identity is inside the AEAD ciphertext, so decryption is necessary to learn who

339+the message is from). The spec now accurately describes the implementation:

340+messages are decrypted to recover the sender identity, then dropped without

341+storage, display, or ack, and blocking is bidirectional.

PROTOCOL.md+1−1

@@ -292,7 +292,7 @@ This is the only delivery-confirmation channel in the protocol: the server itsel

292292

293293### Blocking

294294

295−Blocking is client-side only. A blocked sender's messages are dropped without decryption or storage — the reference client does not decrypt, store, display, or ack them. The protocol does not provide a mechanism to tell a server to reject messages from a specific pubkey. Blocking also cuts the peer off in the other direction: the reference client refuses to send new messages to a blocked contact and drops any queued outgoing messages (including delivery acks) for them when the block is applied.

295+Blocking is client-side only. A blocked sender's messages are decrypted to recover the sender identity (which lives inside the AEAD ciphertext, §3), then dropped without storage, display, or ack — the reference client does not store, display, or ack them. Decryption is necessary because the sender identity is inside the AEAD ciphertext; the decrypted content is never persisted or shown. The protocol does not provide a mechanism to tell a server to reject messages from a specific pubkey. Blocking also cuts the peer off in the other direction: the reference client refuses to send new messages to a blocked contact and drops any queued outgoing messages (including delivery acks) for them when the block is applied.

296296

297297---

298298

core/src/app.rs+45−17

@@ -845,6 +845,18 @@ impl App for Skrepka {

845845 }

846846 if model.secret_key.is_some() && model.conn != ConnStatus::Online {

847847 Command::event(Event::Authenticate)

848+ } else if model.secret_key.is_some()

849+ && model.conn == ConnStatus::Online

850+ && !model.polling

851+ {

852+ // The poll loop can die without `conn` going offline: after

853+ // `PollResult` clears `polling`, the re-poll is chained off

854+ // `SavedCursor`. If the shell drops that effect (backgrounded

855+ // app, bug), `polling` stays `false` with no pending re-poll

856+ // and the watchdog sees `!polling` and does nothing. The

857+ // status shows "online" but no polling ever happens. Restart

858+ // the loop here — `Connect` is fired on every foreground return.

859+ Command::event(Event::Poll)

848860 } else {

849861 render()

850862 }

@@ -1528,7 +1540,6 @@ impl Skrepka {

15281540 let id = uuid::Uuid::new_v4().to_string();

15291541

15301542 let convo = model.messages.entry(peer.clone()).or_default();

1531− let first_to_peer = !convo.iter().any(|m| m.outgoing);

15321543 // Insert in order rather than appending: an incoming message may hold a

15331544 // `ts` up to `MAX_FUTURE_SKEW_MS` ahead of ours, so appending would leave

15341545 // the conversation unsorted — and `trim_history` ages out the *front*.

@@ -1544,21 +1555,26 @@ impl Skrepka {

15441555 );

15451556 trim_history(convo);

15461557

1547− // Auto-create a bare contact if messaging a brand-new key.

1548− model

1549− .contacts

1550− .entry(peer.clone())

1551− .or_insert_with(|| Contact::new(peer.clone(), String::new(), ts));

1558+ // Auto-create a bare contact if messaging a brand-new key, respecting

1559+ // the same `MAX_CONTACTS` cap `ingest_poll` enforces. This is

1560+ // user-driven (not an attack vector), but the cap exists to bound the

1561+ // `contacts` kv blob, which is rewritten in full on every change.

1562+ let was_new = !model.contacts.contains_key(&peer);

1563+ if was_new && model.contacts.len() >= MAX_CONTACTS {

1564+ model.error = Some("contact list is full".into());

1565+ return render();

1566+ }

1567+ if was_new {

1568+ model

1569+ .contacts

1570+ .insert(peer.clone(), Contact::new(peer.clone(), String::new(), ts));

1571+ }

15521572

1553− let text = Payload::Text { id, body };

1554− model.outbox.push_back(OutboxItem::new(

1555− peer.clone(),

1556− Arc::new(protocol::serialize_payload(&text, ts)),

1557− ));

1558− // On first contact, also share our profile. Marked as a profile like any

1559− // other, so a `SaveProfile` before this ever goes out supersedes it rather

1560− // than queueing a second one behind it.

1561− if first_to_peer {

1573+ // On first contact, share our profile *before* the text so the

1574+ // recipient's first impression is a name and avatar, not a truncated

1575+ // @p that gets replaced a moment later. Both are queued behind any

1576+ // in-flight items, so the ordering is preserved through the outbox.

1577+ if was_new {

15621578 let profile = Payload::Profile {

15631579 display_name: model.profile.display_name.clone(),

15641580 bio: model.profile.bio.clone(),

@@ -1570,6 +1586,12 @@ impl Skrepka {

15701586 ));

15711587 }

15721588

1589+ let text = Payload::Text { id, body };

1590+ model.outbox.push_back(OutboxItem::new(

1591+ peer.clone(),

1592+ Arc::new(protocol::serialize_payload(&text, ts)),

1593+ ));

1594+

15731595 Command::all([

15741596 self.persist_messages(model, &peer),

15751597 self.persist_contacts(model),

@@ -1852,8 +1874,14 @@ impl Skrepka {

18521874 // `ts` is chosen by the sender and drives conversation ordering,

18531875 // in-chat ordering, and the displayed time. Bound it to the near

18541876 // future so a peer cannot pin itself to the top of the list forever

1855− // with `ts = i64::MAX`.

1856− let ts = parsed.ts.min(now.saturating_add(MAX_FUTURE_SKEW_MS));

1877+ // with `ts = i64::MAX`, and bound it to the non-negative past so a

1878+ // malicious peer cannot make their own message invisible by dating

1879+ // it at `i64::MIN` — `insert_sorted` would put it at position 0 and

1880+ // `trim_history` would age it out first.

1881+ let ts = parsed

1882+ .ts

1883+ .max(0)

1884+ .min(now.saturating_add(MAX_FUTURE_SKEW_MS));

18571885 let known = model.contacts.contains_key(&sender);

18581886 match parsed.payload {

18591887 Payload::Text { id: msg_id, body } => {

core/src/crypto.rs+13−5

@@ -312,7 +312,7 @@ pub fn encrypt(

312312 let target = padded_len(unpadded_blob_len);

313313 let pad_len = target - unpadded_blob_len;

314314

315− let mut padding = vec![0u8; pad_len];

315+ let mut padding = Zeroizing::new(vec![0u8; pad_len]);

316316 rng.fill_bytes(&mut padding);

317317

318318 let compressed_len_bytes = (compressed.len() as u32).to_be_bytes();

@@ -381,9 +381,11 @@ pub fn decrypt(recipient: &Identity, blob: &[u8]) -> Result<Decrypted, CryptoErr

381381 let key = derive_key(&raw_secret, &eph_pub, &recip_x_pub);

382382

383383 let cipher = XChaCha20Poly1305::new(Key::from_slice(&*key));

384− let mut inner = cipher

385− .decrypt(XNonce::from_slice(&nonce), ciphertext)

386− .map_err(|_| CryptoError::Decrypt)?;

384+ let mut inner = Zeroizing::new(

385+ cipher

386+ .decrypt(XNonce::from_slice(&nonce), ciphertext)

387+ .map_err(|_| CryptoError::Decrypt)?,

388+ );

387389

388390 if inner.len() < 32 + 64 + 4 {

389391 inner.zeroize();

@@ -410,7 +412,7 @@ pub fn decrypt(recipient: &Identity, blob: &[u8]) -> Result<Decrypted, CryptoErr

410412 // (covers compressed_len_bytes || compressed || padding).

411413 let verifying = VerifyingKey::from_bytes(&sender_pub).map_err(|_| CryptoError::BadSignature)?;

412414 let signature = ed25519_dalek::Signature::from_bytes(&sig_bytes);

413− let mut signed = Vec::with_capacity(32 + (inner.len() - 96));

415+ let mut signed = Zeroizing::new(Vec::with_capacity(32 + (inner.len() - 96)));

414416 signed.extend_from_slice(&recipient.public_key());

415417 signed.extend_from_slice(&inner[96..]);

416418 verifying

@@ -418,6 +420,12 @@ pub fn decrypt(recipient: &Identity, blob: &[u8]) -> Result<Decrypted, CryptoErr

418420 .map_err(|_| CryptoError::BadSignature)?;

419421

420422 let plaintext = decompress(compressed)?;

423+ // `inner` and `signed` hold the decrypted sender identity and message

424+ // content; zeroize them explicitly on the success path too, not just on

425+ // errors (the `Zeroizing` wrapper handles drop, but being explicit here

426+ // keeps the success and error paths symmetric).

427+ inner.zeroize();

428+ signed.zeroize();

421429 Ok(Decrypted {

422430 plaintext,

423431 sender_hex: hex::encode(sender_pub),

core/src/protocol.rs+5−1

@@ -115,7 +115,11 @@ pub fn parse_payload(bytes: &[u8]) -> Option<ParsedPayload> {

115115 .iter()

116116 .filter_map(|x| x.as_str().map(str::to_string))

117117 .collect();

118− if ack_ids.iter().any(|id| id.len() > MAX_ID_LEN) {

118+ // Re-check after filtering: a peer could pack the array with

119+ // non-string junk that passes the raw count but leaves more

120+ // real ids than the cap allows after filtering. The raw check

121+ // above is an early out; this is the authoritative one.

122+ if ack_ids.len() > MAX_ACK_IDS || ack_ids.iter().any(|id| id.len() > MAX_ID_LEN) {

119123 return None;

120124 }

121125 Payload::DeliveryAck { ack_ids }