TLS: Avoid undefined behavior in pointer arithmetic

Reorder terms in a way that no invalid pointers are generated with
pos+len operations. end-pos is always defined (with a valid pos pointer)
while pos+len could end up pointing beyond the end pointer which would
be undefined behavior.

Signed-off-by: Jouni Malinen <j@w1.fi>

diff --git a/src/tls/tlsv1_client_write.c b/src/tls/tlsv1_client_write.c
index d192f44f40882eec8dad6820a75b4bd3f17f2cd4..c5a4d4e96020f42ae63a6750385efbd4fd8fb8b2 100644 (file)
--- a/src/tls/tlsv1_client_write.c
+++ b/src/tls/tlsv1_client_write.c
@@ -134,6 +134,11 @@ static int tls_write_client_certificate(struct tlsv1_client *conn,
        struct x509_certificate *cert;
 
        pos = *msgpos;
+       if (TLS_RECORD_HEADER_LEN + 1 + 3 + 3 > end - pos) {
+               tls_alert(conn, TLS_ALERT_LEVEL_FATAL,
+                         TLS_ALERT_INTERNAL_ERROR);
+               return -1;
+       }
 
        wpa_printf(MSG_DEBUG, "TLSv1: Send Certificate");
        rhdr = pos;
@@ -154,7 +159,7 @@ static int tls_write_client_certificate(struct tlsv1_client *conn,
        pos += 3;
        cert = conn->cred ? conn->cred->cert : NULL;
        while (cert) {
-               if (pos + 3 + cert->cert_len > end) {
+               if (3 + cert->cert_len > (size_t) (end - pos)) {
                        wpa_printf(MSG_DEBUG, "TLSv1: Not enough buffer space "
                                   "for Certificate (cert_len=%lu left=%lu)",
                                   (unsigned long) cert->cert_len,
@@ -265,9 +270,16 @@ static int tlsv1_key_x_dh(struct tlsv1_client *conn, u8 **pos, u8 *end)
        wpa_hexdump(MSG_DEBUG, "TLSv1: DH Yc (client's public value)",
                    dh_yc, dh_yc_len);
 
+       if (end - *pos < 2) {
+               tls_alert(conn, TLS_ALERT_LEVEL_FATAL,
+                         TLS_ALERT_INTERNAL_ERROR);
+               os_free(csecret);
+               os_free(dh_yc);
+               return -1;
+       }
        WPA_PUT_BE16(*pos, dh_yc_len);
        *pos += 2;
-       if (*pos + dh_yc_len > end) {
+       if (dh_yc_len > (size_t) (end - *pos)) {
                wpa_printf(MSG_DEBUG, "TLSv1: Not enough room in the "
                           "message buffer for Yc");
                tls_alert(conn, TLS_ALERT_LEVEL_FATAL,

diff --git a/src/tls/tlsv1_server_write.c b/src/tls/tlsv1_server_write.c
index 15e6692178ff80fe6585dc003683a48aa0a36192..65cda3c24c5400e49fa435e921e5b84ded24c4be 100644 (file)
--- a/src/tls/tlsv1_server_write.c
+++ b/src/tls/tlsv1_server_write.c
@@ -168,6 +168,11 @@ static int tls_write_server_certificate(struct tlsv1_server *conn,
        }
 
        pos = *msgpos;
+       if (TLS_RECORD_HEADER_LEN + 1 + 3 + 3 > end - pos) {
+               tlsv1_server_alert(conn, TLS_ALERT_LEVEL_FATAL,
+                                  TLS_ALERT_INTERNAL_ERROR);
+               return -1;
+       }
 
        tlsv1_server_log(conn, "Send Certificate");
        rhdr = pos;
@@ -188,7 +193,7 @@ static int tls_write_server_certificate(struct tlsv1_server *conn,
        pos += 3;
        cert = conn->cred->cert;
        while (cert) {
-               if (pos + 3 + cert->cert_len > end) {
+               if (3 + cert->cert_len > (size_t) (end - pos)) {
                        wpa_printf(MSG_DEBUG, "TLSv1: Not enough buffer space "
                                   "for Certificate (cert_len=%lu left=%lu)",
                                   (unsigned long) cert->cert_len,
@@ -371,7 +376,7 @@ static int tls_write_server_key_exchange(struct tlsv1_server *conn,
        /* body - ServerDHParams */
        server_params = pos;
        /* dh_p */
-       if (pos + 2 + dh_p_len > end) {
+       if (2 + dh_p_len > (size_t) (end - pos)) {
                wpa_printf(MSG_DEBUG, "TLSv1: Not enough buffer space for "
                           "dh_p");
                tlsv1_server_alert(conn, TLS_ALERT_LEVEL_FATAL,
@@ -385,7 +390,7 @@ static int tls_write_server_key_exchange(struct tlsv1_server *conn,
        pos += dh_p_len;
 
        /* dh_g */
-       if (pos + 2 + conn->cred->dh_g_len > end) {
+       if (2 + conn->cred->dh_g_len > (size_t) (end - pos)) {
                wpa_printf(MSG_DEBUG, "TLSv1: Not enough buffer space for "
                           "dh_g");
                tlsv1_server_alert(conn, TLS_ALERT_LEVEL_FATAL,
@@ -399,7 +404,7 @@ static int tls_write_server_key_exchange(struct tlsv1_server *conn,
        pos += conn->cred->dh_g_len;
 
        /* dh_Ys */
-       if (pos + 2 + dh_ys_len > end) {
+       if (2 + dh_ys_len > (size_t) (end - pos)) {
                wpa_printf(MSG_DEBUG, "TLSv1: Not enough buffer space for "
                           "dh_Ys");
                tlsv1_server_alert(conn, TLS_ALERT_LEVEL_FATAL,
@@ -457,7 +462,7 @@ static int tls_write_server_key_exchange(struct tlsv1_server *conn,
                         *   SignatureAlgorithm signature;
                         * } SignatureAndHashAlgorithm;
                         */
-                       if (hlen < 0 || pos + 2 > end) {
+                       if (hlen < 0 || end - pos < 2) {
                                tlsv1_server_alert(conn, TLS_ALERT_LEVEL_FATAL,
                                                   TLS_ALERT_INTERNAL_ERROR);
                                return -1;

diff --git a/src/tls/x509v3.c b/src/tls/x509v3.c
index b51dfcd44732027ba7f6eb8f9a980e208a8f1c43..e7b7c4115c12b3ab0a0a53935d5f1a91a53edbc8 100644 (file)
--- a/src/tls/x509v3.c
+++ b/src/tls/x509v3.c
@@ -199,12 +199,11 @@ static int x509_parse_algorithm_identifier(
                           hdr.class, hdr.tag);
                return -1;
        }
+       if (hdr.length > buf + len - hdr.payload)
+               return -1;
        pos = hdr.payload;
        end = pos + hdr.length;
 
-       if (end > buf + len)
-               return -1;
-
        *next = end;
 
        if (asn1_get_oid(pos, end - pos, &id->oid, &pos))
@@ -243,7 +242,7 @@ static int x509_parse_public_key(const u8 *buf, size_t len,
        }
        pos = hdr.payload;
 
-       if (pos + hdr.length > end)
+       if (hdr.length > end - pos)
                return -1;
        end = pos + hdr.length;
        *next = end;
@@ -319,7 +318,7 @@ static int x509_parse_name(const u8 *buf, size_t len, struct x509_name *name,
        }
        pos = hdr.payload;
 
-       if (pos + hdr.length > buf + len)
+       if (hdr.length > buf + len - pos)
                return -1;
 
        end = *next = pos + hdr.length;
@@ -677,7 +676,7 @@ static int x509_parse_validity(const u8 *buf, size_t len,
        pos = hdr.payload;
        plen = hdr.length;
 
-       if (pos + plen > buf + len)
+       if (plen > (size_t) (buf + len - pos))
                return -1;
 
        *next = pos + plen;
@@ -801,7 +800,7 @@ static int x509_parse_ext_basic_constraints(struct x509_certificate *cert,
                }
                cert->ca = hdr.payload[0];
 
-               if (hdr.payload + hdr.length == pos + len) {
+               if (hdr.length == pos + len - hdr.payload) {
                        wpa_printf(MSG_DEBUG, "X509: BasicConstraints - cA=%d",
                                   cert->ca);
                        return 0;
@@ -1503,12 +1502,12 @@ struct x509_certificate * x509_certificate_parse(const u8 *buf, size_t len)
        }
        pos = hdr.payload;
 
-       if (pos + hdr.length > end) {
+       if (hdr.length > end - pos) {
                x509_certificate_free(cert);
                return NULL;
        }
 
-       if (pos + hdr.length < end) {
+       if (hdr.length < end - pos) {
                wpa_hexdump(MSG_MSGDUMP, "X509: Ignoring extra data after DER "
                            "encoded certificate",
                            pos + hdr.length, end - (pos + hdr.length));