RSA pkcs#1 v1.5 negative tests#10777
Conversation
|
retest this please |
There was a problem hiding this comment.
Pull request overview
Adds TLS 1.2 negative tests to ensure classic RSA PKCS#1 v1.5 “signature-content” binding checks are enforced by both peers, covering tampered ServerKeyExchange parameters and tampered CertificateVerify transcript content.
Changes:
- Add a client-side negative test that tampers DHE-RSA ServerKeyExchange DH parameters and expects
VERIFY_SIGN_ERROR. - Add a server-side negative test that tampers the DHE ClientKeyExchange DH public value to break the CertificateVerify transcript and expects
VERIFY_CERT_ERRORwithpeerAuthGood == 0. - Register the new tests in the TLS API test header.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/api/test_tls.h | Adds prototypes and registers the two new TLS 1.2 negative tests in the test list. |
| tests/api/test_tls.c | Implements the new memio-based TLS 1.2 negative tests for SKE parameter/signature binding and CertificateVerify transcript/signature binding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…Exchange with tampered signed parameters (VERIFY_SIGN_ERROR), covering the rsa_sa_algo signature-content binding in DoServerKeyExchange. Structured the test following test_tls12_certreq_odd_sigalgs structure. - add TLS 1.2 negative test that a server rejects a mutual-auth DHE-RSA CertificateVerify whose signed transcript was tampered (VERIFY_CERT_ERROR, peerAuthGood stays 0), covering the rsa_sa_algo signature-content binding in DoCertificateVerify.
gasbytes
force-pushed
the
rsa-pkcs-v1.5-negative-test
branch
from
c2271ca to
5eb6ee6
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) not tied to a diff line (full detail below)
Findings not tied to a diff line
New comment lines exceed 80-column convention
File: tests/api/test_tls.c:377,408,447
Function: test_tls12_ske_sig_param_binding / test_tls12_bad_cv_sig_content
Severity: Low
Three newly added comment lines run past the 80-column width that wolfSSL generally follows: line 377 (* dh_p, dh_g, dh_Ys, each opaque<1..2^16-1> ...), line 408 (/* ...and that the parameter/signature mismatch was the reason for failure. */), and line 447 (/* Pin classic safe-prime DH params (no q) so a one-byte change to the client's). The file already contains some long lines, so this is purely a consistency nit, not a functional problem.
Recommendation: Reflow the three comments to stay within 80 columns to match the prevailing style.
Referenced code: tests/api/test_tls.c:377,408,447 (2 lines)
Error-code comparison without WC_NO_ERR_TRACE wrapper
File: tests/api/test_tls.c:409,525
Function: test_tls12_ske_sig_param_binding / test_tls12_bad_cv_sig_content
Severity: Low
The new tests compare wolfSSL_get_error() against the bare constants VERIFY_SIGN_ERROR (line 409) and VERIFY_CERT_ERROR (line 525). Elsewhere in the tree these error macros are wrapped, e.g. tests/api/test_certman.c:740 uses WC_NO_ERR_TRACE(VERIFY_CERT_ERROR) and tests/api/test_tls.c:712 uses WC_NO_ERR_TRACE(BUFFER_ERROR). Without the wrapper, builds with WOLFSSL_DEBUG_TRACE_ERROR_CODES emit a spurious trace log line when the constant is referenced. The comparison value itself is still correct, and the immediate neighbors in this file (lines 115, 129, 608) also use bare constants, so this is only a consistency nit.
Recommendation: Optionally wrap the error constants in WC_NO_ERR_TRACE() for consistency with trace-enabled builds; not required given existing local usage.
Referenced code: tests/api/test_tls.c:409,525 (2 lines)
Review generated by Skoll
Description
Add TLS 1.2 negative tests that the classic rsa_sa_algo signature-content checks are enforced on both peers, a client rejects a DHE-RSA ServerKeyExchange with tampered signed parameters (VERIFY_SIGN_ERROR) in DoServerKeyExchange, and a server rejects a mutual-auth DHE-RSA CertificateVerify with a tampered signed transcript (VERIFY_CERT_ERROR, peerAuthGood stays 0) in DoCertificateVerify.
Testing
./configure --enable-all --enable-debug &&
make -j8 &&
make check
Checklist