Send BUFFER_ERROR if size does not meet minimum Requirements #7602

night1rider · 2024-05-30T18:10:14Z

PR in response to #7275

Send a BUFFER_ERROR if size does not meet the minimum requirement for the extension.

dgarske · 2024-05-30T18:26:19Z

src/tls.c

@@ -19,6 +19,23 @@
 * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA
 */

+// Define minimum size constants used in parsing macros


Please use C style commenting /* */

src/tls.c

lealem47 · 2024-05-31T21:03:10Z

Macro list I came up with after doing the math on the extension_data field for the various extensions referencing their RFC's. The minimum size check should only be done for the first group if parsing the client hello, as the extension_data for those extensions will be empty in the server hello. The second group can be checked regardless.

/* Check if parsing client hello */
#define SNI_MIN_SIZE   4
#define EDI_MIN_SIZE   0
#define TCA_MIN_SIZE   2
#define CSR_MIN_SIZE   5
#define PKM_MIN_SIZE   1
#define CSR2_MIN_SIZE  7


/* Check regardless */
#define CID_MIN_SIZE   1
#define ALPN_MIN_SIZE  2
#define SRTP_MIN_SIZE  3
#define KS_MIN_SIZE    1
#define ETM_MIN_SIZE   0
#define PSK_MIN_SIZE   2
#define CCT_MIN_SIZE   1
#define SCT_MIN_SIZE   1
#define PHA_MIN_SIZE   0
#define THM_MIN_SIZE   0
#define SA_MIN_SIZE    2
#define SAC_MIN_SIZE   2
#define EC_MIN_SIZE    2
#define MFL_MIN_SIZE   1
#define CKE_MIN_SIZE   3
#define SV_MIN_SIZE    2
#define SCR_MIN_SIZE   1
#define PF_MIN_SIZE    1
#define CAN_MIN_SIZE   3
#define QTP_MIN_SIZE   0
#define STK_MIN_SIZE   0

src/tls.c

night1rider · 2024-06-04T16:59:00Z

Retest this please

dgarske · 2024-06-04T17:55:27Z

src/tls.c

@@ -1978,14 +1978,18 @@ int TLSX_ALPN_GetRequest(TLSX* extensions, void** data, word16 *dataSz)
 #define ALPN_FREE_ALL     TLSX_ALPN_FreeAll
 #define ALPN_GET_SIZE     TLSX_ALPN_GetSize
 #define ALPN_WRITE        TLSX_ALPN_Write
-#define ALPN_PARSE        TLSX_ALPN_ParseAndSet
+#define ALPN_PARSE(a, b, c, d, e) (((e) == client_hello ? \


Is the parse macro the right place to fix this min check? Have you looked a code size impact? Using IDE/GCC-ARM is a good way to gauge code size impact.

I don't want to see the macro doing this.
The macro is do either call or not.
Can you generically check? Table with values and look it up?

Addressed the feedback and left a comment on the PR

wolfssl/ssl.h

night1rider · 2024-06-05T21:54:44Z

Retest this please

lealem47 · 2024-06-05T21:57:27Z

wolfssl/ssl.h

+    #define WOLFSSL_STK_MIN_SIZE_SERVER   0
+#endif
+
+#define TLSX_SERVER_NAME_DEFINE                0x0000 /* a.k.a. SNI */


Why not use the extension enums already defined in internal.h for these values? You'd have to remove all the guards around some of them for this fix so that they are always defined but I don't see why we couldn't do that.

