-
Notifications
You must be signed in to change notification settings - Fork 49
fix: ensure key material is cleared in error paths #1443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be removed.
| defer func() { | ||
| if output.Plaintext != nil { | ||
| clear(output.Plaintext) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a nil check. Per the language spec clear is a no-op if the slice is nil.
| defer func() { | |
| if output.Plaintext != nil { | |
| clear(output.Plaintext) | |
| } | |
| }() | |
| defer clear(output.Plaintext) |
| defer func() { | ||
| if resp.Plaintext != nil { | ||
| clear(resp.Plaintext) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a nil check. Per the language spec clear is a no-op if the slice is nil.
| defer func() { | |
| if resp.Plaintext != nil { | |
| clear(resp.Plaintext) | |
| } | |
| }() | |
| defer clear(resp.Plaintext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this file.
This commit fixes security vulnerabilities where sensitive key material could remain in memory after errors: 1. envelope.go: Fix decryptRow to clear rawDrk buffer even on error - Previously, early return on error skipped the defer statement - Now uses defer with nil check to ensure cleanup 2. AWS KMS plugins: Clear plaintext keys on decrypt errors - Both v1 and v2 implementations now clear resp.Plaintext/output.Plaintext - Uses closure pattern to ensure cleanup even when continuing to next region - Prevents key material from failed regions remaining in memory These changes are critical for security as they prevent sensitive cryptographic material from persisting in memory after failures, reducing the attack surface for memory disclosure vulnerabilities. All changes use Go's built-in clear() function for secure memory wiping.
- Remove extra whitespace that was causing gci formatting errors - Apply gofumpt formatting to ensure consistent code style
7e1fe86 to
24c4f2b
Compare
Summary
clear()functionThe Problem: Key Material Not Being Wiped
1. envelope.go - Memory Leak in decryptRow
Before (VULNERABLE):
Why it's vulnerable: When
crypto.Decryptfails, the function returns immediately. Thedeferstatement is never reached, sorawDrk(which may contain partial key material from failed decryption) is NEVER cleared from memory.After (SECURE):
How it's fixed: The
deferis now placed immediately after we haverawDrk, ensuring it will execute regardless of any subsequent errors.2. AWS KMS - Multi-Region Retry Memory Accumulation
Before (VULNERABLE):
Why it's vulnerable: In a multi-region setup, if region 1 succeeds in KMS decryption but fails in the subsequent crypto operation,
resp.Plaintextcontains the master key but is never cleared. Then we try region 2, potentially accumulating multiple plaintext keys in memory.After (SECURE):
How it's fixed: Each iteration now uses a closure with defer to ensure
resp.Plaintextis cleared before trying the next region. This prevents accumulation of key material across retry attempts.Security Impact
What Could Happen Before These Changes:
What Happens After These Changes:
clear()), ensures proper memory wipingTesting
defer func() { clear(buffer) }()is guaranteed to executeRelated PRs
clear()functionTogether, these PRs fix critical security vulnerabilities in the Asherah Go implementation's memory management.