Attestation state decrypt: missing length check and unauthenticated encryption #3600

Problem

I found two issues in decrypt in app/controlplane/pkg/biz/attestationstate.go while reviewing #3594. They existed before that PR, and the PR does not change them.

1. Missing length check before the magic prefix check

decrypt checks that the input is longer than the salt and that what follows is at least one AES block (the IV). It does not check that the remaining ciphertext is at least len(magic) bytes before it does this:

if string(ciphertext[:len(magic)]) != magic {

A stored value of 32 to 44 bytes (16 salt + 16 IV + 0..12 bytes) passes the earlier checks and then panics with a slice-out-of-range error.

encrypt always prepends the magic prefix, so values that encrypt writes cannot trigger this. Only a truncated or corrupted row in the database can. The impact is low, but decrypt should return an error, not panic.

2. The encryption is not authenticated

The state is encrypted with AES-256-CFB, with a key derived by PBKDF2 from the request token. There is no MAC or AEAD tag. The MagicPrefix__ check only detects a wrong passphrase. It does not protect integrity: CFB is malleable, so a change to the stored ciphertext can produce changed plaintext that still passes the magic check.

The code comment says the only purpose is encryption at rest in the database. So this matters only to someone who can already write to the database. But nothing detects tampering with the stored state.

Proposal

• Add a length check (len(ciphertext) < len(magic) returns an error) before the magic comparison, with a unit test for short inputs.
• Think about moving to an AEAD such as AES-GCM for new writes. The tag gives integrity and also replaces the magic prefix as the wrong-passphrase signal. Existing rows need a migration path, for example a version byte, or a fallback to CFB on read until the stored states expire or are reset.

🤖 Posted by Maximus bot (Claude Code) on behalf of @migmartri