ssl preprocessor incorrect event 'SSL_INVALID_CLIENT_HELLO'

[Bram <bram-fabeg () mail wizbit be>]

Hi,


The ssl preprocessor procduces an incorrect event for  
'SSL_INVALID_CLIENT_HELLO' on some SSL streams.
Attached is a cap file which can be used to trigger this.

snort configuration:

dynamicpreprocessor directory /usr/lib/snort_dynamicpreprocessor/
preprocessor stream5_global: \
   track_tcp yes, \
   track_udp no, \
   track_icmp no
preprocessor stream5_tcp: policy first, ports client 443

preprocessor ssl: ports { 443 465 563 636 989 992 993 994 995 7801  
7702 7900 7901 7902 7903 7904 7905 7906 6907 7908 7909 7910 7911 7912  
7913 7914 7915 7916 7917 7918 7919 7920 }, trustservers,  
noinspect_encrypted

alert ( msg: "SSL_INVALID_CLIENT_HELLO"; sid: 1; gid: 137; rev: 2;  
metadata: rule-type preproc ; )
alert ( msg: "SSL_INVALID_SERVER_HELLO"; sid: 2; gid: 137; rev: 2;  
metadata: rule-type preproc ; )

output alert_fast: stdout


Running snort:
$ snort -l /var/log -c /etc/ips/snort.conf --daq-dir /lib/daq/ -r  
/tmp/137_1_2.cap  2>&1 | grep -i ssp_ssl
07/09-16:06:03.915563  [**] [137:1:2] (ssp_ssl) Invalid Client HELLO  
after Server HELLO Detected [**] [Priority: 0] {TCP} 10.10.1.1:51843  
-> 10.10.1.2:443



Analyzing this shows that the incorrect event is generated on packet 8  
in the dump which is 'Application Data'

The problem is located in src/dynamic-preprocessors/libs/ssl.c: SSL_decode:
    /* Check if it's possibly a SSLv2 server-hello, in which case the version
     * is at byte 7 */
    else if(size >= 8 && pkt[7] == 2)
    {
        /* A version of '2' at byte 7 overlaps with TLS record-data length.
         * Check if a hypothetical TLS record-data length agress with its
         * record length */
        datalen = THREE_BYTE_LEN( (pkt+6) );

        record = (SSL_record_t*)pkt;
        reclen = ntohs(record->length);

        /* If these lengths match, it's v3 */
        /* Otherwise, it's v2 */
        if(reclen - SSL_HS_PAYLOAD_OFFSET != datalen)
            return SSL_decode_v2(pkt, size, pkt_flags);
    }

the 7th byte in the packet happens to be '02'...

printing 'reclen' and 'datalen' with gdb shows:
<pre>
(gdb) print datalen
$29 = 5440189
(gdb) print reclen
$30 = 32
</pre>

=> values don't match so it starts parsing it as SSLv2 which is not correct...

Looking further shows that 'THREE_BYTE_LEN' is defined as:
        #define THREE_BYTE_LEN(x) (x[2] | x[1] << 8 | x[0] << 16)

which makes the code:
    else if(size >= 8 && pkt[7] == 2)
    {
        /* A version of '2' at byte 7 overlaps with TLS record-data length.
         * Check if a hypothetical TLS record-data length agress with its
         * record length */
        datalen = (pkt[8] | pkt[7] << 8 | pkt[6] << 16);

Layout of the SSLv3/TLSv1 packet according to rfc2246 / rfc6101:
* byte 0: contenttype
* bytes 1, 2: version
* bytes 3, 4: length
* bytes 5-...: data

The code above makes absolutely no sense to me:
* pkt[7] is a fixed value so why is it included in the calculation?
* pkt[6], pkt[7] and pkt[8] are part of the encrypted data


The actual length of the SSL packet is stored in bytes 3 and 4 for  
SSL3/TLS but this length is already stored in reclen:
        record = (SSL_record_t*)pkt;
        reclen = ntohs(record->length);


Just for reference:
        typedef struct _SSL_record
        {
            uint8_t type;
            uint8_t major;
            uint8_t minor;
            uint16_t length;
        } SSL_record_t;

        ....


I'm completely clueless to what the code attempts to do...


Going back to the dump shows that it then attempts to parse the TLSv1  
packet as SSLv2.
The SSLv2 structure is defined as:
        typedef struct _SSLv2_record
        {
            uint16_t length;
            uint8_t type;
        } SSLv2_record_t;

What this means: the 'type' field in the SSLv2_record_t is the same as  
the 'minor' field in the SSL_record_t.
For TLSv1 the minor version is always set to 1.
When the TLSv1 records gets parsed as a SSLv2 record then it is seen  
as an SSLv2 Client Hello (SSL_V2_CHELLO is set to 1)


In SSL_decode another part of the code also looks broken - but no  
attempt was made to trigger this:
    if(pkt[4] == 2)
    {
        /* This could be a TLS server hello.  Check for a TLS version  
string */
        if(size >= 10)
        {
            if(pkt[9] == 3)
            {
               /* Saw a TLS version, but this could also be an SSHv2 length.
                 * If it is, check if a hypothetical TLS record-data  
length agress
                 * with its record length */
                datalen = THREE_BYTE_LEN( (pkt+6) );

                record = (SSL_record_t*)pkt;
                reclen = ntohs(record->length);

                /* If these lengths match, it's v3 */
                /* Otherwise, it's v2 */
                if(reclen - SSL_HS_PAYLOAD_OFFSET != datalen)
                    return SSL_decode_v2(pkt, size, pkt_flags);
            }
        }
    }

For SSL3/TLS:
* pkt[4] is part of the length
* pkt[9], pkt[6], pkt[7], pkt[8] are part of the encrypted...



In summary: the code is broken and I have no clue why it is written  
this way... the comment in the code is a bit to vague to fully  
understand what exactly it is trying to do/compare...


Can you please have a look at this?


Best regards,

Bram


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.

Attachment: 137_1_2.cap
Description:

------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk

_______________________________________________
Snort-devel mailing list
Snort-devel () lists sourceforge net
https://lists.sourceforge.net/lists/listinfo/snort-devel
Archive:
http://sourceforge.net/mailarchive/forum.php?forum_name=snort-devel

Please visit http://blog.snort.org for the latest news about Snort!