Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #526
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #526
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wh_Client_AesGcm returns WH_ERROR_BADARGS when the request exceeds WOLFHSM_CFG_COMM_DATA_LEN. That is a hard error, so it propagates out of wc_AesGcmDecrypt as a decrypt failure, and TLS reads a decrypt failure as a corrupt record. Measured on a TC4Dx demo with COMM_DATA_LEN 8192, a record carrying more than about 8.1 KiB of plaintext gets bad_record_mac and the connection is torn down. RFC 8446 permits 2^14, so any peer sending a full size record kills the session. wh_Client_AesCbcRequest, wh_Client_AesCtrRequest and wh_Client_AesEcbRequest share the defect and fail the same way for any payload larger than the transport can carry. Report CRYPTOCB_UNAVAILABLE for that case instead, so wolfCrypt falls back to software. Rather than re-deriving each request's trailing-data size in the cryptocb, which would put a second copy of the wire format in another translation unit, the four request builders now return a distinguishable WH_ERROR_REQUEST_SIZE and the cryptocb maps that one code. The size stays computed exactly once, next to the layout it describes, so adding a field to a request struct cannot leave the two disagreeing. That check already sits ahead of every memcpy into the comm buffer and ahead of the send, so the error carries no side effects and falling back after it is safe. Nothing extra is exposed for a client-side key, because the non-DMA path already sends the key in the request, so it is on the client either way. That reasoning does not hold for a key the client named by id. The server overrides the inline key with the cached one whenever the key id is not erased, so a request that never leaves the client also never reaches wh_Server_KeystoreReadKeyEnforce. Falling back there breaks a key that has no local material at all, and makes usage policy enforcement a function of payload size: a key cached without WH_NVM_FLAGS_USAGE_ENCRYPT refuses one block and encrypts a full buffer. Gate the fallback on WH_KEYID_ISERASED so a bound key id keeps the hard error. Records from 8.2 KiB to the 16384 byte maximum now complete instead of dropping the connection. whTest_CryptoAesCommBuffer covers both halves: the client-key fallback matches a software reference byte for byte, and the oversized server-key request stays an error even with the key installed locally, where a fallback would otherwise succeed silently. Every mode gets its own oversized round-trip, because the four size computations are derived separately and a wrong term in any of them would otherwise only show up as a fallback that never fires. The server-key half holds for every mode, and every mode asserts it, but the test is only sensitive to the CBC and ECB checks: dropping WH_KEYID_ISERASED from either makes the suite fail. It cannot isolate the CTR or the GCM one, because both re-dispatch the whole buffer through the ECB callback, whose own check refuses a key-ID key just the same - software CTR by construction, and software GCM because wc_AesGcmEncrypt encrypts its counter blocks with a single wc_AesEcbEncrypt call whenever HAVE_AES_ECB is defined, which this suite does define. Dropping the GCM check alone leaves the oversized server-key request failing with WH_ERROR_REQUEST_SIZE all the same; dropping the GCM and the ECB check together is what makes the GCM leg fire. So those two legs pin the contract - an oversized request on a cached key must not silently fall back to software - rather than one particular check. The guarantee is enforced twice over there rather than missing; only the attribution to a single check is lost.
The DMA AES-GCM request already sends the IV, the auth tag and the key as trailing data inside the packet, but passed the AAD as a DmaBuffer - an address the server has to translate, map and release for what is usually a handful of bytes. Send it inline when it is small enough, and keep the DMA path for anything larger. This is not only about saving a round of address handling. The AAD is frequently built somewhere the server cannot reach at all: wolfSSL assembles the TLS 1.3 additional data in a stack local, and on targets where task stacks are outside the address range the server can address, that disqualified the entire request - payload buffers included - and dropped the whole operation back to software. Measured on an AURIX TC4Dx with the CSS engine behind wolfHSM, that was three quarters of all crypto: the port's own self-test dispatch goes from 172 DMA / 523 comm-buffer to 692 / 3, and TLS record decryption stops falling back to software entirely. A request with aad.addr == 0 and aad.sz > 0 carries the AAD immediately after the key. The server accounts for it in the expected request size, reads it from the packet, and skips both the address translation and the release. WOLFHSM_CFG_DMA_INLINE_AAD_MAX_SIZE bounds how much the request will carry, defaulting to 128 bytes - ample for a TLS 1.2 or 1.3 record header and for the AUTOSAR and CAN headers that motivate this, while leaving the rest of WOLFHSM_CFG_COMM_DATA_LEN alone. Anything larger goes over DMA exactly as before, as does everything when the knob is set to 0. It is a client-side policy only: the server reads whatever the request carries and never consults the value, so a client and a server built with different settings still interoperate. The server cannot lean on that knob, and must not lean on aad.sz either. It arrives as a client-supplied uint64_t, and the expected-size check is an exact equality, so an unbounded AAD term in that sum could be chosen to wrap it back onto the received size - passing validation with an arbitrary ivSz and handing GHASH a length that walks gigabytes past the message. Bound it before the sum is formed: an inline AAD cannot exceed the request carrying it, and no AAD may exceed what wc_AesGcmEncrypt can hash. The size check, the DMA translation and the wc_AesGcm call then all use that one validated length rather than validating one and truncating another. The client always builds a self-consistent frame, so that bound can only be reached by a hand-built packet. whTest_CryptoReqSize drives the handler directly with five: an inline AAD the message is too short to hold, one declared larger than the whole message, one whose low 32 bits match the frame exactly, one sized so the 64-bit sum wraps back onto the received size, and a correctly framed control that must not be rejected. The third is what pins the bound rather than the pre-existing equality check: truncated to uint32_t it matches the frame, so only a test made before that cast rejects it. Both sides locate that AAD from the key size the CLIENT put on the wire, not from the resolved key length. They differ for an HSM-side key: the wire carries keySz 0 while the server replaces keyLen with the keystore key's own length, and computing the offset from that reads the AAD past where the client wrote it - encrypt then tags over the wrong bytes and only the decrypt fails, as AES_GCM_AUTH_E. Mixed versions fail closed rather than silently. An old server computes the expected request size without the inline AAD, so the length check rejects the request with WH_ERROR_BADARGS; an old client always sends a real address and is unaffected. Only the inline case is new on the wire, and only in the client-to-new-server direction. Inlining also takes the AAD out of the DMA callbacks and the client-side allowlist, so wh_Client_AesGcmDmaRequest and wh_Client_AesGcmDmaResponse now document that the AAD is address-translated and POST-cleaned only when it is large enough to travel over DMA. The DMA form of the AAD had no coverage before - every AAD in the suite fits the comm buffer, so all of them would now take the inline path and the DMA branch would never run again. The AES-GCM DMA async round-trip therefore runs twice, at the inline cap and one byte past it. Setting the knob to 0 leaves only the DMA form, and the test says so in its output rather than reporting a pass that covered half of what it claims. A round-trip alone would not have been enough. Encrypt and decrypt carry the same AAD, so an AAD the server reads at the wrong offset or length still verifies against itself and the test passes. Each leg therefore compares its ciphertext and tag against a software reference computed on INVALID_DEVID. An off-by-one in the DMA AAD length fails the tag comparison on the above-cap leg; without it the suite stays green.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #526
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
bigbrett
left a comment
bigbrett
left a comment
There was a problem hiding this comment.
Love:
- The new "data too large for transport" error code
Reservations, but could go either way on:
- Making the AES GCM DMA API variant allow for small AAD to be passed inline
I actually do like this feature, however is size always the correct metric here for determining inline vs DMA AAD? I wonder if we should make this explicit for the user. When invoked under the hood of wolfCrypt then yes, we will need to make an opinionated decision since there is no good way to pass supplementary info other than through the Client context DMA mode state. Perhaps there are multiple DMA modes instead of just on/off? I'm fine with it as-is but just brainstorming
Problems:
- Silent SW fallback on the client side.
This behavior is incorrect based on the usage model of wolfHSM.
We should never silently fall back to software for a supported algorithm just because the transport is not large enough to handle the data. HSM offload is explicit based on a caller-supplied devId in the context. If the caller says "I want this operation offloaded to the HSM" and we can't offload to the HSM for some reason, then the operation must fail. I think the case you introduced where the key is local to the client in RAM anyway makes this less damaging, however I don't want users to have to try and reverse engineer where the crypto is happening.
Currently the water is a bit muddy on this for compound operations (e.g. algo A isnt supported for offload but it uses algo B internally so in this case we let algo A run in software with algo B offloaded inside) but I hate that too and the real solution is simply to support all those algorithms. I'm sure we missed a few edge cases in the current library but I want to flag it here so we don't add additional cases of this happening.
I know you are doing this to support offload of client-side TLS. Could wolfSSL instead just chose to do this crypto locally in this case, instead of trying to have wolfHSM silently fallback to local software crypto if arbitrary conditions are met? I think the burden needs to be on the caller here to explicitly route their crypto calls properly.
|
@Frauschi spent a little more time diving into this after the review and it seems there are more places than I thought where wolfHSM through the wolfCrypt API can silently fall back to software. I think we need to fix this, however it is admittedly a bit beyond the scope of this PR. Do you think we could still prevent the silent fallback here, and I can simultaneously see if there is a uniform solution across the codebase to prevent silent fallbacks going forward? |
4 participants
Summary
Two independent fixes to the AES client paths, both driven by the same failure mode on an AURIX TC4Dx port: a request that will not fit the comm buffer, or an AAD the server cannot address, drops the whole operation back to software.
client: fall back rather than fail when AES will not fit the comm buffer- an AES request larger thanWOLFHSM_CFG_COMM_DATA_LENreturnedWH_ERROR_BADARGS, which propagates out ofwc_AesGcmDecryptas a decrypt failure and reads to TLS as a corrupt record. WithCOMM_DATA_LEN8192 a record above ~8.1 KiB getsbad_record_macand the connection is torn down; RFC 8446 permits 2^14, so any peer sending a full-size record kills the session. The cryptocb now reportsCRYPTOCB_UNAVAILABLEso wolfCrypt falls back to software.Carry a small AES-GCM AAD in the request instead of over DMA- the DMA AES-GCM request already sends the IV, tag and key inline but passed the AAD as aDmaBuffer. wolfSSL builds the TLS 1.3 additional data in a stack local, and where task stacks are outside the range the server can address, that disqualified the entire request, payload buffers included.Design notes
The fallback is gated on
WH_KEYID_ISERASED. For a key the client named by id, the server overrides the inline key with the cached one, so a request that never leaves the client never reacheswh_Server_KeystoreReadKeyEnforce. Falling back there would break a key with no local material and make usage-policy enforcement a function of payload size. A bound key id keeps the hard error.The size is computed once. Rather than re-deriving each request's trailing-data layout in the cryptocb, the four AES request builders return a distinguishable
WH_ERROR_REQUEST_SIZE(new,-2012) and the cryptocb maps that single code. The check already sits ahead of everymemcpyinto the comm buffer and ahead of the send, so the error carries no side effects and falling back after it is safe. This keeps the wire format in one place.Inline AAD is a client-side policy.
WOLFHSM_CFG_DMA_INLINE_AAD_MAX_SIZE(default 128,0disables) bounds what the client will inline. The server reads whatever the request carries and never consults the value, so client and server may be built with different ones. Inline AAD does require a server new enough to understand the encoding; an older one rejects such a request withWH_ERROR_BADARGSrather than misreading it.The server bounds the declared AAD length before using it.
aad.szarrives as a client-supplieduint64_tand the expected-size check is an exact equality, so an unbounded AAD term in that sum could be chosen to wrap it back onto the received size, passing validation with an arbitraryivSz. It is bounded before the sum is formed, and one validated length then drives the size check, the DMA translation and thewc_AesGcmcall alike.Testing
Full suite passes. New and extended coverage:
whTest_CryptoReqSize(new file) drives_HandleAesGcmDmadirectly with five hand-built frames, since the client always builds a self-consistent one. The case that pins the new bound declares a length whose low 32 bits match the frame exactly - truncated touint32_tit matches, so only a check made before that cast rejects it.whTest_CryptoAesCommBuffernow covers all four modes rather than CBC alone, each with an oversized round-trip against a software reference and an oversized server-key request that must stay a hard error.whTest_CryptoAesDmaAsyncruns the GCM round-trip twice, at the inline cap and one byte past it, so both wire forms of the AAD are exercised; setting the knob to0leaves only the DMA form and the test says so in its output. Each leg compares its ciphertext and tag against a software reference rather than against itself: encrypt and decrypt carry the same AAD, so an AAD read at the wrong offset or length still round-trips cleanly. An off-by-one in the DMA AAD length fails the above-cap leg's tag comparison.Each new guard was verified by breaking it and re-running, not only by passing.
Known gaps
The server-key half of the size guard is asserted for all four modes, but only CBC and ECB are attributable to their own check: dropping
WH_KEYID_ISERASEDfrom either fails the suite. CTR and GCM both re-dispatch the whole buffer through the ECB callback - software CTR by construction, and software GCM becausewc_AesGcmEncryptencrypts its counter blocks with onewc_AesEcbEncryptcall wheneverHAVE_AES_ECBis defined (wolfcrypt/src/aes.c:11275), which this suite does. Dropping the GCM check alone still leaves the oversized server-key request failing withWH_ERROR_REQUEST_SIZE; dropping GCM's and ECB's together is what makes the GCM leg fire. So those two legs pin the contract - an oversized request on a cached key must not silently fall back to software - rather than one particular check. The guarantee is enforced twice there rather than missing; only the attribution is lost.The same
req_len > WOLFHSM_CFG_COMM_DATA_LEN->WH_ERROR_BADARGSpattern appears at ~23 other sites across the client. Only the four AES non-DMA builders were converted here, to keep the error-contract change reviewable; the rest are a candidate follow-up.