Enhance extra user data value and external cookie length max size#10236
Enhance extra user data value and external cookie length max size#10236Roy-Carter wants to merge 7 commits intowolfSSL:masterfrom
Conversation
…hich use high scale of operations require more than 99 index options back from SSL_get_ex_new_index
…r hijacking) can be more than 32 in size based on RFC6347
…hat we define maximum copy of <=254 to avoid buffer overflow attempts upon exactly 255..
|
@julek-wolfssl if you can please take a look , as part of the integration i've come across these 2 minor issues :) |
|
Can one of the admins verify this patch? |
|
@Roy-Carter is an approved contributor. |
|
ok to test |
|
|
@julek-wolfssl can you re run the workflows ? |
|
@Roy-Carter please address test failures. |
|
@julek-wolfssl can we re-run workflows ? |
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 7 total — 6 posted, 1 skipped
Posted findings
- [High] WOLFSSL_COOKIE_LEN defined inside wrong preprocessor guard — breaks build for DTLS without TLS13/PSK —
wolfssl/internal.h:1449-1452 - [Medium] PR description vs. code mismatch — default MAX_COOKIE_LEN not actually raised —
wolfssl/internal.h:1451 - [Medium] cookieSz is a
bytebut WOLFSSL_COOKIE_LEN is unbounded — silent truncation risk when override exceeds 255 —wolfssl/internal.h:5290-5291 - [Medium] No new tests for the expanded configure range or overridable cookie length —
configure.ac:10376-10382, wolfssl/internal.h:1449-1452 - [Low] Trailing whitespace in new #define and #endif —
wolfssl/internal.h:1451-1452 - [Low] Error message wording — 'a number from 1 to 9999' is accurate but consider clarifying the memory tradeoff —
configure.ac:10382
Skipped findings
- [Medium] MAX_EX_DATA=9999 yields very large fixed arrays (~80 KB per object) — document memory cost
Review generated by Skoll via openclaw
| /* Maximum size for a DTLS cookie */ | ||
| #define WOLFSSL_COOKIE_LEN 32 | ||
| #endif | ||
|
|
There was a problem hiding this comment.
🟠 [High] WOLFSSL_COOKIE_LEN defined inside wrong preprocessor guard — breaks build for DTLS without TLS13/PSK
🚫 BLOCK bug
The new WOLFSSL_COOKIE_LEN macro is placed inside the #if defined(WOLFSSL_TLS13) || !defined(NO_PSK) block that starts at line 1445 and ends at line 1468. However, MAX_COOKIE_LEN = WOLFSSL_COOKIE_LEN is referenced unconditionally inside enum Misc at line 1577, and the cookie[MAX_COOKIE_LEN] buffer at line 5290 is guarded by #ifdef WOLFSSL_DTLS, which is independent of WOLFSSL_TLS13 and NO_PSK. A DTLS 1.2 build with NO_PSK defined and without WOLFSSL_TLS13 would leave WOLFSSL_COOKIE_LEN undefined while MAX_COOKIE_LEN still tries to expand it, causing a compile error (undeclared identifier WOLFSSL_COOKIE_LEN). The macro must live outside the TLS13/PSK guard — either at the top of the file with other user-overridable tunables, or gated on WOLFSSL_DTLS.
Suggestion:
| /* Move the define outside the TLS13/PSK guard, e.g. just above enum Misc */ | |
| #ifndef WOLFSSL_COOKIE_LEN | |
| /* Maximum size for a DTLS cookie */ | |
| #define WOLFSSL_COOKIE_LEN 32 | |
| #endif | |
| enum Misc { | |
| ... | |
| MAX_COOKIE_LEN = WOLFSSL_COOKIE_LEN, /* max dtls cookie size */ |
| #endif | ||
|
|
||
| #if defined(WOLFSSL_TLS13) || !defined(NO_PSK) | ||
|
|
There was a problem hiding this comment.
🟡 [Medium] PR description vs. code mismatch — default MAX_COOKIE_LEN not actually raised
💡 SUGGEST question
The PR description states DTLS MAX_COOKIE_LEN raised to 254 - RFC 6347 defines cookie as opaque<0..2^8-1>, so max valid length is 255. Set to 254 to prevent buffer overflow attempts at boundary. However the default for WOLFSSL_COOKIE_LEN is 32 and MAX_COOKIE_LEN remains 32 unless the user explicitly overrides it. This silently leaves stock builds with the same restriction the PR claims to remove. Either (a) raise the default (as the description promises) or (b) update the PR description and clearly document the new override knob in the relevant header/docs so users know how to opt in.
Suggestion:
| /* If the intent is truly to raise the cap, bump the default */ | |
| #ifndef WOLFSSL_COOKIE_LEN | |
| /* Maximum size for a DTLS cookie (RFC 6347 opaque<0..2^8-1>) */ | |
| #define WOLFSSL_COOKIE_LEN 255 | |
| #endif |
| MAX_COOKIE_LEN = WOLFSSL_COOKIE_LEN, /* max dtls cookie size */ | ||
| COOKIE_SZ = 20, /* use a 20 byte cookie */ | ||
| SUITE_LEN = 2, /* cipher suite sz length */ | ||
| ENUM_LEN = 1, /* always a byte */ |
There was a problem hiding this comment.
🟡 [Medium] cookieSz is a byte but WOLFSSL_COOKIE_LEN is unbounded — silent truncation risk when override exceeds 255
💡 SUGGEST bug
cookieSz is declared as byte (uint8_t). Any user who defines -DWOLFSSL_COOKIE_LEN=300 (or similar >255) will get a cookie buffer of 300 bytes but a size field that can only represent 0..255. In src/internal.c:38156 the check peerCookieSz > MAX_COOKIE_LEN is compared against a word32-ish value, but downstream assignments cookieSz = ... will silently truncate. Since WOLFSSL_COOKIE_LEN is now a user-overridable macro, a compile-time guard should reject values > 255 (the protocol maximum per RFC 6347 anyway).
Suggestion:
| ENUM_LEN = 1, /* always a byte */ | |
| #ifndef WOLFSSL_COOKIE_LEN | |
| #define WOLFSSL_COOKIE_LEN 32 | |
| #endif | |
| #if WOLFSSL_COOKIE_LEN > 255 | |
| #error "WOLFSSL_COOKIE_LEN must be <= 255 per RFC 6347 (opaque<0..2^8-1>)" | |
| #endif |
| #endif | ||
|
|
||
| #if defined(WOLFSSL_TLS13) || !defined(NO_PSK) | ||
|
|
There was a problem hiding this comment.
🔵 [Low] Trailing whitespace in new #define and #endif
🔧 NIT style
Both new lines have trailing whitespace: #define WOLFSSL_COOKIE_LEN 32 and #endif . wolfSSL's style discourages trailing spaces; some CI checks may flag this.
Suggestion:
| #define WOLFSSL_COOKIE_LEN 32 | |
| #endif |
| AM_CFLAGS="$AM_CFLAGS -DHAVE_EX_DATA -DMAX_EX_DATA=$ENABLED_EX_DATA" | ||
| ;; | ||
| *) AC_MSG_ERROR([Invalid argument to --enable-context-extra-user-data -- must be yes, no, or a number from 1 to 99]) | ||
| *) AC_MSG_ERROR([Invalid argument to --enable-context-extra-user-data -- must be yes, no, or a number from 1 to 9999]) |
There was a problem hiding this comment.
🔵 [Low] Error message wording — 'a number from 1 to 9999' is accurate but consider clarifying the memory tradeoff
🔧 NIT style
The error message only lists the accepted range. It would be user-friendly to note (either here or in the docs) that very large values have a memory cost because the ex_data array is fixed size. Not a blocker.
Suggestion:
| *) AC_MSG_ERROR([Invalid argument to --enable-context-extra-user-data -- must be yes, no, or a number from 1 to 9999]) | |
| AC_MSG_ERROR([Invalid argument to --enable-context-extra-user-data -- must be yes, no, or a number from 1 to 9999 (note: each index reserves one pointer per object, so large values increase memory use)]) |
Description
Enhance configuration limits and fix max size constants to align with RFCs and large-scale deployment needs.
SSL_get_ex_new_index limit raised - --enable-context-extra-user-data now accepts values up to 9999 (was 99). Large platforms with high-scale operations need more than 99 ex_data indices. I've encountered it since my code uses :
SSL_EX_DATA_IND_DTLS_SESSION = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
SSL_EX_DATA_IND_PSK = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
and on "Strong" machines in which i had 50+ cores running it which means (2x50) I failed to initialize an index for a DTLS session.
DTLS MAX_COOKIE_LEN raised to 254 - RFC 6347 defines cookie as opaque<0..2^8-1>, so max valid length is 255. Set to 254 to prevent buffer overflow attempts at boundary. Previous value of 32 was too restrictive for legitimate external cookie use. I've encountered it while trying to inject an external cookie which had valid length of more than 32 .
Testing
Build configuration tested with --enable-context-extra-user-data values: 1, 99, 100, 999, 9999
Verified configure.ac pattern matching rejects invalid inputs (0, 10000, strings)
DTLS cookie handling reviewed for buffer safety with new MAX_COOKIE_LEN