[Snort-devel] ssh preprocessor does not whitelist ssh connections

Bhagya Bantwal bbantwal at ...402...
Tue Sep 10 10:36:38 EDT 2013


Florian,

Thanks for reporting this issue. Comments inline.

-B


On Wed, Aug 21, 2013 at 12:11 PM, Florian Westphal <
florian.westphal at ...3285...> wrote:

> Seems like the ssh preprocessor has problems decoding all the
> ssh key exchange messages (snort 2.9.5.3) - the flow does not
> enter the "encrypted" stage.
>
> I've put a reproducer pcap here: http://strlen.de/fw/ssh3.pcap
> (the invalid checksums are due to tcp offloading, snort runs with
>  checksumming verification disabled).
>
> I have following stream5 settings:
>
> preprocessor stream5_tcp: policy windows, detect_anomalies, require_3whs
> 180, \
>    overlap_limit 10, small_segments 3 bytes 150, timeout 180, \
>    ports client 21
>    ports both 80
>
> Adding 22 to "client" or "both" does not change the result.
>
> I've tried to debug this, and the main problem seems to be that
> without "both 22" it fails to see the server key exchange init
> because this packet is too big and will be part of two tcp packets.
>
> With "both 22", it fails to detect the client key exchange init since
> that message will follow the client ssh banner in one single packet.
>
> Also, it looks like there are a couple of other problems.
> The ssh preprocessor uses STREAM_FLPOLICY_SET_APPEND, but that is ignored
> silently by stream5.
>

Yep. This should have been  ABSOLUTE.

>
> Fixing this caused followup bug, looks like
> _dpd.streamAPI->missed_packets()
> is always be true when reasm is enabled mid-stream.
>
>
-- Yes. This is a bug we are looking into.



