Fix: bound explicit EC-domain field lengths to prevent buffer overflow - #11273
Fix: bound explicit EC-domain field lengths to prevent buffer overflow#11273jackctj117 wants to merge 2 commits into
Conversation
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Pull request overview
This PR hardens explicit EC-domain parameter decoding against malformed DER that could trigger buffer overflows when converting explicit parameters (prime, A, B, order, and derived base-point coordinates) into fixed-size hex-string buffers.
Changes:
- Added explicit field-length bounds in
EccSpecifiedECDomainDecode()to reject DER explicit-parameter fields larger thanMAX_ECC_BYTES. - Added regression tests that construct explicit-parameter SubjectPublicKeyInfo blobs with oversized fields and verify rejection with
ASN_PARSE_E. - Integrated the new regression test into both the API test suite and
wolfcrypt/test/test.cECC tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
wolfcrypt/src/asn.c |
Adds MAX_ECC_BYTES bounds for explicit EC-domain fields during template-based ASN decoding. |
tests/api/test_ecc.c |
Adds API-level regression test that builds explicit-parameter SPKI inputs and checks acceptance/rejection behavior. |
tests/api/test_ecc.h |
Declares and registers the new API regression test in the ECC test group. |
wolfcrypt/test/test.c |
Adds a second regression test variant in the wolfCrypt test harness and wires it into ecc_test(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| byte pub[68]; | ||
| pub[0] = 0x00; /* BIT STRING unused-bits */ | ||
| pub[1] = 0x04; /* uncompressed point */ | ||
| for (i = 0; i < 32; i++) pub[2 + i] = 0x44; | ||
| for (i = 0; i < 32; i++) pub[34 + i] = 0x55; | ||
| bo = ecc11288_tlv(body, bo, 0x03, pub, 66); | ||
| } |
| { | ||
| byte pub[68]; | ||
| pub[0] = 0x00; | ||
| pub[1] = 0x04; | ||
| for (i = 0; i < 32; i++) pub[2 + i] = 0x44; | ||
| for (i = 0; i < 32; i++) pub[34 + i] = 0x55; | ||
| bo = ecc_ssdd_tlv(body, bo, 0x03, pub, 66); /* pubkey BIT STRING */ | ||
| } |
| if ((dataASN[ECCSPECIFIEDASN_IDX_PRIME_P].data.ref.length > | ||
| (word32)MAX_ECC_BYTES) || | ||
| (dataASN[ECCSPECIFIEDASN_IDX_PARAM_A].data.ref.length > | ||
| (word32)MAX_ECC_BYTES) || | ||
| (dataASN[ECCSPECIFIEDASN_IDX_PARAM_B].data.ref.length > | ||
| (word32)MAX_ECC_BYTES) || | ||
| (dataASN[ECCSPECIFIEDASN_IDX_ORDER].data.ref.length > | ||
| (word32)MAX_ECC_BYTES)) { | ||
| ret = ASN_PARSE_E; |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11273
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| int test_wc_EccPublicKeyDecode_specifiedOverflow(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(WOLFSSL_CUSTOM_CURVES) && defined(WOLFSSL_ASN_TEMPLATE) && \ |
There was a problem hiding this comment.
New ECC test guard omits HAVE_ECC, breaking the unit-test build · Logic errors
The guard omits HAVE_ECC, yet the body uses MAX_ECC_BYTES, ecc_key, and wc_ecc_init, which ecc.h defines only inside its #ifdef HAVE_ECC. --disable-ecc --enable-ecccustcurves defines WOLFSSL_CUSTOM_CURVES without HAVE_ECC, so test_ecc.c fails to compile. Every other function in this file guards on HAVE_ECC.
Fix: Add defined(HAVE_ECC) to the #if conditions at lines 3508 and 3602.
| static const byte cof1[1] = { 0x01 }; | ||
| byte num[512]; | ||
| byte inner[1024]; | ||
| byte body[2048]; |
There was a problem hiding this comment.
DER builder puts 3.6 KB of fixed buffers on the stack in wolfcrypt/test/test.c · Buffer safety in test payload builders
num[512], inner[1024] and body[2048] add ~3.6 KB to the stack frame in ecc_test()'s call chain, unguarded by WOLFSSL_SMALL_STACK. test.c is compiled into MCU targets as a smoke test and heap-allocates buffers as small as 1280 bytes there; this frame overflows such stacks. The actual maximum needed is a few hundred bytes.
Fix: Right-size the buffers to the required maximum or allocate them from HEAP_HINT under WOLFSSL_SMALL_STACK, as elsewhere in test.c.
| { 0x2a,0x86,0x48,0xce,0x3d,0x02,0x01 }; /* 1.2.840.10045.2.1 */ | ||
| static const byte ver2[1] = { 0x02 }; | ||
| static const byte cof1[1] = { 0x01 }; | ||
| byte num[512]; |
There was a problem hiding this comment.
3.5 KB of unconditional stack scratch buffers added to wolfcrypt/test/test.c · Resource leaks on error paths
ecc_ssdd_build puts 3,584 bytes of scratch arrays on the stack in a single frame with no WOLFSSL_SMALL_STACK heap fallback and no opt-out macro. test.c is compiled into microcontroller/RTOS smoke-test builds, where this overflows the test thread stack.
Related known finding #5983 (similar but distinct): Both involve scratch-buffer memory handling under small-stack concerns, but they are in different files/functions and faulting operations: unconditional stack allocation in ecc_ssdd_build versus a direct return bypassing heap cleanup in blake2b_final. The root causes and required patches differ, so one patch would not fix both.
Fix: Allocate the scratch buffers from HEAP_HINT under WOLFSSL_SMALL_STACK (or gate the whole test), matching ecc_test_custom_curves.
| int test_wc_EccPublicKeyDecode_specifiedOverflow(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(WOLFSSL_CUSTOM_CURVES) && defined(WOLFSSL_ASN_TEMPLATE) && \ |
There was a problem hiding this comment.
New ECC unit test compiled without a HAVE_ECC guard · Preprocessor-conditional security bypass
The new test body is guarded only by WOLFSSL_CUSTOM_CURVES && WOLFSSL_ASN_TEMPLATE && !WOLFSSL_NO_MALLOC, but MAX_ECC_BYTES, ecc_key, wc_ecc_init and wc_EccPublicKeyDecode exist only under HAVE_ECC (ecc.h:32). configure.ac:8053 defines WOLFSSL_CUSTOM_CURVES without requiring ECC, so --enable-ecccustcurves --disable-ecc breaks the unit-test build. Every other test in this file includes defined(HAVE_ECC).
Fix: Add defined(HAVE_ECC) && to the guards at tests/api/test_ecc.c:3508 and :3602.
| int test_wc_EccPublicKeyDecode_specifiedOverflow(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(WOLFSSL_CUSTOM_CURVES) && defined(WOLFSSL_ASN_TEMPLATE) && \ |
There was a problem hiding this comment.
New ECC test lacks HAVE_ECC guard, breaking the build when custom curves are enabled without ECC · Conditional compilation / build breakage
The guard omits HAVE_ECC, but MAX_ECC_BYTES, ecc_key, wc_ecc_init, wc_ecc_free and wc_EccPublicKeyDecode are all declared inside #ifdef HAVE_ECC in wolfssl/wolfcrypt/ecc.h. configure.ac:8053 defines WOLFSSL_CUSTOM_CURVES independently of ECC, so --disable-ecc --enable-ecccustcurves fails to compile tests/api/test_ecc.c. Every other test in this file guards on HAVE_ECC; the wolfcrypt/test/test.c copy is already inside a HAVE_ECC block.
Fix: Add defined(HAVE_ECC) && to both the helper-function guard at line 3508 and the test-body guard at line 3602.
| int test_wc_EccPublicKeyDecode_specifiedOverflow(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(WOLFSSL_CUSTOM_CURVES) && defined(WOLFSSL_ASN_TEMPLATE) && \ |
There was a problem hiding this comment.
New ECC test lacks HAVE_ECC guard, breaking builds with custom curves but ECC disabled · Preprocessor-conditional security bypass
The new helper block (line 3508) and test body are gated only on WOLFSSL_CUSTOM_CURVES && WOLFSSL_ASN_TEMPLATE && !WOLFSSL_NO_MALLOC. All of ecc_key, MAX_ECC_BYTES, wc_ecc_init and wc_EccPublicKeyDecode live under #ifdef HAVE_ECC, and configure accepts --disable-ecc --enable-ecccustcurves, so that config fails to compile. Every other test in this file guards on HAVE_ECC.
Fix: Add defined(HAVE_ECC) to both preprocessor guards in tests/api/test_ecc.c (lines 3508 and 3602).
This pull request addresses a security issue (issue 11288) related to potential buffer overflows when decoding explicit EC domain parameters from DER-encoded data. The main change is to ensure that all explicit-parameter field lengths (prime, coordinate, A, B, and order) are properly bounded to prevent overflows. It also adds regression tests to verify the fix and prevent future regressions.
Security and correctness improvements:
EccSpecifiedECDomainDecodeto reject DER-encoded EC domain parameters with field lengths exceedingMAX_ECC_BYTES, preventing buffer overflows and ensuring safe handling of explicit EC-domain parameters.Testing and regression coverage:
test_wc_EccPublicKeyDecode_specifiedOverflowintest_ecc.cthat builds DER-encoded EC public keys with oversized fields to verify that the new length checks correctly reject them, and that boundary cases are accepted.test_ecc.hand included it in the ECC test group for automated test runs. [1] [2]ecc_ssdd_overflow_test) inwolfcrypt/test/test.cfor additional coverage, including helper functions for DER construction, and integrated it into the main ECC test routine. [1] [2]These changes collectively harden the ECC decoding logic against malformed or malicious input and ensure that future changes will be tested for this class of vulnerability.