Merge pull request #1782 in SNORT/snort3 from ~NIHDESAI/snort3:ftp_leak to master

Squashed commit of the following:

commit 2cf5fb38604fcb5c90504db35b0b7086dbb120ea
Author: Nihal Desai <nihdesai@cisco.com>
Date:   Mon Sep 23 08:17:46 2019 -0400

    ftp: catch invalid server command format

diff --git a/src/service_inspectors/ftp_telnet/ftp.cc b/src/service_inspectors/ftp_telnet/ftp.cc
index c90297f1c725d58c67edf33ca57d3cb7d187d6da..0350210223d65590284602846ff7a9d9afbbd239 100644 (file)
--- a/src/service_inspectors/ftp_telnet/ftp.cc
+++ b/src/service_inspectors/ftp_telnet/ftp.cc
@@ -181,114 +181,6 @@ static int snort_ftp(Packet* p)
     return FTPP_INVALID_PROTO;
 }
 
-/*
- * Function: ResetStringFormat (FTP_PARAM_FMT *Fmt)
- *
- * Purpose: Recursively sets nodes that allow strings to nodes that check
- *          for a string format attack within the FTP parameter validation tree
- *
- * Arguments: Fmt       => pointer to the FTP Parameter configuration
- *
- * Returns: None
- *
- */
-static void ResetStringFormat(FTP_PARAM_FMT* Fmt)
-{
-    int i;
-    if (!Fmt)
-        return;
-
-    if (Fmt->type == e_unrestricted)
-        Fmt->type = e_strformat;
-
-    ResetStringFormat(Fmt->optional_fmt);
-    for (i=0; i<Fmt->numChoices; i++)
-    {
-        ResetStringFormat(Fmt->choices[i]);
-    }
-    ResetStringFormat(Fmt->next_param_fmt);
-}
-
-static int ProcessFTPDataChanCmdsList(
-    FTP_SERVER_PROTO_CONF* ServerConf, const FtpCmd* fc)
-{
-    const char* cmd = fc->name.c_str();
-    int iRet;
-
-    FTP_CMD_CONF* FTPCmd =
-        ftp_cmd_lookup_find(ServerConf->cmd_lookup, cmd, strlen(cmd), &iRet);
-
-    if (FTPCmd == nullptr)
-    {
-        /* Add it to the list */
-        // note that struct includes 1 byte for null, so just add len
-        FTPCmd = (FTP_CMD_CONF*)snort_calloc(sizeof(FTP_CMD_CONF)+strlen(cmd));
-        strncpy(FTPCmd->cmd_name, cmd, strlen(cmd) + 1);
-
-        // FIXIT-L make sure pulled from server conf when used if not overridden
-        //FTPCmd->max_param_len = ServerConf->def_max_param_len;
-
-        ftp_cmd_lookup_add(ServerConf->cmd_lookup, cmd,
-            strlen(cmd), FTPCmd);
-    }
-    if ( fc->flags & CMD_DIR )
-        FTPCmd->dir_response = fc->number;
-
-    if ( fc->flags & CMD_LEN )
-    {
-        FTPCmd->max_param_len = fc->number;
-        FTPCmd->max_param_len_overridden = 1;
-    }
-    if ( fc->flags & CMD_DATA )
-        FTPCmd->data_chan_cmd = true;
-
-    if ( fc->flags & CMD_REST )
-        FTPCmd->data_rest_cmd = true;
-
-    if ( fc->flags & CMD_XFER )
-        FTPCmd->data_xfer_cmd = true;
-
-    if ( fc->flags & CMD_PUT )
-        FTPCmd->file_put_cmd = true;
-
-    if ( fc->flags & CMD_GET )
-        FTPCmd->file_get_cmd = true;
-
-    if ( fc->flags & CMD_CHECK )
-    {
-        FTP_PARAM_FMT* Fmt = FTPCmd->param_format;
-        if (Fmt)
-        {
-            ResetStringFormat(Fmt);
-        }
-        else
-        {
-            Fmt = (FTP_PARAM_FMT*)snort_calloc(sizeof(FTP_PARAM_FMT));
-            Fmt->type = e_head;
-            FTPCmd->param_format = Fmt;
-
-            Fmt = (FTP_PARAM_FMT*)snort_calloc(sizeof(FTP_PARAM_FMT));
-            Fmt->type = e_strformat;
-            FTPCmd->param_format->next_param_fmt = Fmt;
-            Fmt->prev_param_fmt = FTPCmd->param_format;
-        }
-        FTPCmd->check_validity = true;
-    }
-    if ( fc->flags & CMD_VALID )
-    {
-        char err[1024];
-        ProcessFTPCmdValidity(
-            ServerConf, cmd, fc->format.c_str(), err, sizeof(err));
-    }
-    if ( fc->flags & CMD_ENCR )
-        FTPCmd->encr_cmd = true;
-
-    if ( fc->flags & CMD_LOGIN )
-        FTPCmd->login_cmd = true;
-
-    return 0;
-}
-
 //-------------------------------------------------------------------------
 // class stuff
 //-------------------------------------------------------------------------
