Re: ssl preprocessor incorrect event 'SSL_INVALID_CLIENT_HELLO'

[Bhagya Bantwal <bbantwal () sourcefire com>]

Bram,

Thanks for reporting this issue. This a bug in Snort. The packets 7 (CSS)
and 8(encrypted data) get reassembled and SSL identifies it as TLSv1 and
processes the handshake successfully. However packet 8 which triggered the
reassembly gets processed again and Snort erroneously tries to re-determine
the protocol from the application data.

A bug has been filed for this issue.

-Thanks
Bhagya

On Thu, Jul 11, 2013 at 9:58 AM, Bram <bram-fabeg () mail wizbit be> wrote:

> 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.
------------------------------------------------------------------------------
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!