Skip to content

crypto: make Session::decrypt fail closed and overflow-safe - #1432

Closed
zgiles wants to merge 2 commits into
mobile-shell:masterfrom
zgiles:harden/crypto-decrypt-failclosed
Closed

zgiles wants to merge 2 commits into
mobile-shell:masterfrom
zgiles:harden/crypto-decrypt-failclosed

Conversation

@zgiles

@zgiles zgiles commented Oct 22, 2026

Copy link
Copy Markdown

decrypt() narrowed size_t to int, guarded buffer bounds only with assert(), and called exit(1) on a negative length. It now rejects bad lengths with a CryptoException and uses size_t arithmetic throughout. No change on the network path.

Test: decrypt-bounds.

zgiles added 2 commits July 22, 2026 18:27
decrypt() computed body_len/pt_len as int from a size_t length, guarded the
receive-buffer bounds with assert() (compiled out under -DNDEBUG), and killed
the process with exit(1) if pt_len went negative. The invariant it depends on
-- pt_len must never equal ae_decrypt()'s AE_INVALID (-1) sentinel, or a
forged packet would be accepted -- held only by luck of the caller.

Rework:
  - reject len < 24 (throws, not exit()),
  - compute body_len/pt_len in size_t (no narrowing, no overflow),
  - reject body_len/pt_len that would not fit the fixed receive buffers
    (bounding by ciphertext_buffer/plaintext_buffer, which is the real
    constraint: encrypt() can legitimately produce up to RECEIVE_MTU + 8
    bytes for a maximum-size message).

len >= 24 guarantees pt_len >= 0, so it never collides with AE_INVALID. No
behavior change on the network path; decrypt() is now safe for any caller and
never exit()s.
Checks that decrypt() rejects out-of-range lengths (0, 1, 23, and beyond the
receive buffer) with a CryptoException rather than an abort/exit, that a
tampered ciphertext fails the integrity check (the property the pt_len >= 0
invariant protects), and that minimum-length (24-byte, empty payload) and
normal round-trips both succeed.
@achernya achernya closed this Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants