tls: fix hash calculations if client cert is requested and sent
authorDenys Vlasenko <vda.linux@googlemail.com>
Tue, 6 Feb 2018 12:33:00 +0000 (13:33 +0100)
committerDenys Vlasenko <vda.linux@googlemail.com>
Wed, 14 Feb 2018 16:38:40 +0000 (17:38 +0100)
Symptoms: connecting to
    openssl s_server -cert vsftpd.pem -port 990 -debug -cipher AES128-SHA
works, but with "-verify 1" option added it does not.

function                                             old     new   delta
tls_xread_record                                     474     499     +25
tls_handshake                                       1582    1607     +25
bad_record_die                                        98     110     +12
tls_run_copy_loop                                    282     293     +11
tls_xread_handshake_block                             58      51      -7
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 4/1 up/down: 73/-7)              Total: 66 bytes

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
networking/tls.c

index fd3cb0dba9aed54463befd91f87c6461eb834f6d..7936afca219ab838ad27800890d41d1b14e83ccf 100644 (file)
 # define dbg_der(...) ((void)0)
 #endif
 
-#define RECORD_TYPE_CHANGE_CIPHER_SPEC  20
-#define RECORD_TYPE_ALERT               21
-#define RECORD_TYPE_HANDSHAKE           22
-#define RECORD_TYPE_APPLICATION_DATA    23
-
-#define HANDSHAKE_HELLO_REQUEST         0
-#define HANDSHAKE_CLIENT_HELLO          1
-#define HANDSHAKE_SERVER_HELLO          2
-#define HANDSHAKE_HELLO_VERIFY_REQUEST  3
-#define HANDSHAKE_NEW_SESSION_TICKET    4
-#define HANDSHAKE_CERTIFICATE           11
-#define HANDSHAKE_SERVER_KEY_EXCHANGE   12
-#define HANDSHAKE_CERTIFICATE_REQUEST   13
-#define HANDSHAKE_SERVER_HELLO_DONE     14
-#define HANDSHAKE_CERTIFICATE_VERIFY    15
-#define HANDSHAKE_CLIENT_KEY_EXCHANGE   16
-#define HANDSHAKE_FINISHED              20
+#define RECORD_TYPE_CHANGE_CIPHER_SPEC  20 /* 0x14 */
+#define RECORD_TYPE_ALERT               21 /* 0x15 */
+#define RECORD_TYPE_HANDSHAKE           22 /* 0x16 */
+#define RECORD_TYPE_APPLICATION_DATA    23 /* 0x17 */
+
+#define HANDSHAKE_HELLO_REQUEST         0  /* 0x00 */
+#define HANDSHAKE_CLIENT_HELLO          1  /* 0x01 */
+#define HANDSHAKE_SERVER_HELLO          2  /* 0x02 */
+#define HANDSHAKE_HELLO_VERIFY_REQUEST  3  /* 0x03 */
+#define HANDSHAKE_NEW_SESSION_TICKET    4  /* 0x04 */
+#define HANDSHAKE_CERTIFICATE           11 /* 0x0b */
+#define HANDSHAKE_SERVER_KEY_EXCHANGE   12 /* 0x0c */
+#define HANDSHAKE_CERTIFICATE_REQUEST   13 /* 0x0d */
+#define HANDSHAKE_SERVER_HELLO_DONE     14 /* 0x0e */
+#define HANDSHAKE_CERTIFICATE_VERIFY    15 /* 0x0f */
+#define HANDSHAKE_CLIENT_KEY_EXCHANGE   16 /* 0x10 */
+#define HANDSHAKE_FINISHED              20 /* 0x14 */
 
 #define SSL_NULL_WITH_NULL_NULL                 0x0000
 #define SSL_RSA_WITH_NULL_MD5                   0x0001
@@ -512,10 +512,12 @@ static void bad_record_die(tls_state_t *tls, const char *expected, int len)
        bb_error_msg("got bad TLS record (len:%d) while expecting %s", len, expected);
        if (len > 0) {
                uint8_t *p = tls->inbuf;
-               while (len > 0) {
+               if (len > 99)
+                       len = 99; /* don't flood, a few lines should be enough */
+               do {
                        fprintf(stderr, " %02x", *p++);
                        len--;
-               }
+               } while (len != 0);
                fputc('\n', stderr);
        }
        xfunc_die();
