Hi Thierry,
On Sat, Feb 25, 2017 at 01:01:54PM +0100, thierry.fourn...@arpalert.org wrote:
> The patch implementing this idea is in attachment. It returns the
> client-hello cioher list as binary, hexadecimal string, xxh64 and with
> the decoded ciphers.
Is this supposed to be the last version ? I'm asking because it's still
bogus regarding the length calculation :
> +static inline
> +void ssl_sock_parse_clienthello(int write_p, int version, int content_type,
> + const void *buf, size_t len,
> + struct ssl_capture *capture)
> +{
> + unsigned char *msg;
> + unsigned char *end;
> + unsigned int rec_len;
> +
> + /* This function is called for "from client" and "to server"
> + * connections. The combination of write_p == 0 and content_type == 22
> + * is only avalaible during "from client" connection.
> + */
> +
> + /* "write_p" is set to 0 is the bytes are received messages,
> + * otherwise it is set to 1.
> + */
> + if (write_p != 0)
> + return;
> +
> + /* content_type contains the type of message received or sent
> + * according with the SSL/TLS protocol spec. This message is
> + * encoded with one byte. The value 256 (two bytes) is used
> + * for designing the SSL/TLS record layer. According with the
> + * rfc6101, the expected message (other than 256) are:
> + * - change_cipher_spec(20)
> + * - alert(21)
> + * - handshake(22)
> + * - application_data(23)
> + * - (255)
> + * We are interessed by the handshake and specially the client
> + * hello.
> + */
> + if (content_type != 22)
> + return;
> +
> + /* The message length is at least 4 bytes, containing the
> + * message type and the message length.
> + */
> + if (len < 4)
> + return;
> +
> + /* First byte of the handshake message id the type of
> + * message. The konwn types are:
> + * - hello_request(0)
> + * - client_hello(1)
> + * - server_hello(2)
> + * - certificate(11)
> + * - server_key_exchange (12)
> + * - certificate_request(13)
> + * - server_hello_done(14)
> + * We are interested by the client hello.
> + */
> + msg = (unsigned char *)buf;
> + if (msg[0] != 1)
> + return;
> +
> + /* Next three bytes are the length of the message. The total length
> + * must be this decoded length + 4. If the length given as argument
> + * is not the same, we abort the protocol dissector.
> + */
> + rec_len = (msg[1] << 3) + (msg[2] << 2) + msg[3];
Here. The correct statement is :
rec_len = msg[1] * 65536 + msg[2] * 256 + msg[3];
(or << 16, << 8)
> + if (len < rec_len + 4)
> + return;
> + msg += 4;
> + end = msg + rec_len;
> + if (end <= msg)
> + return;
This one looks wrong as it prevents rec_len from being NULL, the
correct overflow test is if (end < msg).
> + /* Expect 2 bytes for protocol version (1 byte for major and 1 byte
> + * for minor, the random, composed by 4 bytes for the unix time and
> + * 28 bytes for unix payload, and them 1 byte for the session id. So
> + * we jump 1 + 1 + 4 + 28 + 1 bytes.
> + */
> + msg += 1 + 1 + 4 + 28 + 1;
> + if (msg >= end)
> + return;
It seems like this one should be "if (msg > end)" given that it accounts for
a length. However given that it's covered by the next one, maybe it can
simply be dropped.
> + /* Next two bytes are the ciphersuite length. */
> + if (msg + 2 > end)
> + return;
> + rec_len = (msg[0] << 2) + msg[1];
Wrong shift again.
> + msg += 2;
> + if (msg + rec_len > end || msg + rec_len < msg)
> + return;
> +
> + /* Compute the xxh64 of the ciphersuite. */
> + capture->xxh64 = XXH64(msg, rec_len, 0);
> +
> + /* Capture the ciphersuite. */
> + capture->ciphersuite_len = rec_len;
> + if (capture->ciphersuite_len > global_ssl.capture_cipherlist)
> + capture->ciphersuite_len = global_ssl.capture_cipherlist;
> + memcpy(capture->ciphersuite, msg, capture->ciphersuite_len);
> +}
> +
The rest looks OK though. Just let me know.
Thanks,
Willy