typedef enum { TLSX_SERVER_NAME = 0x0000, /* a.k.a. SNI */ TLSX_MAX_FRAGMENT_LENGTH = 0x0001, TLSX_TRUSTED_CA_KEYS = 0x0003, TLSX_TRUNCATED_HMAC = 0x0004, TLSX_STATUS_REQUEST = 0x0005, /* a.k.a. OCSP stapling */ TLSX_SUPPORTED_GROUPS = 0x000a, /* a.k.a. Supported Curves */ TLSX_EC_POINT_FORMATS = 0x000b, TLSX_SIGNATURE_ALGORITHMS = 0x000d, /* HELLO_EXT_SIG_ALGO */ TLSX_USE_SRTP = 0x000e, /* 14 */ TLSX_APPLICATION_LAYER_PROTOCOL = 0x0010, /* a.k.a. ALPN */ TLSX_STATUS_REQUEST_V2 = 0x0011, /* a.k.a. OCSP stapling v2 */ TLSX_CLIENT_CERTIFICATE_TYPE = 0x0013, /* RFC8446 */ TLSX_SERVER_CERTIFICATE_TYPE = 0x0014, /* RFC8446 */ TLSX_ENCRYPT_THEN_MAC = 0x0016, /* RFC 7366 */ TLSX_EXTENDED_MASTER_SECRET = 0x0017, /* HELLO_EXT_EXTMS */ TLSX_SESSION_TICKET = 0x0023, TLSX_PRE_SHARED_KEY = 0x0029, TLSX_EARLY_DATA = 0x002a, TLSX_SUPPORTED_VERSIONS = 0x002b, TLSX_COOKIE = 0x002c, TLSX_PSK_KEY_EXCHANGE_MODES = 0x002d, TLSX_CERTIFICATE_AUTHORITIES = 0x002f, TLSX_POST_HANDSHAKE_AUTH = 0x0031, TLSX_SIGNATURE_ALGORITHMS_CERT = 0x0032, TLSX_KEY_SHARE = 0x0033, TLSX_CONNECTION_ID = 0x0036, TLSX_KEY_QUIC_TP_PARAMS = 0x0039, /* RFC 9001, ch. 8.2 */ TLSX_CKS = 0xff92, /* X9.146; ff indicates personal * use and 92 is hex for 146. */ TLSX_RENEGOTIATION_INFO = 0xff01, TLSX_KEY_QUIC_TP_PARAMS_DRAFT = 0xffa5, /* from draft-ietf-quic-tls-27 */ TLSX_ECH = 0xfe0d, /* from draft-ietf-tls-esni-13 */ } TLSX_Type;

It could be changed in this PR, I am mainly unsure of due to lack of knowledge of if this may break something. However on first glance I don't see what it would break if the guards where removed. It would also increase code size slightly I would assume.

night1rider · 2024-06-05T23:35:14Z

I removed the logic from the macro (I added these macros), and squashed it out of the pr.

I switch this over, to a jump table due to the enum highest value for TLSX_Type being too large to setup a lookup table with structs.
I copied the TLSX_Type values to #define because the current enum values are guarded, but I need them available even if some TLSX are not on in the build, to fix the bug. I didn't want to remove the guards on the enum type since I am unsure of the implications of that change. Essentially even if the Client/Server doesn't support an extension like CSR or SNI we still obtain the hello_client/server message and need to check that its (size is) valid since it will effect the other extension offsets during the offset calculations

Code impact wise for this initial solution using IDE/GCC-ARM makefile
Master Commit: 70d317e

/usr/bin/arm-none-eabi-size ./Build/WolfSSLServer.elf
   text    data     bss     dec     hex filename
 140519     136      52  140707   225a3 ./Build/WolfSSLServer.elf
/usr/bin/arm-none-eabi-size ./Build/WolfSSLClient.elf
   text    data     bss     dec     hex filename
 140537     136      52  140725   225b5 ./Build/WolfSSLClient.elf

This PR Commit as of writing this:

/usr/bin/arm-none-eabi-size ./Build/WolfSSLServer.elf
   text    data     bss     dec     hex filename
 140775     136      52  140963   226a3 ./Build/WolfSSLServer.elf
/usr/bin/arm-none-eabi-size ./Build/WolfSSLClient.elf
   text    data     bss     dec     hex filename
 140793     136      52  140981   226b5 ./Build/WolfSSLClient.elf

For WolfSSLServer.elf:
text: +256 bytes (0.182%)
data: 0 bytes (no change)
bss: 0 bytes (no change)
dec: +256 bytes (0.182%)
For WolfSSLClient.elf:
text: +256 bytes (0.182%)
data: 0 bytes (no change)
bss: 0 bytes (no change)
dec: +256 bytes (0.182%)

This will be a flat increase to all builds that use HAVE_TLS_EXTENSIONS.

It can be reduced with the usage of either NO_WOLFSSL_SERVER or NO_WOLFSSL_CLIENT

@dgarske @SparkiDev @ejohnstown thoughts?

dgarske

Your updated solution is much better. Almost there.

dgarske · 2024-06-06T14:51:14Z

wolfssl/internal.h

@@ -2891,6 +2891,39 @@ typedef enum {
 #endif
 } TLSX_Type;

+/* Same list a the above enum TLSX_Type */


I'd prefer if you could just make the above TLSX_Type types always available and use those directly.

Remove these... _DEFINE types (not used now)

