Skip to content

Zero stack buffers in keywrap and datawrap handlers#343

Closed
padelsbach wants to merge 1 commit intowolfSSL:mainfrom
padelsbach:keywrap-datawrap-zeroization
Closed

Zero stack buffers in keywrap and datawrap handlers#343
padelsbach wants to merge 1 commit intowolfSSL:mainfrom
padelsbach:keywrap-datawrap-zeroization

Conversation

@padelsbach
Copy link
Copy Markdown
Contributor

@padelsbach padelsbach commented Apr 22, 2026

Ensure sensitive buffers are zeroed when returning from _HandleKeyWrapRequest, _HandleKeyUnwrapAndExportRequest, and _HandleDataWrapRequest

Fixes F-2297 and F-2303

@padelsbach padelsbach marked this pull request as ready for review April 22, 2026 19:56
Copy link
Copy Markdown

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #343

Scan targets checked: wolfhsm-core-bugs, wolfhsm-src

No new issues found in the changed files. ✅

Copy link
Copy Markdown
Contributor

@bigbrett bigbrett left a comment

Choose a reason for hiding this comment

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

One nit to change the force zero API to use the wolfHSM native one.

That said, I'm somewhat unconvinced on this PR - I think the fenrir findings are false positive and aren't really necessary because this is server code and by definition we are running in a secure environment with only trusted access to RAM. So unless there was a buffer overread that somehow was able to exfiltrate that data back to the client, this fixes an essentially unexploitable path. Otherwise, we would be force zeroing pretty much any time any secret or any plaintext is present. I suppose I can leave it in as it is relatively innocuous, however I'm tempted not to, since we don't apply this scheme in a meaningful manner across the codebase. I'd almost not rather have force zero applied in an ad-hoc manner until we come up with a coherent policy of what must be zeroed and what must not.

TLDR Unless there is a real exfiltration path, it seems like noise. Until we produce a coherent security policy that can be uniformly enforced across the entire code base, I'm tempted to skip these findings (again, unless there is a REAL exfiltration path)

Comment thread src/wh_server_keystore.c
}

return WH_ERROR_OK;
wc_ForceZero(data, sizeof(data));
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.

we should be using wh_Utils_ForceZero() rather than the wolfCrypt API directly.

@bigbrett bigbrett assigned padelsbach and unassigned bigbrett May 1, 2026
@padelsbach
Copy link
Copy Markdown
Contributor Author

Seemed like a nice defensive enhancement if we are not sensitive to cycle count, but I see your point. Let's revisit if/when we have a policy in place. Closing.

@padelsbach padelsbach closed this May 1, 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.

3 participants