Skip to content

rsa key validation test and additions to rsa fromdata test#339

Open
sebastian-carpenter wants to merge 1 commit into
wolfSSL:masterfrom
sebastian-carpenter:rsa-validate-tests
Open

rsa key validation test and additions to rsa fromdata test#339
sebastian-carpenter wants to merge 1 commit into
wolfSSL:masterfrom
sebastian-carpenter:rsa-validate-tests

Conversation

@sebastian-carpenter

@sebastian-carpenter sebastian-carpenter commented Dec 17, 2025

Copy link
Copy Markdown
Contributor

Adding tests for rsa key validation as well as adding some more cases to the rsa fromdata test.

This test is kind of slow due to the additions.

Bugs:

  • WP segfaults when NULL is provided for param->data field (fixed)
  • General failure of rsa key validation (mostly fixed)
    • public key should have a few other tests
  • WP accepts any type of parameter value for rsa fields (fixed)

Additions:

  • Prime check in wp_rsa_validate against 133 primes. Later uses mp_prime_is_prime to check if n is composite.
  • The 'selection' variable used for rsa key importing was mostly unused, added section to make use of it.
    • This is to follow OSSL's implementation closer. Involved checking for D param, not just N and E.
  • No param may have more bits than the modulus.
  • Added checks that all CRT parameters are provided.
    • These checks vary between OSSL versions
  • Verify that d*e is logically equivalent to 1 mod( lcm(p-1, q-1))

Problems?

  • Should negative values be stored as 0? OSSL stores them as a negative and fails later but WP must store these values as unsigned. What a hassle! Fail when a negative value is received even though OpenSSL would succeed for the same input.
  • OpenSSL can store the absence of a CRT parameter (dQ, dP, u) wolfProvider cannot. Reject when a CRTp equal to 0 is provided, otherwise the key check could pass on wolfProvider.

Comment thread src/wp_rsa_kmgmt.c
ok = 0;
}

if (ok) {
for(prime = 0; prime < VALIDATE_PRIMES_SIZE; prime++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you not use mp_prime_is_prime() with 0 trials?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would fail before it tests primes because you can't have 0 trials (as far as I know).
OpenSSL does 5 rounds of miller-rabin anyway so I'd like to use mp_prime_is_prime() but I get different results between OpenSSL and wolfProvider.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ended up doing mp_prime_is_prime with 5 trials. Looks like I still need the table of primes to check the GCD though otherwise the results diverge from OSSL.

@sebastian-carpenter sebastian-carpenter force-pushed the rsa-validate-tests branch 5 times, most recently from 6dd7067 to 982f4fd Compare January 12, 2026 21:27
@sebastian-carpenter sebastian-carpenter marked this pull request as ready for review January 12, 2026 23:06
ColtonWilley
ColtonWilley previously approved these changes Feb 2, 2026
@padelsbach

Copy link
Copy Markdown
Contributor

@sebastian-carpenter, this looks important to integrate. Can you resolve the conflicts?

@sebastian-carpenter

Copy link
Copy Markdown
Contributor Author

@sebastian-carpenter, this looks important to integrate. Can you resolve the conflicts?

I'm working on some changes. I've got it rebased and just need to work through some issues I found.

Copilot AI review requested due to automatic review settings July 1, 2026 22:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR strengthens wolfProvider’s RSA key validation and import behavior to better align with OpenSSL, and adds/extends unit tests to cover key integrity and EVP_PKEY_fromdata edge cases (NULL data, CRT completeness, bit-size bounds, negative values).

Changes:

  • Adds a new RSA key integrity unit test and registers it in the unit test harness.
  • Expands RSA fromdata tests with additional selections and parameter permutations (including NULL OSSL_PARAM data cases on newer OpenSSL).
  • Updates RSA key validation/import logic: additional modulus/exponent sanity checks, small-prime factor screening, CRT completeness checks, and stricter parameter type/value handling.

Reviewed changes

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

File Description
test/unit.h Declares the new test_rsa_key_integrity unit test.
test/unit.c Registers the new RSA key integrity test in the test case table.
test/test_rsa.c Expands RSA fromdata coverage and adds a new RSA key integrity test helper suite.
src/wp_rsa_kmgmt.c Enhances RSA validation and hardens RSA parameter import/CRT verification.

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

Comment thread test/test_rsa.c Outdated
Comment thread src/wp_rsa_kmgmt.c Outdated
Comment thread src/wp_rsa_kmgmt.c
Comment thread test/test_rsa.c Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/wp_rsa_kmgmt.c
Comment thread src/wp_rsa_kmgmt.c
Comment thread test/test_rsa.c
updated rsa_key_import / validate to survive new tests
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.

5 participants