wolfHSM Hardening Fixes

[aidangarske]

F-4235, F-4329, F-4978, F-5139, F-5141, F-5142, F-5473, F-5474, F-5475, F-6029,
F-6030, F-6031, F-6207

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #425

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

No new issues found in the changed files. ✅

[bigbrett]

I'm medium on these force zero changes. In all of our currently supported transports, the transport buffer itself has to be trusted or the whole threat model falls apart, and the client's local comm buffers even more so. Zeroizing these here in light of these assumptions is overkill IMO and would be a bit of a performance hit. I think we need to have a uniform policy about when we require force zeroing of shared memory and apply it across the entire code base at once. Perhaps a build time configuration that would enable "strict" force zeroing where the server just clears the entirety of the comm buffers once received? But again, pretty sure this is overkill. @billphipps @rizlik thoughts?

[aidangarske]

I'm medium on these force zero changes. In all of our currently supported transports, the transport buffer itself has to be trusted or the whole threat model falls apart, and the client's local comm buffers even more so. Zeroizing these here in light of these assumptions is overkill IMO and would be a bit of a performance hit. I think we need to have a uniform policy about when we require force zeroing of shared memory and apply it across the entire code base at once. Perhaps a build time configuration that would enable "strict" force zeroing where the server just clears the entirety of the comm buffers once received? But again, pretty sure this is overkill. @billphipps @rizlik thoughts?

@bigbrett, I am fine with doing it however you want seems like there is a lot of tradeoffs for each. Currently wolfssl does it like this but if we want to make a better design for wolfHSM that sounds good! Ill leave it for now till we decide

[Frauschi]

Besides the open discussion on the ForceZero policy for CommBuf data, the other changes LGTM.

(Only to give my two cents to the ForceZero discussion: I'm with @bigbrett on this. Have them in, but behind some kind of feature flag.)

[rizlik]

I'm medium on these force zero changes. In all of our currently supported transports, the transport buffer itself has to be trusted or the whole threat model falls apart, and the client's local comm buffers even more so. Zeroizing these here in light of these assumptions is overkill IMO and would be a bit of a performance hit. I think we need to have a uniform policy about when we require force zeroing of shared memory and apply it across the entire code base at once. Perhaps a build time configuration that would enable "strict" force zeroing where the server just clears the entirety of the comm buffers once received? But again, pretty sure this is overkill. @billphipps @rizlik thoughts?

@bigbrett I do think zeroing is worth even if the comm buffer is considered trusted, being zeroing more a defense in depth against possible client co-resident threats.

But yes having an performance opt-out guard is also a nice to have but more importantly I second the idea of doing that in a single place, probably when the client receives the response.
The problem I see here is that as this PR does the zeroing there is an implicit assumption that the transport cb send is done with the buffer the moment it returns.
If this is something we want to lock in the transport API, we should add it in the docs, otherwise, zeroing must happen at response time.

regarding zeroing at the server: this adds the requirement for a server to be able to write on the request buffer, while until now it can be mapped read-only.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[aidangarske]

@bigbrett @rizlik, I added a WOLFHSM_CFG_DISABLE_CLIENT_COMM_ZEROIZE macro that is a specific performance opt out macro for the zeroization changes and has client communication-buffer zeroization by default. The cleanup is now centralized and preserve the in place response and DMA handling until parsing completes then the time sensitive fields are cleared . Let me know if this is what you guys where thinking