@@ -441,10 +333,6 @@ static Inspector* fs_ctor(Module* mod)
 {
     FtpServerModule* fsm = (FtpServerModule*)mod;
     FTP_SERVER_PROTO_CONF* conf = fsm->get_data();
-    unsigned i = 0;
-
-    while ( const FtpCmd* cmd = fsm->get_cmd(i++) )
-        ProcessFTPDataChanCmdsList(conf, cmd);
 
     return new FtpServer(conf);
 }

diff --git a/src/service_inspectors/ftp_telnet/ftp_module.cc b/src/service_inspectors/ftp_telnet/ftp_module.cc
index b18ff45933d02da7fecaaed116a32180c63293a7..65e47f9974f8a1d5668ab16b7a1f87967d09dcef 100644 (file)
--- a/src/service_inspectors/ftp_telnet/ftp_module.cc
+++ b/src/service_inspectors/ftp_telnet/ftp_module.cc
@@ -23,9 +23,11 @@
 #endif
 
 #include "ftp_module.h"
-
+#include "ftpp_return_codes.h"
+#include "ftp_cmd_lookup.h"
+#include "ftp_parse.h"
 #include "log/messages.h"
-
+#include "utils/util.h"
 #include "ft_main.h"
 #include "ftpp_si.h"
 
@@ -471,7 +473,108 @@ bool FtpServerModule::set(const char*, Value& v, SnortConfig*)
 
 //-------------------------------------------------------------------------
 
