Standalone SPDM 1.2/1.3/1.4 requester#6
Open
aidangarske wants to merge 9 commits into
Open
Conversation
aidangarske
changed the title
aidangarske
force-pushed
the
cleanup-and-skoll-hardening
branch
from
53cd346 to
f216e73
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Strips Nuvoton TPM mode and WOLFSPDM_MODE so wolfSPDM is a pure
DSP0274/DSP0277 standard requester. Adds the missing spdm-emu
integration (new examples/spdm_demo CLI + 18-test matrix driver) and
rewrites the broken spdm-emu CI workflow. Then applies a deep skoll
review-security pass on the diff and the full repo.
Security:
- KEY_EXCHANGE_RSP responder signature verified before key derivation
- ResponderVerifyData HMAC mismatch fatal; constant-time compare
- Seq-number wrap cap + explicit replay mismatch check (DSP0277 11.3)
- Strict Algorithm Set B enforcement in ParseAlgorithms
- Mutual-auth requested by responder refused before sessionId commit
- 1.3+ RequesterContext echo verified in CHALLENGE_AUTH
- Inner MCTP-type byte validated post-decrypt
- Encrypt/DecryptInternal use goto-exit + wc_ForceZero on plaintext
- wc_ForceZero of transient secrets in DeriveAppDataKeys/UpdatedKeys
- Disconnect frees responderPubKey + clears measurement state
- ConnectStandard resets stale ctx fields between retries
- Compile-time _Static_assert on WOLFSPDM_CTX_STATIC_SIZE
Standards / interop:
- SPDM 1.2/1.3/1.4 negotiation, wolfSPDM_SetMaxVersion runtime clamp
- 1.3+ RequesterContext in GET_MEASUREMENTS and CHALLENGE; 1.4
OpaqueLength in FINISH and FINISH_RSP (256B cap on incoming)
- ParseAlgorithms walks AlgStruct via ExtAsymSelCount/ExtHashSelCount
- Finish ERROR sniff uses explicit sessionId compare (no heuristic)
- ExchangeMsg snapshots transcriptLen and rolls back on failure
API:
- Renamed wolfSPDM_GetVersion_Negotiated -> wolfSPDM_GetNegotiatedVersion
(old name retained as exported ABI-compat alias)
- New: wolfSPDM_SetMaxVersion, wolfSPDM_GetLastPeerError
- Removed: SetRequesterKeyPair, SignHash, SetResponderPubKey, SetMode
- GetSessionId returns id from KEY_EX onward (I/O callback needs it
to tag the secured FINISH record)
- WOLFSPDM_API visibility shim, bit-field packed flags struct
Tests + CI:
- 53 unit tests (was 27); 18/18 integration tests pass against spdm-emu
(6 scenarios x SPDM 1.2/1.3/1.4)
- CI drops --enable-nuvoton across all matrices, adds 3-arch
spdm-emu integration job (ubuntu-22.04 x64, ubuntu-24.04 x64,
ubuntu-24.04-arm aarch64)
- examples/spdm_demo.c: cast int port to uint16_t for htons (-Wconversion). Sanitize SPDM_EMU_PATH via realpath() + length/control-char/traversal checks before using it in fopen() (CodeQL "uncontrolled data in path expression" High). - src/spdm_msg.c: move loop counter 'i' into inner scope where it is used (cppcheck style: variableScope). - test/test_spdm.c: same htons cast fix; size context buffer via WOLFSPDM_CTX_STATIC_SIZE so it tracks wolfSSL config-dependent struct growth (previously hard-coded 16384 was too small under the CI's wolfSSL build, leaving the legacy smoke test failing on startup). - test/unit_test.c: drop redundant encSz initializer (cppcheck style: redundantInitialization); value is reassigned before first read. All 53 unit tests pass; -Wall -Wextra -Wpedantic -Werror -Wconversion -Wshadow build clean.
test/test_spdm.c was the failing spdm-emu integration step: a static isSecured flag on TCP_CTX was never flipped to 1, so FINISH (which IS encrypted but runs before WOLFSPDM_STATE_CONNECTED) was sent with MCTP type 0x05 (plain) instead of 0x06 (secured). The emu rejected with ERROR 0x01. Replaced with the dynamic is_secured_spdm() pattern from examples/spdm_demo.c, which matches the first 4 tx bytes against the live session ID. Verified end-to-end against the local DMTF spdm-emu. PR review fixes: - src/spdm_secured.c: do not call wc_AesFree on an uninitialized aes when wc_AesInit fails. Bail with explicit cleanup before the exit label in both Encrypt/Decrypt paths. - examples/spdm_demo.c tcp_io_callback: loop send()/recv() (handling EINTR/short returns) via new send_all/recv_all helpers; previous code treated any short send as a fatal error. - examples/spdm_demo.c sanitize_emu_path: switch from realpath(p, NULL) (GNU extension) to POSIX realpath(p, buf[PATH_MAX]). - examples/spdm_test.sh: gate the port-in-use check on ss/netstat/lsof availability with a clear warning when none are present. - src/spdm_msg.c: replace AlgStruct magic numbers (AlgType 2/3/5 and bits 0x0010/0x0002/0x0001) with named constants SPDM_ALG_TYPE_DHE/ AEAD/KEY_SCHEDULE and SPDM_DHE_ALGO_SECP384R1/AEAD_ALGO_AES_256_GCM/ KEY_SCHEDULE_SPDM. Added the AlgType defines to wolfspdm/spdm_types.h. - docs/ATTESTATION.md: overview said SPDM 1.2 only; updated to 1.2-1.4 with a version-specific note about RequesterContext. All 53 unit tests still pass; strict build (-Wall -Wextra -Wpedantic -Werror -Wconversion -Wshadow) clean.
Parser updates per DSP0274 / DSP0277: - ParseCapabilities now extracts CTExponent, DataTransferSize, and MaxSPDMmsgSize. GetCertificate clamps the per-fragment Length to the responder's negotiated DataTransferSize. - ParseDigests stores Param1 SlotMask; ConnectStandard picks the lowest populated slot rather than always using slot 0. - ParseCertificate, ParseChallengeAuth, ParseMeasurements all check the echoed SlotID. ParseCertificate uses the shared parse-or-error helper so short ERROR responses route to PEER_ERROR. - ParseAlgorithms validates the Length field and the MeasurementSpecificationSel / OtherParamsSel echoes. - Certificate fetch loop has an iteration cap and a forward-progress guard so a peer cannot stall the requester. State and lifecycle: - Connect requires either a configured trust anchor or an explicit wolfSPDM_AllowUntrustedCerts opt-in. - New wolfSPDM_SetRequesterSessionId; Init draws a random non-reserved ReqSessionID by default. - GetMeasurements requires the encrypted channel and returns SUCCESS on the no-signature retrieval path. - Public encrypt / decrypt / secured-exchange entry points share a single state guard that requires the application-data phase. - KeyUpdate snapshots the keying material and rolls it back if the ACK fails to decrypt. - Disconnect and the Connect reset path call a shared key-wipe helper so derived material does not survive into the next attempt. Cleanup of intermediate digests, MACs, and stack key material on every exit path of ParseKeyExchangeRsp, BuildFinish, Finish, BuildSignedHash, HkdfExpandLabel, DeriveAppDataKeys, VerifyMeasurementSig, and VerifyChallengeAuthSig. Defense-in-depth bound checks on EncryptInternal plainSz, DecryptInternal cipherLen, and the HkdfExpandLabel assembly buffer. DecryptInternal also enforces strict DSP0277 Length equality so trailing bytes past the declared record boundary are rejected. Wire-format cleanups: - Default requester capabilities drop the responder-only CERT_CAP and CHAL_CAP bits. - ECDH shared-secret zero-pad walks the full curve size in a fixed loop so the memory-access pattern is independent of the X-coordinate's leading-zero count. - Removed the unused ctx->th2 field. New unit tests cover the constant-time MAC compare, IV byte positions, sessionId-mismatch decrypt path, the empty-payload encrypt / decrypt boundary, ParseVersion entryCount=0 and all-below-min, ParseAlgorithms numAlgs=0, and the full CAPABILITIES field extraction. Suite is 60/60 locally; integration matrix is 18/18 against the DMTF spdm-emu across SPDM 1.2, 1.3, and 1.4.
ParseCertificate now declares the CertChainAdd return value inside the block that uses it, so cppcheck no longer flags the variable scope. test/test_spdm calls wolfSPDM_AllowUntrustedCerts since the standalone smoke test drives spdm-emu with self-signed test certs and does not configure a trust anchor.
- configure.ac errors out on LIBWOLFSSL_VERSION_HEX < 0x05008000 so the floor is documented at build time rather than via runtime breakage. - spdm_internal.h ships a wc_ForceZero stub when LIBWOLFSSL_VERSION_HEX < 0x05008004 since the public symbol was added in v5.8.4; older releases only expose a WOLFSSL_LOCAL ForceZero which is not linkable. - wolfssl-versions.yml resolves the latest -stable tag dynamically and builds/unit-tests both static and --enable-dynamic-mem against v5.8.0-stable, that latest -stable, and master. - nightly.yml dispatches a nightly-trigger repository_dispatch event at 02:17 UTC; every existing workflow now listens for it so upstream wolfSSL drift surfaces within ~24h on master. Verified locally: 64/64 unit + 18/18 spdm-emu integration tests on both v5.8.0-stable and the installed v5.9.0.
Hoist transient locals to function-top declarations so the new check-empty-brace-scopes.py CI gate passes. No behavioral changes: unit_test 64/64 and spdm-emu integration 18/18 still pass. ParseKeyExchangeRsp now wipes the intermediate th1Partial / signedDigest buffers in the cleanup path regardless of which branch returned, matching the existing zeroize-on-exit policy for the rest of the handshake material in that function.
aidangarske
force-pushed
the
cleanup-and-skoll-hardening
branch
from
d542ce4 to
60f4767
Compare
cppcheck flagged ver / algType / algCount / algSel / extLen as having function-wide scope when they are only used inside the for-loop body. The for-loop body brace is not a bare scope (it follows for(...)) so the check-empty-brace-scopes.py gate stays clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Strips Nuvoton TPM mode (
WOLFSPDM_MODE) so wolfSPDM is a pure DSP0274/DSP0277 standard requester. Adds the missing spdm-emu integration (newexamples/spdm_demoCLI + 18-test matrix driver) and rewrites the broken spdm-emu CI workflow. Then applies a deep skoll review-security pass on the diff and the full repo.Security hardening
EncryptInternal/DecryptInternalrefactored togoto exitwithwc_ForceZeroon plaintext stack bufferswc_ForceZeroof transient secrets inDeriveAppDataKeys/DeriveUpdatedKeysDisconnectfreesresponderPubKey+ clears measurement state;ConnectStandardresets stale ctx fields between retries_Static_assertonWOLFSPDM_CTX_STATIC_SIZEStandards / interop
WOLFSPDM_MIN/MAX_SPDM_VERSIONcompile-time caps andwolfSPDM_SetMaxVersionruntime clamptranscriptLenand rolls back on failureAPI changes
wolfSPDM_GetNegotiatedVersion(oldwolfSPDM_GetVersion_Negotiatedretained as exported ABI-compat alias)wolfSPDM_SetMaxVersion,wolfSPDM_GetLastPeerErrorwolfSPDM_SetRequesterKeyPair,wolfSPDM_SignHash,wolfSPDM_SetResponderPubKey,wolfSPDM_SetMode(Nuvoton mode is gone; these wrote ctx state nothing read)wolfSPDM_GetSessionIdreturns id fromSTATE_KEY_EXonward so I/O callbacks can tag the encrypted FINISH record (non-zero return does NOT imply session established)WOLFSPDM_APIvisibility shim (maps toWOLFTPM_APIwhen built inside wolfTPM)Tests + CI
lastPeerErrorCodecapture, ParseMeasurements 1.3+ RequesterContext, ParseFinishRsp 1.4 OpaqueLength, ParseChallengeAuth echo verification, sequence wrap rejection, sequence mismatch, GetSessionId mid-handshake, FINISH 1.4 OpaqueLength, KeyExchange requires cert, GetMeasurements uses SecuredExchange in STATE_MEASURED, BuildKeyExchange OpaqueData, ParseVersion 1.4examples/spdm_demoCLI: `--emu / --meas / --no-sig / --challenge / --heartbeat / --key-update / --ver`examples/spdm_test.sh18-test driver (6 scenarios x SPDM 1.2/1.3/1.4); cleanup tightened to only kill emulator instances we startedTest plan