@@ -671,9 +673,11 @@ static void xwrite_encrypted(tls_state_t *tls, unsigned size, unsigned type)
        // AES_128_CBC   Block      16      16     16
        // AES_256_CBC   Block      32      16     16
 
-       /* Fill IV and padding in outbuf */
        tls_get_random(buf - AES_BLOCKSIZE, AES_BLOCKSIZE); /* IV */
-       dbg("before crypt: 5 hdr + %u data + %u hash bytes\n", size, tls->MAC_size);
+       dbg("before crypt: 5 hdr + %u data + %u hash bytes\n",
+                       size - tls->MAC_size, tls->MAC_size);
+
+       /* Fill IV and padding in outbuf */
        // RFC is talking nonsense:
        //    "Padding that is added to force the length of the plaintext to be
        //    an integral multiple of the block cipher's block length."
@@ -773,7 +777,7 @@ static const char *alert_text(int code)
        return itoa(code);
 }
 
-static int tls_xread_record(tls_state_t *tls)
+static int tls_xread_record(tls_state_t *tls, const char *expected)
 {
        struct record_hdr *xhdr;
        int sz;
@@ -796,13 +800,16 @@ static int tls_xread_record(tls_state_t *tls)
                if (total >= RECHDR_LEN && target == MAX_INBUF) {
                        xhdr = (void*)tls->inbuf;
                        target = RECHDR_LEN + (0x100 * xhdr->len16_hi + xhdr->len16_lo);
-                       if (target > MAX_INBUF) {
-                               /* malformed input (too long): yell and die */
-                               tls->buffered_size = 0;
-                               tls->ofs_to_buffered = total;
-                               tls_error_die(tls);
+
+                       if (target > MAX_INBUF /* malformed input (too long) */
+                        || xhdr->proto_maj != TLS_MAJ
+                        || xhdr->proto_min != TLS_MIN
+                       ) {
+                               sz = total < target ? total : target;
+                               if (sz > 24)
+                                       sz = 24; /* don't flood */
+                               bad_record_die(tls, expected, sz);
                        }
-                       /* can also check type/proto_maj/proto_min here */
                        dbg("xhdr type:%d ver:%d.%d len:%d\n",
                                xhdr->type, xhdr->proto_maj, xhdr->proto_min,
                                0x100 * xhdr->len16_hi + xhdr->len16_lo
@@ -1137,13 +1144,11 @@ static void find_key_in_der_cert(tls_state_t *tls, uint8_t *der, int len)
 static int tls_xread_handshake_block(tls_state_t *tls, int min_len)
 {
        struct record_hdr *xhdr;
-       int len = tls_xread_record(tls);
+       int len = tls_xread_record(tls, "handshake record");
 
        xhdr = (void*)tls->inbuf;
        if (len < min_len
         || xhdr->type != RECORD_TYPE_HANDSHAKE
-        || xhdr->proto_maj != TLS_MAJ
-        || xhdr->proto_min != TLS_MIN
        ) {
                bad_record_die(tls, "handshake record", len);
        }
@@ -1195,7 +1200,9 @@ static void send_client_hello_and_alloc_hsd(tls_state_t *tls, const char *sni)
 //   0023 0000 - session_ticket
 //   000a 0008 0006001700180019 - supported_groups
 //   000b 0002 0100 - ec_point_formats
-//   000d 0016 00140401040305010503060106030301030302010203 - signature_algorithms
+//   000d 0016 0014 0401 0403 0501 0503 0601 0603 0301 0303 0201 0203 - signature_algorithms
+// wolfssl library sends this option, RFC 7627 (closes a security weakness, some servers may require it. TODO?):
+//   0017 0000 - extended master secret
        };
        struct client_hello *record;
        int len;
@@ -1354,7 +1361,7 @@ static void get_server_cert(tls_state_t *tls)
        xhdr = (void*)tls->inbuf;
        certbuf = (void*)(xhdr + 1);
        if (certbuf[0] != HANDSHAKE_CERTIFICATE)
-               tls_error_die(tls);
+               bad_record_die(tls, "certificate", len);
        dbg("<< CERTIFICATE\n");
        // 4392 bytes:
        // 0b  00|11|24 00|11|21 00|05|b0 30|82|05|ac|30|82|04|94|a0|03|02|01|02|02|11|00|9f|85|bf|66|4b|0c|dd|af|ca|50|86|79|50|1b|2b|e4|30|0d...
@@ -1611,6 +1618,7 @@ void FAST_FUNC tls_handshake(tls_state_t *tls, const char *sni)
        //                      <-------             Finished
        // Application Data     <------>     Application Data
        int len;
+       int got_cert_req;
 
        send_client_hello_and_alloc_hsd(tls, sni);
        get_server_hello(tls);
@@ -1638,7 +1646,8 @@ void FAST_FUNC tls_handshake(tls_state_t *tls, const char *sni)
                len = tls_xread_handshake_block(tls, 4);
        }
 
-       if (tls->inbuf[RECHDR_LEN] == HANDSHAKE_CERTIFICATE_REQUEST) {
+       got_cert_req = (tls->inbuf[RECHDR_LEN] == HANDSHAKE_CERTIFICATE_REQUEST);
+       if (got_cert_req) {
                dbg("<< CERTIFICATE_REQUEST\n");
                // RFC 5246: "If no suitable certificate is available,
                // the client MUST send a certificate message containing no
@@ -1647,7 +1656,9 @@ void FAST_FUNC tls_handshake(tls_state_t *tls, const char *sni)
                // Client certificates are sent using the Certificate structure
                // defined in Section 7.4.2."
                // (i.e. the same format as server certs)
-               send_empty_client_cert(tls);
+
+               /*send_empty_client_cert(tls); - WRONG (breaks handshake hash calc) */
+               /* need to hash _all_ server replies first, up to ServerHelloDone */
                len = tls_xread_handshake_block(tls, 4);
        }
 
@@ -1657,6 +1668,9 @@ void FAST_FUNC tls_handshake(tls_state_t *tls, const char *sni)
        // 0e 000000 (len:0)
        dbg("<< SERVER_HELLO_DONE\n");
 
+       if (got_cert_req)
+               send_empty_client_cert(tls);
+
        send_client_key_exchange(tls);
 
        send_change_cipher_spec(tls);
@@ -1667,7 +1681,7 @@ void FAST_FUNC tls_handshake(tls_state_t *tls, const char *sni)
        send_client_finished(tls);
 
        /* Get CHANGE_CIPHER_SPEC */
-       len = tls_xread_record(tls);
+       len = tls_xread_record(tls, "switch to encrypted traffic");
        if (len != 1 || memcmp(tls->inbuf, rec_CHANGE_CIPHER_SPEC, 6) != 0)
                bad_record_die(tls, "switch to encrypted traffic", len);
        dbg("<< CHANGE_CIPHER_SPEC\n");
@@ -1685,9 +1699,9 @@ void FAST_FUNC tls_handshake(tls_state_t *tls, const char *sni)
        }
 
        /* Get (encrypted) FINISHED from the server */
-       len = tls_xread_record(tls);
+       len = tls_xread_record(tls, "'server finished'");
        if (len < 4 || tls->inbuf[RECHDR_LEN] != HANDSHAKE_FINISHED)
-               tls_error_die(tls);
+               bad_record_die(tls, "'server finished'", len);
        dbg("<< FINISHED\n");
        /* application data can be sent/received */
 
@@ -1763,7 +1777,7 @@ void FAST_FUNC tls_run_copy_loop(tls_state_t *tls)
                if (pfds[1].revents) {
                        dbg("NETWORK HAS DATA\n");
  read_record:
-                       nread = tls_xread_record(tls);
+                       nread = tls_xread_record(tls, "encrypted data");
                        if (nread < 1) {
                                /* TLS protocol has no real concept of one-sided shutdowns:
                                 * if we get "TLS EOF" from the peer, writes will fail too
@@ -1775,7 +1789,7 @@ void FAST_FUNC tls_run_copy_loop(tls_state_t *tls)
                                break;
                        }
                        if (tls->inbuf[0] != RECORD_TYPE_APPLICATION_DATA)
-                               bb_error_msg_and_die("unexpected record type %d", tls->inbuf[0]);
+                               bad_record_die(tls, "encrypted data", nread);
                        xwrite(STDOUT_FILENO, tls->inbuf + RECHDR_LEN, nread);
                        /* We may already have a complete next record buffered,
                         * can process it without network reads (and possible blocking)