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

92e93f0iko3 months ago

PROTOCOL-REVIEW.md+5−3

compare with master

unifiedsplit

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

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

335

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

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

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

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

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

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

339−decryption or storage, and blocking is bidirectional.

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

292

293### Blocking

294

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.

296

297---

298

core/src/app.rs+45−17

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

845 }

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

847 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)

848 } else {

860 } else {

849 render()

861 render()

850 }

862 }

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

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

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

1529

1541

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

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

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

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

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

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

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

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

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

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

1544 );

1555 );

1545 trim_history(convo);

1556 trim_history(convo);

1546

1557

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

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

1548− model

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

1549− .contacts

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

1550− .entry(peer.clone())

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

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

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

1571+ }

1552

1572

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

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

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

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

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

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

1561− if first_to_peer {

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

1577+ if was_new {

1562 let profile = Payload::Profile {

1578 let profile = Payload::Profile {

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

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

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

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

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

1570 ));

1586 ));

1571 }

1587 }

1572

1588

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+

1573 Command::all([

1595 Command::all([

1574 self.persist_messages(model, &peer),

1596 self.persist_messages(model, &peer),

1575 self.persist_contacts(model),

1597 self.persist_contacts(model),

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

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

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

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

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

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

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

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

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)

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

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

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

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

1858 match parsed.payload {

1886 match parsed.payload {

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

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

core/src/crypto.rs+13−5

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

312 let target = padded_len(unpadded_blob_len);

313 let pad_len = target - unpadded_blob_len;

314

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

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

316 rng.fill_bytes(&mut padding);

317

318 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

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

382

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

384− let mut inner = cipher

384+ let mut inner = Zeroizing::new(

385+ cipher

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

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

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

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

388+ );

387

389

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

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

389 inner.zeroize();

391 inner.zeroize();

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

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

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

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

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

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

414 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)));

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

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

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

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

416 verifying

418 verifying

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

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

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

419

421

420 let plaintext = decompress(compressed)?;

422 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();

421 Ok(Decrypted {

429 Ok(Decrypted {

422 plaintext,

430 plaintext,

423 sender_hex: hex::encode(sender_pub),

431 sender_hex: hex::encode(sender_pub),

core/src/protocol.rs+5−1

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

115 .iter()

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

117 .collect();

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.

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

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

119 return None;

123 return None;

120 }

124 }

121 Payload::DeliveryAck { ack_ids }

125 Payload::DeliveryAck { ack_ids }