Address ML-KEM PR review feedback and fix security issues#2
Merged
Conversation
- Use wc_mlkem.h instead of mlkem.h across all includes - Remove redundant sz from keygen request (determined by level) - Remove redundant ctSz/ssSz from encaps request and ssSz from decaps request (output sizes fixed by ML-KEM level) - Remove redundant size parameter from MakeExportKey/MakeCacheKey APIs, aligning non-DMA with DMA signatures - Move large ML-KEM key buffers from stack to heap via XMALLOC - Add WH_DEBUG_CLIENT_VERBOSE logging to all ML-KEM client methods - Add doxygen documentation comments to all ML-KEM function declarations in wh_client_crypto.h - Refactor _MlKemMakeKey to use early-exit pattern for readability - Fix encaps/decaps buffer size checks: error on insufficient buffers instead of silently truncating - Update server encaps/decaps handlers to compute output sizes from key level instead of request fields - Update tests and benchmarks for API changes https://claude.ai/code/session_011esueUzLf9urCUHRPEhyXb
- Fix missed test call with old 4-arg MakeExportKey signature - Zero sensitive key material with ForceZero before XFREE in all import/export/keygen functions to prevent key data remaining on the heap after deallocation - Add response buffer bounds validation in encaps/decaps to prevent reading past the comm buffer on corrupted server responses - Fix early returns in encaps/decaps (both DMA and non-DMA) that skipped key eviction when dataPtr was NULL after auto-import - Zero-initialize DMA response structs in server-side keygen, encaps, and decaps handlers to prevent leaking uninitialized stack memory to the client on error paths - Initialize evict and size variables in DMA server handlers https://claude.ai/code/session_011esueUzLf9urCUHRPEhyXb
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
2 participants
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
Review Comment Fixes
wc_mlkem.hinstead ofmlkem.hacross all 8 filesszfrom keygen request,ctSz/ssSzfrom encaps request,ssSzfrom decaps request (all derivable from ML-KEM level)sizefromMakeExportKey/MakeCacheKey, aligning non-DMA with DMA signaturesWC_ML_KEM_MAX_PRIVATE_KEY_SIZEstack buffers to heap viaXMALLOCWH_DEBUG_CLIENT_VERBOSE()to all ML-KEM client methods_MlKemMakeKeyto early-exit pattern for readabilitywc_MlKemKey_CipherTextSize()/wc_MlKemKey_SharedSecretSize()instead of trusting request fieldsSecurity and Bug Fixes
ForceZero()beforeXFREE()in all import/export/keygen functionsmemcpyin encaps/decapsevict,ctLen,ssLenin DMA server handlersMakeExportKeysignatureTest plan
https://claude.ai/code/session_011esueUzLf9urCUHRPEhyXb