dgarske · 2024-06-06T14:51:47Z

wolfssl/ssl.h

@@ -5371,6 +5371,176 @@ WOLFSSL_API int wolfSSL_dtls_cid_get_tx(WOLFSSL* ssl, unsigned char* buffer,
 #define DTLS1_2_VERSION                  0xFEFD
 #define DTLS1_3_VERSION                  0xFEFC

+/* Define minimum size constants used in parsing macros */


Can you describe how these "min" values were established. I recall you saying they came from @lealem47

@dgarske The values were established adding up the minimum sizes of the data types in the required structs for the extension_data field for the various extensions referencing their RFC's (primarily https://datatracker.ietf.org/doc/html/rfc6066 and https://datatracker.ietf.org/doc/html/rfc8446). I assumed zero-length when applicable. For example, WOLFSSL_CSR_MIN_SIZE_CLIENT = 5 was derived from the struct below.

struct { CertificateStatusType status_type; select (status_type) { case ocsp: OCSPStatusRequest; } request; } CertificateStatusRequest; enum { ocsp(1), (255) } CertificateStatusType; struct { ResponderID responder_id_list<0..2^16-1>; Extensions request_extensions; } OCSPStatusRequest; opaque ResponderID<1..2^16-1>; opaque Extensions<0..2^16-1>;

The CertificateStatusRequest extension has a status_type (1 or 255, which is 1 byte). The OCSPStatusRequest consists of two opaques, the responder_id_list and request_extensions. Both of those start with a 16-bit length, so 4 bytes total of 0 lengths. So 5 total. My math on these should be double-checked.

Please add code comments and feedback from Lealem.

dgarske · 2024-06-06T18:34:00Z

src/tls.c

@@ -14366,6 +14366,134 @@ int TLSX_ParseVersion(WOLFSSL* ssl, const byte* input, word16 length,
    return ret;
 }
 #endif
+#ifndef NO_WOLFSSL_SERVER
+static word16 TLSX_GetMinSize_Client(word16* type) {


Function start brace on new line please

dgarske · 2024-06-06T18:34:04Z

src/tls.c

+#endif
+
+#ifndef NO_WOLFSSL_CLIENT
+static word16 TLSX_GetMinSize_Server(const word16 *type) {


dgarske · 2024-06-06T18:34:22Z

wolfssl/internal.h

+#define TLSXT_USE_SRTP                   0x000e /* 14 */
+#define TLSXT_APPLICATION_LAYER_PROTOCOL 0x0010 /* a.k.a. ALPN */
+#define TLSXT_STATUS_REQUEST_V2          0x0011 /* a.k.a. OCSP stapling v2 */
+#define TLSXT_CLIENT_CERTIFICATE        0x0013 /* RFC8446 */


NIT: Add space to align 0x0013

dgarske · 2024-06-06T18:34:45Z

wolfssl/internal.h

@@ -2891,6 +2891,39 @@ typedef enum {
 #endif
 } TLSX_Type;

+/* Same list a the above enum TLSX_Type */


Remove these... _DEFINE types (not used now)

dgarske · 2024-06-06T18:35:20Z

wolfssl/ssl.h

@@ -5371,6 +5371,176 @@ WOLFSSL_API int wolfSSL_dtls_cid_get_tx(WOLFSSL* ssl, unsigned char* buffer,
 #define DTLS1_2_VERSION                  0xFEFD
 #define DTLS1_3_VERSION                  0xFEFC

+/* Define minimum size constants used in parsing macros */


Please add code comments and feedback from Lealem.

night1rider · 2024-06-07T15:00:04Z

Retest this please

Send BUFFER_ERROR if size does not meet minimum Requirements

night1rider assigned lealem47 and night1rider May 30, 2024

dgarske requested changes May 30, 2024

View reviewed changes

src/tls.c Outdated Show resolved Hide resolved

night1rider force-pushed the Parsing-bug branch from d098f9e to b2214ed Compare May 30, 2024 22:19

lealem47 reviewed May 31, 2024

View reviewed changes

src/tls.c Outdated Show resolved Hide resolved

lealem47 removed their assignment May 31, 2024

night1rider force-pushed the Parsing-bug branch 2 times, most recently from 2176467 to 4fbdb37 Compare June 3, 2024 18:20

night1rider marked this pull request as ready for review June 4, 2024 16:08

night1rider force-pushed the Parsing-bug branch from 03045bd to cb06657 Compare June 4, 2024 16:19

dgarske assigned dgarske and wolfSSL-Bot Jun 4, 2024

dgarske self-requested a review June 4, 2024 16:32

dgarske assigned SparkiDev and lealem47 Jun 4, 2024

dgarske requested changes Jun 4, 2024

View reviewed changes

dgarske unassigned dgarske, SparkiDev and lealem47 Jun 5, 2024

JacobBarthelmeh unassigned wolfSSL-Bot Jun 5, 2024

night1rider force-pushed the Parsing-bug branch 2 times, most recently from 999fe65 to ba9a483 Compare June 5, 2024 21:20

lealem47 reviewed Jun 5, 2024

View reviewed changes

night1rider force-pushed the Parsing-bug branch 3 times, most recently from d1d9cce to 729ce8e Compare June 5, 2024 23:26

night1rider force-pushed the Parsing-bug branch from 729ce8e to d741978 Compare June 6, 2024 03:28

night1rider assigned dgarske and wolfSSL-Bot Jun 6, 2024

dgarske requested changes Jun 6, 2024

View reviewed changes

dgarske removed their assignment Jun 6, 2024

night1rider unassigned wolfSSL-Bot Jun 6, 2024

dgarske requested changes Jun 6, 2024

View reviewed changes

night1rider force-pushed the Parsing-bug branch from cadb846 to 7a9225c Compare June 6, 2024 22:14

night1rider force-pushed the Parsing-bug branch 2 times, most recently from 0dd0b88 to a06f19d Compare June 7, 2024 15:53

dgarske previously approved these changes Jun 7, 2024

View reviewed changes

dgarske assigned JacobBarthelmeh and wolfSSL-Bot Jun 7, 2024

Send BUFFER_ERROR if size does not meet minimum reqs for the extension

ebca337

night1rider dismissed dgarske’s stale review via ebca337 June 7, 2024 16:26

night1rider force-pushed the Parsing-bug branch from a06f19d to ebca337 Compare June 7, 2024 16:26

JacobBarthelmeh approved these changes Jun 7, 2024

View reviewed changes

dgarske approved these changes Jun 7, 2024

View reviewed changes

JacobBarthelmeh unassigned JacobBarthelmeh and night1rider Jun 7, 2024

JacobBarthelmeh merged commit f7bc78c into wolfSSL:master Jun 7, 2024
111 checks passed

jefferyq2 pushed a commit to jefferyq2/wolfssl that referenced this pull request Jun 9, 2024

Merge pull request wolfSSL#7602 from night1rider/Parsing-bug

2e83fae

Send BUFFER_ERROR if size does not meet minimum Requirements

lealem47 mentioned this pull request Jun 10, 2024

[Bug]: ClientHello's status_request extension handling issue #7275

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send BUFFER_ERROR if size does not meet minimum Requirements #7602

Send BUFFER_ERROR if size does not meet minimum Requirements #7602

night1rider commented May 30, 2024 •

edited

Loading

dgarske May 30, 2024

night1rider May 30, 2024

lealem47 commented May 31, 2024 •

edited

Loading

night1rider commented Jun 4, 2024

dgarske Jun 4, 2024

SparkiDev Jun 4, 2024

night1rider Jun 5, 2024

night1rider commented Jun 5, 2024

lealem47 Jun 5, 2024

night1rider Jun 5, 2024 •

edited

Loading

night1rider commented Jun 5, 2024

dgarske left a comment

dgarske Jun 6, 2024

dgarske Jun 6, 2024

dgarske Jun 6, 2024

lealem47 Jun 6, 2024

dgarske Jun 6, 2024

dgarske Jun 6, 2024

dgarske Jun 6, 2024

dgarske Jun 6, 2024

dgarske Jun 6, 2024

dgarske Jun 6, 2024

night1rider commented Jun 7, 2024

Send BUFFER_ERROR if size does not meet minimum Requirements #7602

Send BUFFER_ERROR if size does not meet minimum Requirements #7602

Conversation

night1rider commented May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lealem47 commented May 31, 2024 • edited Loading

night1rider commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

night1rider commented Jun 5, 2024

Choose a reason for hiding this comment

night1rider Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

night1rider commented Jun 5, 2024

dgarske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

night1rider commented Jun 7, 2024

night1rider commented May 30, 2024 •

edited

Loading

lealem47 commented May 31, 2024 •

edited

Loading

night1rider Jun 5, 2024 •

edited

Loading