Skip to content

F-1370 : Tighten key_len check from >= to ==#10122

Open
miyazakh wants to merge 1 commit intowolfSSL:masterfrom
miyazakh:f-1370_SigGetSize
Open

F-1370 : Tighten key_len check from >= to ==#10122
miyazakh wants to merge 1 commit intowolfSSL:masterfrom
miyazakh:f-1370_SigGetSize

Conversation

@miyazakh
Copy link
Copy Markdown
Contributor

@miyazakh miyazakh commented Apr 2, 2026

Description

Summary

wc_SignatureGetSize() (and callers that propagate key_len) previously accepted any key_len >= sizeof(key_type), meaning an oversized value silently passed the guard and allowed the void* to be dereferenced as the wrong type. This PR tightens the check to exact equality and aligns documentation and tests accordingly.

Changes

wolfcrypt/src/signature.c

  • Change key_len >= sizeof(ecc_key)key_len == sizeof(ecc_key) and the equivalent RSA check.
  • Add inline comments explaining that size equality is a necessary but not sufficient type check, since the void* API cannot verify the actual runtime type of the pointed-to object.

doc/dox_comments/header_files/signature.h

  • Update \param key for all seven Signature API functions to document the exact type requirement and the caller's responsibility for passing the correct concrete type.
  • Update \param key_len to state "Must be exactly sizeof(ecc_key) or sizeof(RsaKey) " (previously said "Size of the key structure", which implied >= semantics).
  • Update \return for wc_SignatureGetSize() to document that a key_len mismatch returns BAD_FUNC_ARG.

Testing

  • Add bad-arg test cases for key_len = sizeof(ecc_key) - 1 and key_len = sizeof(ecc_key) + 1 in test_wc_SignatureGetSize_ecc.
  • Add bad-arg test cases for key_len = sizeof(RsaKey) - 1 and key_len = sizeof(RsaKey) + 1 in test_wc_SignatureGetSize_rsa.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@miyazakh miyazakh self-assigned this Apr 2, 2026
Copilot AI review requested due to automatic review settings April 2, 2026 01:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Tightens wc_SignatureGetSize() key-length validation to require exact structure size matches, reducing the risk of mis-typed void* keys being dereferenced, and updates tests/docs to reflect the stricter contract.

Changes:

  • Enforce key_len == sizeof(ecc_key) / key_len == sizeof(RsaKey) in wc_SignatureGetSize().
  • Add/adjust API tests for key_len off-by-one and zero-length cases (ECC/RSA).
  • Update Doxygen to document the exact-size requirement and caller type-safety responsibility.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
wolfcrypt/src/signature.c Tightens ECC/RSA key_len checks from >= to == and adds explanatory comments.
tests/api/test_signature.h Fixes test registration to include RSA test instead of duplicating ECC.
tests/api/test_signature.c Adds negative tests for key_len being 0, sizeof-1, and sizeof+1 for ECC and RSA.
doc/dox_comments/header_files/signature.h Updates Doxygen for key/key_len constraints and clarifies error behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +13 to +22
\param key Pointer to the key structure corresponding to sig_type:
pass an ecc_key* (cast to const void*) for
WC_SIGNATURE_TYPE_ECC, or a RsaKey* for
WC_SIGNATURE_TYPE_RSA / WC_SIGNATURE_TYPE_RSA_W_ENC.
The caller is responsible for ensuring the pointer refers to the correct
type; this function cannot verify the actual runtime type of the object.
\param key_len Must be exactly sizeof(ecc_key) or
sizeof(RsaKey) matching the sig_type. Passing any other value
causes the function to return BAD_FUNC_ARG without dereferencing key.
The conventional idiom is to pass sizeof(*key) at the call site.
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

The updated docs state key_len “must be exactly …” unconditionally, but the tests show wc_SignatureGetSize(sig_type, NULL, key_len) is valid and returns 0 (even when key_len is non-matching). Please clarify the contract, e.g., “If key is non-NULL, key_len must be exactly …; if key is NULL, key_len is ignored and the function returns 0,” to avoid misleading API consumers.

Copilot uses AI. Check for mistakes.
Comment on lines +14 to +15
pass an ecc_key* (cast to const void*) for
WC_SIGNATURE_TYPE_ECC, or a RsaKey* for
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

This text calls out a cast to const void* for ecc_key* but not for RsaKey*, even though both convert implicitly to const void* in C/C++. Consider making this consistent (either remove the cast mention entirely, or mention it uniformly for both) to avoid implying different call requirements.

Suggested change
pass an ecc_key* (cast to const void*) for
WC_SIGNATURE_TYPE_ECC, or a RsaKey* for
pass an ecc_key* for WC_SIGNATURE_TYPE_ECC, or a RsaKey* for

Copilot uses AI. Check for mistakes.
Comment on lines +96 to +100
/* Verify that key_len matches exactly sizeof(ecc_key).
* This is a necessary but not sufficient type check: the void*
* API cannot verify the actual runtime type of the pointed-to
* object. Callers must pass a valid ecc_key* cast to void*. */
if (key_len == sizeof(ecc_key)) {
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

The comment refers to a void* API and says “cast to void*”, but the function parameter is const void*. To keep the comment accurate (and avoid encouraging dropping const), adjust wording to const void* / “cast to const void*”.

Copilot uses AI. Check for mistakes.
@miyazakh
Copy link
Copy Markdown
Contributor Author

miyazakh commented Apr 2, 2026

retest this please

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.

2 participants