> I hacked up a patch that fixes _this_particular_scenario_, perhaps
> it helps.  Main point of this exercise is to get snort to issue a
> Whitelist verdict - I am working on a nfqueue-based DAQ that can use
> this for setting "don't queue this flow anymore" information in the kernel.
>
> If you need more information please let me know.
>
> Cheers,
> ---
>  src/dynamic-preprocessors/ssh/spp_ssh.c |  167
> +++++++++++++++++++------------
>  src/dynamic-preprocessors/ssh/spp_ssh.h |    1 +
>  2 files changed, 103 insertions(+), 65 deletions(-)
>
> diff --git a/src/dynamic-preprocessors/ssh/spp_ssh.c
> b/src/dynamic-preprocessors/ssh/spp_ssh.c
> index 01d8a3b..31518b4 100644
> --- a/src/dynamic-preprocessors/ssh/spp_ssh.c
> +++ b/src/dynamic-preprocessors/ssh/spp_ssh.c
> @@ -93,10 +93,10 @@ static void FreeSSHData( void* );
>  static void  ParseSSHArgs(SSHConfig *, u_char*);
>  static void ProcessSSH( void*, void* );
>  static inline int CheckSSHPort( uint16_t );
> -static int ProcessSSHProtocolVersionExchange( SSHData*, SFSnortPacket*,
> +static unsigned int ProcessSSHProtocolVersionExchange( SSHData*,
> SFSnortPacket*,
>                 uint8_t, uint8_t );
> -static int ProcessSSHKeyExchange( SSHData*, SFSnortPacket*, uint8_t );
> -static int ProcessSSHKeyInitExchange( SSHData*, SFSnortPacket*, uint8_t );
> +static void ProcessSSHKeyExchange( SSHData*, SFSnortPacket*, uint8_t );
> +static int ProcessSSHKeyInitExchange( SSHData*, SFSnortPacket*, uint8_t,
> unsigned int);
>  static void _addPortsToStream5Filter(struct _SnortConfig *, SSHConfig *,
> tSfPolicyId);
>  #ifdef TARGET_BASED
>  static void _addServicesToStream5Filter(struct _SnortConfig *,
> tSfPolicyId);
> @@ -499,6 +499,37 @@ DisplaySSHConfig(SSHConfig *config)
>         _dpd.logMsg("\n");
>  }
>
> +/* Returns the true length of the ssh packet, including
> + * the ssh packet header and all padding.
> + *
> + * If the packet length is invalid, 0 is returned.
> + * The return value is never larger than buflen.
> + *
> + * PARAMETERS:
> + * p: Pointer to the SSH packet.
> + * buflen: the size of packet buffer.
> + */
> +static unsigned int SSHPacket_GetLength(SSH2Packet *p, size_t buflen)
> +{
> +       unsigned int ssh_length;
> +
> +       if (buflen < sizeof(*p))
> +               return 0;
> +
> +       ssh_length = ntohl(p->packet_length);
> +
> +       if (ssh_length < sizeof(*p) + 1 || ssh_length >
> SSH2_PACKET_MAX_SIZE)
> +               return 0;
> +       ssh_length += sizeof(p->packet_length); /* not included in
> p->packet_length */
> +
> +       if (buflen < ssh_length)
> +               return buflen; /* truncated */
> +       /* buflen > ssh_length is ok.
> +        * We might have another ssh packet following this one in
> reassembled packet buffers
> +        */
> +       return ssh_length;
> +}
> +
>  /* Main runtime entry point for SSH preprocessor.
>   * Analyzes SSH packets for anomalies/exploits.
>   *
> @@ -534,7 +565,7 @@ ProcessSSH( void* ipacketp, void* contextp )
>      /* Make sure this preprocessor should run. */
>      /* check if we're waiting on stream reassembly */
>      if ( packetp->flags & FLAG_STREAM_INSERT )
> -        return;
> +        return;
>
>      PREPROC_PROFILE_START(sshPerfStats);
>
> @@ -617,11 +648,10 @@ ProcessSSH( void* ipacketp, void* contextp )
>      if (sessp->state_flags & SSH_FLG_MISSED_PACKETS)
>          return;
>
> -    /* If we picked up mid-stream or missed any packets (midstream pick up
> -     * means we've already missed packets) set missed packets flag and
> make
> -     * sure we don't do any more reassembly on this session */
> -    if ((_dpd.streamAPI->get_session_flags(packetp->stream_session_ptr) &
> SSNFLAG_MIDSTREAM)
> -            ||
> _dpd.streamAPI->missed_packets(packetp->stream_session_ptr, SSN_DIR_BOTH))
> +    /* If we picked up mid-stream (means we've already missed packets) set
> +     * missed packets flag and make sure we don't do any more reassembly
> on
> +     * this session */
> +    if (_dpd.streamAPI->get_session_flags(packetp->stream_session_ptr) &
> SSNFLAG_MIDSTREAM)
>      {
>          /* Don't turn off reassembly if autodetected since another
> preprocessor
>           * may actually be looking at this session as well and the SSH
> @@ -634,7 +664,6 @@ ProcessSSH( void* ipacketp, void* contextp )
>          }
>
>          sessp->state_flags |= SSH_FLG_MISSED_PACKETS;
> -
>          return;
>      }
>
> @@ -642,7 +671,7 @@ ProcessSSH( void* ipacketp, void* contextp )
>      if ( !(sessp->state_flags & SSH_FLG_REASSEMBLY_SET ))
>      {
>          _dpd.streamAPI->set_reassembly(packetp->stream_session_ptr,
> -                        STREAM_FLPOLICY_FOOTPRINT, SSN_DIR_BOTH,
> STREAM_FLPOLICY_SET_APPEND);
> +                        STREAM_FLPOLICY_FOOTPRINT, SSN_DIR_BOTH,
> STREAM_FLPOLICY_SET_ABSOLUTE);
>          sessp->state_flags |= SSH_FLG_REASSEMBLY_SET;
>      }
>
> @@ -652,21 +681,23 @@ ProcessSSH( void* ipacketp, void* contextp )
>
>         if ( !(sessp->state_flags & SSH_FLG_SESS_ENCRYPTED ))
>         {
> +               unsigned int offset = 0;
>                 /* If server and client have not performed the protocol
>                  * version exchange yet, must look for version strings.
>                  */
>                 if ( (sessp->state_flags & SSH_FLG_BOTH_IDSTRING_SEEN)
>                         != SSH_FLG_BOTH_IDSTRING_SEEN )
>                 {
> -                       if ( ProcessSSHProtocolVersionExchange( sessp,
> -                                       packetp, direction, known_port ) ==
> -                               SSH_FAILURE )
> +                       offset = ProcessSSHProtocolVersionExchange( sessp,
> packetp, direction, known_port);
> +                       if (offset == 0)
>                         {
>                                 /*Error processing protovers exchange msg
> */
> +                               PREPROC_PROFILE_END(sshPerfStats);
> +                               return;
>                         }
> -
> -            PREPROC_PROFILE_END(sshPerfStats);
> -                       return;
> +                       /* found protocol version.  Stream reassembly
> might have appended an ssh packet,
> +                        * such as the key exchange init.  Thus call
> ProcessSSHKeyInitExchange() too.
> +                        */
>                 }
>
>                 /* Expecting to see the key init exchange at this point
> @@ -677,10 +708,10 @@ ProcessSSH( void* ipacketp, void* contextp )
>                      ((sessp->state_flags & SSH_FLG_V2_KEXINIT_DONE )
>                         != SSH_FLG_V2_KEXINIT_DONE ))
>                 {
> -                   ProcessSSHKeyInitExchange( sessp, packetp, direction );
> +                       ProcessSSHKeyInitExchange( sessp, packetp,
> direction, offset);
>
>              PREPROC_PROFILE_END(sshPerfStats);
> -                       return;
> +            return;
>                 }
>
>                 /* If SSH2, need to process the actual key exchange msgs.
> @@ -759,7 +790,6 @@ ProcessSSH( void* ipacketp, void* contextp )
>                                 packetp->stream_session_ptr,
>                                 packetp,
>                                 SSN_DIR_BOTH, -1, 0 );
> -
>                 }
>
>         }
> @@ -919,15 +949,16 @@ static inline int SSHCheckStrlen(char *str, int max)
> {
>   * direction:  Which direction the packet is going.
>   * known_port:  A pre-configured or default server port is involved.
>   *
> - * RETURNS:    SSH_SUCCESS, if successfully processed a proto exch msg
> - *             SSH_FAILURE, otherwise.
> + * RETURNS:    returns length of protocol version string
> + *             0 if no version exchange was found.
>   */
> -static int
> +static unsigned int
>  ProcessSSHProtocolVersionExchange( SSHData* sessionp, SFSnortPacket*
> packetp,
>         uint8_t direction, uint8_t known_port )
>  {
>         char* version_stringp = (char*) packetp->payload;
>         uint8_t version;
> +       char *version_end;
>
>         /* Get the version. */
>         if ( packetp->payload_size >= 6 &&
> @@ -977,7 +1008,7 @@ ProcessSSHProtocolVersionExchange( SSHData* sessionp,
> SFSnortPacket* packetp,
>              ALERT(SSH_EVENT_PROTOMISMATCH, SSH_EVENT_PROTOMISMATCH_STR);
>                 }
>
> -               return SSH_FAILURE;
> +               return 0;
>         }
>
>         /* Saw a valid protocol exchange message. Mark the session
> @@ -994,8 +1025,12 @@ ProcessSSHProtocolVersionExchange( SSHData*
> sessionp, SFSnortPacket* packetp,
>         }
>
>         sessionp->version = version;
> +       version_end = memchr(version_stringp, '\n', packetp->payload_size);
> +       if (version_end)
> +               return (version_end - version_stringp) + 1;
>
> -       return SSH_SUCCESS;
> +       /* incomplete version string, should end with \n or \r\n for sshv2
> */
> +       return 6;
>  }
>
>  /* Called to process SSH1 key exchange or SSH2 key exchange init
> @@ -1007,15 +1042,24 @@ ProcessSSHProtocolVersionExchange( SSHData*
> sessionp, SFSnortPacket* packetp,
>   * sessionp:    Pointer to SSH data for packet's session.
>   * packetp:    Pointer to the packet to inspect.
>   * direction:  Which direction the packet is going.
> + * offset:     Offset in payload buffer where we expect the first
> SSH2Packet
>   *
>   * RETURN:     SSH_SUCCESS, if a valid key exchange message is processed
>   *             SSH_FAILURE, otherwise.
>   */
>  static int
>  ProcessSSHKeyInitExchange( SSHData* sessionp, SFSnortPacket* packetp,
> -       uint8_t direction )
> +       uint8_t direction, unsigned int offset)
>  {
>         SSH2Packet* ssh2packetp = NULL;
> +       unsigned int payload_size = packetp->payload_size;
> +       const char *payload = packetp->payload;
> +
> +       if (payload_size < sizeof(*ssh2packetp) || (payload_size -
> sizeof(*ssh2packetp)) < offset)
> +               return SSH_FAILURE;
> +
> +       payload_size -= offset;
> +       payload += offset;
>
>         if ( sessionp->version == SSH_VERSION_1 )
>         {
> @@ -1023,30 +1067,16 @@ ProcessSSHKeyInitExchange( SSHData* sessionp,
> SFSnortPacket* packetp,
>                 uint8_t padding_length;
>                 uint8_t message_type;
>
> -           /*
> -         * Validate packet payload.
> -         * First 4 bytes should have the SSH packet length,
> -         * minus any padding.
> -         */
> -               if ( packetp->payload_size < 4 )
> -        {
> -            if(ssh_eval_config->EnabledAlerts & SSH_ALERT_PAYSIZE)
> -            {
> -                ALERT(SSH_EVENT_PAYLOAD_SIZE, SSH_PAYLOAD_SIZE_STR);
> -            }
> -
> -                       return SSH_FAILURE;
> -        }
> -
>                 /*
>                  * SSH1 key exchange is very simple and
>                  * consists of only two messages, a server
>                  * key and a client key message.`
>                  */
> -               length = ntohl( *((uint32_t*) packetp->payload) );
> +               memcpy(&length, payload, sizeof(length));
> +               length = ntohl(length);
>
>             /* Packet payload should be larger than length, due to
> padding. */
> -               if ( packetp->payload_size < length )
> +               if (payload_size < length )
>                 {
>              if(ssh_eval_config->EnabledAlerts & SSH_ALERT_PAYSIZE)
>              {
> @@ -1062,9 +1092,9 @@ ProcessSSHKeyInitExchange( SSHData* sessionp,
> SFSnortPacket* packetp,
>           * With the padding calculated, verify payload is sufficiently
> large
>           * to include the message type.
>           */
> -        if ( packetp->payload_size < padding_length + 4 + 1)
> +        if ( payload_size < padding_length + 4 + 1 + offset)
>          {
> -            if(ssh_eval_config->EnabledAlerts & SSH_ALERT_PAYSIZE)
> +            if(offset == 0 && ssh_eval_config->EnabledAlerts &
> SSH_ALERT_PAYSIZE)
>              {
>                  ALERT(SSH_EVENT_PAYLOAD_SIZE, SSH_PAYLOAD_SIZE_STR);
>              }
> @@ -1073,7 +1103,7 @@ ProcessSSHKeyInitExchange( SSHData* sessionp,
> SFSnortPacket* packetp,
>          }
>
>                 message_type =
> -                    *( (uint8_t*) (packetp->payload + padding_length +
> 4));
> +                    *( (uint8_t*) (payload + padding_length + 4));
>
>                 switch( message_type )
>                 {
> @@ -1124,22 +1154,20 @@ ProcessSSHKeyInitExchange( SSHData* sessionp,
> SFSnortPacket* packetp,
>           * This may legitimately occur such as in the case of a
>           * retransmission.
>           */
> -        if ( packetp->payload_size < sizeof(SSH2Packet) )
> +        if ( payload_size < sizeof(SSH2Packet) )
>          {
>                         return SSH_FAILURE;
>          }
>
>                 /* Overlay the SSH2 binary data packet struct on the
> packet */
> -               ssh2packetp = (SSH2Packet*) packetp->payload;
> -               if (( packetp->payload_size < SSH2_HEADERLEN + 1) ||
> -                       ( packetp->payload_size <
> ntohl(ssh2packetp->packet_length) ))
> +               ssh2packetp = (SSH2Packet*) payload;
> +               if ( payload_size < SSH2_HEADERLEN + 1)
>                 {
>                         /* Invalid packet length. */
> -
>                         return SSH_FAILURE;
>                 }
>
> -               switch ( packetp->payload[SSH2_HEADERLEN] )
> +               switch ( payload[SSH2_HEADERLEN] )
>                 {
>                         case SSH_MSG_KEXINIT:
>                                 sessionp->state_flags |=
> @@ -1175,38 +1203,40 @@ ProcessSSHKeyInitExchange( SSHData* sessionp,
> SFSnortPacket* packetp,
>   * sessionp:    Pointer to SSH data for packet's session.
>   * packetp:    Pointer to the packet to inspect.
>   * direction:  Which direction the packet is going.
> - *
> - * RETURN:     SSH_SUCCESS, if a valid key exchange message is processed
> - *             SSH_FAILURE, otherwise.
>   */
> -static int
> +static void
>  ProcessSSHKeyExchange( SSHData* sessionp, SFSnortPacket* packetp,
>         uint8_t direction )
>  {
>         SSH2Packet* ssh2packetp = NULL;
> +       unsigned int offset = 0;
> +       unsigned int payload_size = packetp->payload_size;
> +       unsigned int ssh_length;
>
> -    if ( packetp->payload_size < sizeof(SSH2Packet) )
> +    if ( payload_size < sizeof(SSH2Packet) )
>      {
>                 /* Invalid packet length. */
> -               return SSH_FAILURE;
> +               return;
>      }
>
> -       ssh2packetp = (SSH2Packet*) packetp->payload;
> + next_ssh_packet:
> +       ssh2packetp = (SSH2Packet*) (packetp->payload + offset);
> +       ssh_length = SSHPacket_GetLength(ssh2packetp, payload_size);
>
> -       if (( packetp->payload_size < SSH2_HEADERLEN + 1 ) ||
> -               ( packetp->payload_size <
> ntohl(ssh2packetp->packet_length) ))
> +
> +       if (ssh_length == 0)
>         {
>
> +
>          if(ssh_eval_config->EnabledAlerts & SSH_ALERT_PAYSIZE)
>          {
>                     /* Invalid packet length. */
>              ALERT(SSH_EVENT_PAYLOAD_SIZE, SSH_PAYLOAD_SIZE_STR);
>          }
> -
> -               return SSH_FAILURE;
> +               return;
>         }
>
> -       switch( packetp->payload[SSH2_HEADERLEN] )
> +       switch(packetp->payload[offset + SSH2_HEADERLEN] )
>         {
>                 case SSH_MSG_KEXDH_INIT:
>                         if ( direction == SSH_DIR_FROM_CLIENT )
> @@ -1306,9 +1336,16 @@ ProcessSSHKeyExchange( SSHData* sessionp,
> SFSnortPacket* packetp,
>         {
>                 sessionp->state_flags |=
>                         SSH_FLG_SESS_ENCRYPTED;
> +
> +               return;
>         }
>
> -       return SSH_SUCCESS;
> +       if (ssh_length < payload_size && ssh_length >= 4)
> +       {
> +               offset += ssh_length;
> +               payload_size -= ssh_length;
> +               goto next_ssh_packet;
> +       }
>  }
>
>  static void _addPortsToStream5Filter(struct _SnortConfig *sc, SSHConfig
> *config, tSfPolicyId policy_id)
> diff --git a/src/dynamic-preprocessors/ssh/spp_ssh.h
> b/src/dynamic-preprocessors/ssh/spp_ssh.h
> index 6b31b4c..a88d031 100644
> --- a/src/dynamic-preprocessors/ssh/spp_ssh.h
> +++ b/src/dynamic-preprocessors/ssh/spp_ssh.h
> @@ -191,6 +191,7 @@ typedef struct _sshData
>   * Length of SSH2 header, in bytes.
>   */
>  #define SSH2_HEADERLEN         (5)
> +#define SSH2_PACKET_MAX_SIZE    (256 * 1024)
>
>  /*
>   * SSH2 binary packet struct.
> --
> 1.7.8.6
>
>
>
> ------------------------------------------------------------------------------
> Introducing Performance Central, a new site from SourceForge and
> AppDynamics. Performance Central is your source for news, insights,
> analysis and resources for efficient Application Performance Management.
> Visit us today!
> http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
> _______________________________________________
> Snort-devel mailing list
> Snort-devel at 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!
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.snort.org/pipermail/snort-devel/attachments/20130910/63cb2a83/attachment.html>


More information about the Snort-devel mailing list