-bool FtpServerModule::begin(const char*, int, SnortConfig*)
+// Recursively sets nodes that allow strings to nodes that check
+// for a string format attack within the FTP parameter validation tree
+
+static void ResetStringFormat(FTP_PARAM_FMT* Fmt)
+{
+    int i;
+    if (!Fmt)
+        return;
+
+    if (Fmt->type == e_unrestricted)
+        Fmt->type = e_strformat;
+
+    ResetStringFormat(Fmt->optional_fmt);
+    for (i=0; i<Fmt->numChoices; i++)
+    {
+        ResetStringFormat(Fmt->choices[i]);
+    }
+    ResetStringFormat(Fmt->next_param_fmt);
+}
+
+int ProcessFTPDataChanCmdsList(
+    FTP_SERVER_PROTO_CONF* ServerConf, const FtpCmd* fc)
+{
+    const char* cmd = fc->name.c_str();
+    int iRet = 0;
+
+    FTP_CMD_CONF* FTPCmd =
+        ftp_cmd_lookup_find(ServerConf->cmd_lookup, cmd, strlen(cmd), &iRet);
+
+    if (FTPCmd == nullptr)
+    {
+        /* Add it to the list */
+        // note that struct includes 1 byte for null, so just add len
+        FTPCmd = (FTP_CMD_CONF*)snort_calloc(sizeof(FTP_CMD_CONF)+strlen(cmd));
+        strncpy(FTPCmd->cmd_name, cmd, strlen(cmd) + 1);
+
+        // FIXIT-L make sure pulled from server conf when used if not overridden
+        //FTPCmd->max_param_len = ServerConf->def_max_param_len;
+
+        ftp_cmd_lookup_add(ServerConf->cmd_lookup, cmd,
+            strlen(cmd), FTPCmd);
+        iRet = 0;
+    }
+    if ( fc->flags & CMD_DIR )
+        FTPCmd->dir_response = fc->number;
+
+    if ( fc->flags & CMD_LEN )
+    {
+        FTPCmd->max_param_len = fc->number;
+        FTPCmd->max_param_len_overridden = 1;
+    }
+    if ( fc->flags & CMD_DATA )
+        FTPCmd->data_chan_cmd = true;
+
+    if ( fc->flags & CMD_REST )
+        FTPCmd->data_rest_cmd = true;
+
+    if ( fc->flags & CMD_XFER )
+        FTPCmd->data_xfer_cmd = true;
+
+    if ( fc->flags & CMD_PUT )
+        FTPCmd->file_put_cmd = true;
+
+    if ( fc->flags & CMD_GET )
+        FTPCmd->file_get_cmd = true;
+
+    if ( fc->flags & CMD_CHECK )
+    {
+        FTP_PARAM_FMT* Fmt = FTPCmd->param_format;
+        if (Fmt)
+        {
+            ResetStringFormat(Fmt);
+        }
+        else
+        {
+            Fmt = (FTP_PARAM_FMT*)snort_calloc(sizeof(FTP_PARAM_FMT));
+            Fmt->type = e_head;
+            FTPCmd->param_format = Fmt;
+
+            Fmt = (FTP_PARAM_FMT*)snort_calloc(sizeof(FTP_PARAM_FMT));
+            Fmt->type = e_strformat;
+            FTPCmd->param_format->next_param_fmt = Fmt;
+            Fmt->prev_param_fmt = FTPCmd->param_format;
+        }
+        FTPCmd->check_validity = true;
+    }
+    if ( fc->flags & CMD_VALID && !fc->format.empty())
+    {
+        char err[1024];
+        iRet = ProcessFTPCmdValidity(
+            ServerConf, cmd, fc->format.c_str(), err, sizeof(err));
+    }
+    if ( fc->flags & CMD_ENCR )
+        FTPCmd->encr_cmd = true;
+
+    if ( fc->flags & CMD_LOGIN )
+        FTPCmd->login_cmd = true;
+
+    return iRet;
+}
+
+bool FtpServerModule::begin(const char* fqn, int, SnortConfig*)
 {
     names.clear();
     format.clear();
@@ -480,13 +583,28 @@ bool FtpServerModule::begin(const char*, int, SnortConfig*)
     if ( !conf )
         conf = new FTP_SERVER_PROTO_CONF;
 
+    if ( !strcmp(fqn, "ftp_server") )
+    {
+        for ( auto cmd : cmds )
+             delete cmd;
+
+        cmds.clear();
+    }
     return true;
 }
 
 bool FtpServerModule::end(const char* fqn, int idx, SnortConfig*)
 {
-    if ( !idx )
+
+    if ( !idx && !strcmp(fqn, "ftp_server") )
+    {
+        for( auto cmd : cmds)
+        {
+            if ( FTPP_SUCCESS !=  ProcessFTPDataChanCmdsList(conf, cmd) )
+                return false;
+        }
         return true;
+    }
 
     if ( !strcmp(fqn, "ftp_server.cmd_validity") )
         cmds.emplace_back(new FtpCmd(names, format, number));

diff --git a/src/service_inspectors/ftp_telnet/ftp_parse.cc b/src/service_inspectors/ftp_telnet/ftp_parse.cc
index fe1f71538c8fd7dff0c9bea4f5a251c07dfb9cde..e448ad6a591f7b8cddf069a10313aa34aa7292dc 100644 (file)
--- a/src/service_inspectors/ftp_telnet/ftp_parse.cc
+++ b/src/service_inspectors/ftp_telnet/ftp_parse.cc
@@ -595,6 +595,7 @@ int ProcessFTPCmdValidity(
     /* Need to check to be sure we got a complete command */
     if (iRet)
     {
+        ftpp_ui_config_reset_ftp_cmd_format(HeadFmt);
         return FTPP_FATAL_ERR;
     }