Skip to content

fix(controlplane): return error on short attestation state ciphertext - #3615

Merged
migmartri merged 2 commits into
chainloop-dev:mainfrom
beiryu:fix/3600-attestation-state-decrypt-missing-length
Oct 15, 2026
Merged

migmartri merged 2 commits into
chainloop-dev:mainfrom
beiryu:fix/3600-attestation-state-decrypt-missing-length

Conversation

@beiryu

@beiryu beiryu commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #3600

decrypt in app/controlplane/pkg/biz/attestationstate.go checked the salt and IV lengths but not the length of the remaining data before comparing it with the magic prefix. A stored value of 32 to 44 bytes passed the earlier checks and then caused a slice-out-of-range panic.

This change adds a len(ciphertext) < len(magic) check before the comparison, so a truncated or corrupted value now returns an error.

This does not cover the second point in the issue (moving to an authenticated cipher such as AES-GCM). That needs a format change and a migration path for existing rows, so it is left for a separate change.

Testing

  • Added TestDecryptShortInput in attestationstate_test.go. It calls decrypt with inputs of salt + IV only, one byte after the IV, and one byte short of the magic prefix. It checks that there is no panic and that an error is returned.
  • Added TestEncryptDecryptRoundTrip to check that normal encrypt and decrypt still work.
  • The new tests and the project checks pass.

The decrypt function could panic on a truncated database value. I wanted it to return an error, so one bad row does not crash the request.

View guided diff

@chainloop-platform

chainloop-platform Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

PR validation — ✅ 3 passing

Status Policy Material Messages
✅ Passed pr-user-story-linked pr-info -
✅ Passed pr-min-approvals pr-info -
✅ Passed pr-description-required pr-info -

View attestation ↗

AI Session Checks — ⏭️ bypassed by label

AI Coding Session Check Bypassed

This PR carries the skip-ai-session label, so the AI coding session check was bypassed.

Learn more about Chainloop Trace.

Security Checks — ✅ 5 passing

✅ secret-scan

Status Policy Messages
✅ Passed secrets-detection -

✅ sast-scan

Status Policy Messages
✅ Passed owasp-top10-2025 -
✅ Passed sast -
✅ Passed cwe-top25 -
✅ Passed cwe-top26-40-cusp -
Scans not applied (3)
Scan Reason
vulnerability-scan no manifest/lockfile changed
github-actions-scan no workflow files changed
iac-scan no IaC files changed

View attestation ↗

Security context

✅ Nothing this change touches has a recorded security-fix history.

View in Chainloop ↗ · How this works ↗


Powered by Chainloop and Chainloop Trace

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

View guided diff | Re-trigger cubic

@migmartri

Copy link
Copy Markdown
Member

Thanks for the contribution, could you please sign make sure the commits are signed?

@beiryu
beiryu force-pushed the fix/3600-attestation-state-decrypt-missing-length branch from b3705b1 to 23c286c Compare October 14, 2026 09:47
@beiryu

beiryu commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! The commit is now signed (SSH signature, shows as Verified). The content and the DCO sign-off are unchanged.

migmartri
migmartri previously approved these changes Oct 14, 2026
@migmartri

Copy link
Copy Markdown
Member

Sorry, there seems to be some conflicts, could you resolve them please, thanks!

Fixes chainloop-dev#3600

Signed-off-by: Khanh Dinh Nguyen <77538987+beiryu@users.noreply.github.com>
@beiryu
beiryu force-pushed the fix/3600-attestation-state-decrypt-missing-length branch from 23c286c to 4fda84e Compare October 15, 2026 00:13
migmartri
migmartri previously approved these changes Oct 15, 2026
A row too short to hold the magic prefix is a truncated or tampered value; report it like any other value that does not decrypt to the prefix, as the tampered-row integration test expects.

Signed-off-by: Khanh Dinh Nguyen <77538987+beiryu@users.noreply.github.com>
@beiryu

beiryu commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

@migmartri sorry, I had to push one more commit after your approval (b30a8ac), so the approval was dismissed.

Test (controlplane) failed in TestAttestationStateUseCase/TestRead: the "tampered with" case stores 41 bytes, which leaves 9 bytes after the salt and IV. That is shorter than the magic prefix. On main, ciphertext[:len(magic)] reads into the slice's spare capacity, so the case ended up as ErrInvalidPassphrase. My new ciphertext too short error changed that.

The commit now returns NewErrInvalidPassphrase() for the short case, so a truncated or tampered row is reported the same way as before (no panic), and TestDecryptShortInput asserts that. TestAttestationStateUseCase passes locally against Postgres. Could you take another look when you have a moment?

@migmartri
migmartri enabled auto-merge (squash) October 15, 2026 09:39
@migmartri
migmartri merged commit 10ee64a into chainloop-dev:main Oct 15, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attestation state decrypt: missing length check and unauthenticated encryption

2 participants