Skip to content

security: handle unhandled IndexError exceptions (DoS) in cryptographic classes#1386

Open
renich wants to merge 2 commits into
amberframework:masterfrom
renich:sentinel-fix-dos-crypto-14242777941268977919
Open

security: handle unhandled IndexError exceptions (DoS) in cryptographic classes#1386
renich wants to merge 2 commits into
amberframework:masterfrom
renich:sentinel-fix-dos-crypto-14242777941268977919

Conversation

@renich

@renich renich commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Checks payload and signature sizes before array slicing and parsing in MessageVerifier and MessageEncryptor, preventing unhandled IndexError DoS vectors.

Co-developed-by: Gemini AI renich+gemini@woralelandia.com
Signed-off-by: Rénich Bon Ćirić renich@woralelandia.com

Added bounds and length validation in `Amber::Support::MessageVerifier` and `Amber::Support::MessageEncryptor` to prevent unhandled `IndexError` exceptions when processing malformed input.

Co-authored-by: renich <225115+renich@users.noreply.github.com>
@crimson-knight

Copy link
Copy Markdown
Member

We'll accept this as defense-in-depth hardening — the code change is correct and safe — but the framing needs a correction and it needs tests. ✅ (with changes)

What I verified:

  • The exceptions are real at the unit level. On master, MessageVerifier destructures signed_message.split("--") into two vars (raises IndexError when there's no --), and MessageEncryptor#verify_and_decrypt / #decrypt slice the payload without bounds checks (raises IndexError / ArgumentError: Negative count on undersized input). Your guards (size checks + split("--", 2) with a parts.size == 2 check, raising typed InvalidSignature/InvalidMessage) are correct and introduce no auth-bypass — invalid input still resolves to "not authenticated," and valid round-trips still succeed.

  • However, this is not a DoS / "crashes the worker." Every attacker-reachable caller already swallows the exception: SignedStore#verify and EncryptedStore#verify_and_decrypt both rescue to "", and the top-level Pipe::Error rescues anything else and returns a 500 with the fiber surviving. The only unguarded caller is FileEncryptor.read, which is local server-operator usage, not attacker input. So today the worst case is a swallowed empty value, never a crash.

Requested changes before merge:

  1. Add specs for malformed input on both classes: no-delimiter / truncated / undersized payloads should raise the typed Invalid* exceptions, and valid round-trips should still pass.
  2. Correct the severity language in the PR description (and the .jules/sentinel.md note) — this is graceful-rejection hardening, not a worker-crash DoS fix. The note also has a literal unexpanded ## $(date +%Y-%m-%d) header; please drop in the real date.

Keeping the .jules note itself is fine with us. With the specs added and the wording corrected, we're glad to merge.

…dening

Add unit specs for MessageVerifier and MessageEncryptor covering malformed,
truncated, and undersized payloads. Correct date and severity description
in the PR files and agent notes to reflect graceful-rejection hardening
instead of a worker DoS crash.

Co-developed-by: Gemini AI <renich+gemini@woralelandia.com>
Signed-off-by: Rénich Bon Ćirić <renich@woralelandia.com>
@renich

renich commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

I have updated the PR to address your feedback.

Following your suggestions:

  1. I corrected the severity language in the description and agent notes to accurately specify that this is graceful-rejection hardening (avoiding internal IndexError propagation) rather than a worker DoS crash. I also dropped in the actual date 2026-06-16 instead of the unexpanded header string.
  2. I added unit specs under spec/amber/support/message_encryptor_spec.cr and spec/amber/support/message_verifier_spec.cr that test both valid round-trips and malformed, truncated, and undersized payloads (asserting that the expected InvalidMessage and InvalidSignature exceptions are correctly raised).

All specs compile and pass.

(Note: This contribution was co-developed with Gemini AI. Rénich has directed, reviewed, tested, and takes full responsibility for this code.)

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