]> git.ipfire.org Git - thirdparty/snort3.git/commitdiff
Merge pull request #726 in SNORT/snort3 from appid_ptypes_scan_patch to master
authorHui Cao (huica) <huica@cisco.com>
Tue, 29 Nov 2016 20:00:21 +0000 (15:00 -0500)
committerHui Cao (huica) <huica@cisco.com>
Tue, 29 Nov 2016 20:00:21 +0000 (15:00 -0500)
Squashed commit of the following:

commit cfbad0aea0e04b034f7bcd70d07de6fcfc36dc73
Author: davis mcpherson <davmcphe.cisco.com>
Date:   Mon Nov 28 10:50:23 2016 -0500

    delete auto ptr used to point to member variable of the AppIdSession class, just use the member variable directly.  Some coding style improvements

commit 616baeb1eae80e6d4954c0a1e85db4d34085a13e
Author: davis mcpherson <davmcphe.cisco.com>
Date:   Mon Nov 28 15:06:26 2016 -0500

    make ptype_scan_counts a field of the httpSession struct

commit fef9bdf71276aa9b8966609c49743f6df3136bcd
Author: davis mcpherson <davmcphe.cisco.com>
Date:   Mon Nov 28 14:41:55 2016 -0500

    Add mutex lock around calls into crypto lib for X509 cert processing, crypto lib not thread safe so calls into this lib from multiple packet threads appear to be causing random failures

src/network_inspectors/appid/appid_session.cc
src/network_inspectors/appid/appid_session.h
src/network_inspectors/appid/detector_plugins/detector_http.cc
src/network_inspectors/appid/service_plugins/service_ssl.cc

index 1d48f9e9ebdafefdc31a8181467c98741865226d..6625a98ae4bca95c194c438b5a4fb38cec9bb42c 100644 (file)
@@ -426,8 +426,6 @@ bool AppIdSession::is_packet_ignored(Packet* p)
     return false;
 }
 
-static int ptype_scan_counts[NUMBER_OF_PTYPES];
-
 #ifdef REMOVED_WHILE_NOT_IN_USE
 static inline bool checkThirdPartyReinspect(const Packet* p, AppIdSession* asd)
 {
@@ -477,7 +475,8 @@ void AppIdSession::ProcessThirdPartyResults(Packet* p, int confidence,
         if (!hsession)
         {
             hsession = (httpSession*)snort_calloc(sizeof(httpSession));
-            memset(ptype_scan_counts, 0, 7 * sizeof(ptype_scan_counts[0]));
+            memset(hsession->ptype_scan_counts, 0,
+                NUMBER_OF_PTYPES * sizeof(hsession->ptype_scan_counts[0]));
         }
 
         if (get_session_flags(APPID_SESSION_SPDY_SESSION))
@@ -773,7 +772,7 @@ void AppIdSession::ProcessThirdPartyResults(Packet* p, int confidence,
             attribute_data->httpResponseContent = nullptr;
             scan_flags |= SCAN_HTTP_CONTENT_TYPE_FLAG;
         }
-        if (ptype_scan_counts[LOCATION_PT] && attribute_data->httpResponseLocation)
+        if (hsession->ptype_scan_counts[LOCATION_PT] && attribute_data->httpResponseLocation)
         {
             if (hsession->location)
             {
@@ -800,7 +799,7 @@ void AppIdSession::ProcessThirdPartyResults(Packet* p, int confidence,
             hsession->req_body_buflen = attribute_data->httpRequestBodyLen;
             attribute_data->httpRequestBody = nullptr;
         }
-        if (ptype_scan_counts[BODY_PT] && attribute_data->httpResponseBody)
+        if (hsession->ptype_scan_counts[BODY_PT] && attribute_data->httpResponseBody)
         {
             if (hsession->body)
             {
@@ -3156,7 +3155,7 @@ int AppIdSession::initial_CHP_sweep(char** chp_buffers, uint16_t* chp_buffer_len
       ***************************************************************/
     for (unsigned i = 0; i < NUMBER_OF_PTYPES; i++)
     {
-        ptype_scan_counts[i] = cah->ptype_scan_counts[i];
+        hsession->ptype_scan_counts[i] = cah->ptype_scan_counts[i];
         hsession->ptype_req_counts[i] = cah->ptype_req_counts[i] +
             cah->ptype_rewrite_insert_used[i];
         if (i > 3 && !cah->ptype_scan_counts[i]
@@ -3175,17 +3174,17 @@ int AppIdSession::initial_CHP_sweep(char** chp_buffers, uint16_t* chp_buffer_len
 
     if (thirdparty_appid_module)
     {
-        if ((ptype_scan_counts[CONTENT_TYPE_PT]))
+        if ((hsession->ptype_scan_counts[CONTENT_TYPE_PT]))
             thirdparty_appid_module->session_attr_set(tpsession, TP_ATTR_COPY_RESPONSE_CONTENT);
         else
             thirdparty_appid_module->session_attr_clear(tpsession, TP_ATTR_COPY_RESPONSE_CONTENT);
 
-        if ((ptype_scan_counts[LOCATION_PT]))
+        if ((hsession->ptype_scan_counts[LOCATION_PT]))
             thirdparty_appid_module->session_attr_set(tpsession, TP_ATTR_COPY_RESPONSE_LOCATION);
         else
             thirdparty_appid_module->session_attr_clear(tpsession, TP_ATTR_COPY_RESPONSE_LOCATION);
 
-        if ((ptype_scan_counts[BODY_PT]))
+        if ((hsession->ptype_scan_counts[BODY_PT]))
             thirdparty_appid_module->session_attr_set(tpsession, TP_ATTR_COPY_RESPONSE_BODY);
         else
             thirdparty_appid_module->session_attr_clear(tpsession, TP_ATTR_COPY_RESPONSE_BODY);
@@ -3212,36 +3211,29 @@ void AppIdSession::clearMiscHttpFlags()
 
 void AppIdSession::processCHP(char** version, Packet* p)
 {
-    int i;
-    int found_in_buffer = 0;
-    char* user = nullptr;
-    AppId chp_final;
-    AppId ret = 0;
-    httpSession* http_session = hsession;
-
     char* chp_buffers[NUMBER_OF_PTYPES] =
     {
-        http_session->useragent,
-        http_session->host,
-        http_session->referer,
-        http_session->uri,
-        http_session->cookie,
-        http_session->req_body,
-        http_session->content_type,
-        http_session->location,
-        http_session->body,
+        hsession->useragent,
+        hsession->host,
+        hsession->referer,
+        hsession->uri,
+        hsession->cookie,
+        hsession->req_body,
+        hsession->content_type,
+        hsession->location,
+        hsession->body,
     };
 
     uint16_t chp_buffer_lengths[NUMBER_OF_PTYPES] = {
-        http_session->useragent_buflen,
-        http_session->host_buflen,
-        http_session->referer_buflen,
-        http_session->uri_buflen,
-        http_session->cookie_buflen,
-        http_session->req_body_buflen,
-        http_session->content_type_buflen,
-        http_session->location_buflen,
-        http_session->body_buflen,
+        hsession->useragent_buflen,
+        hsession->host_buflen,
+        hsession->referer_buflen,
+        hsession->uri_buflen,
+        hsession->cookie_buflen,
+        hsession->req_body_buflen,
+        hsession->content_type_buflen,
+        hsession->location_buflen,
+        hsession->body_buflen,
     };
 
     char* chp_rewritten[NUMBER_OF_PTYPES] =
@@ -3258,97 +3250,99 @@ void AppIdSession::processCHP(char** version, Packet* p)
         nullptr,nullptr,nullptr
     };
 
-    if (http_session->chp_hold_flow)
-        http_session->chp_finished = 0;
+    if ( hsession->chp_hold_flow )
+        hsession->chp_finished = 0;
 
-    if (!http_session->chp_candidate)
+    if ( !hsession->chp_candidate )
     {
         // remove artifacts from previous matches before we start again.
-        for (i = 0; i < NUMBER_OF_PTYPES; i++)
-        {
-            if (http_session->new_field[i])
+        for (unsigned i = 0; i < NUMBER_OF_PTYPES; i++)
+            if (hsession->new_field[i])
             {
-                snort_free(http_session->new_field[i]);
-                http_session->new_field[i] = nullptr;
+                snort_free(hsession->new_field[i]);
+                hsession->new_field[i] = nullptr;
             }
-        }
 
-        if (!initial_CHP_sweep(chp_buffers, chp_buffer_lengths, chp_matches))
-            http_session->chp_finished = 1; // this is a failure case.
+        if ( !initial_CHP_sweep(chp_buffers, chp_buffer_lengths, chp_matches) )
+            hsession->chp_finished = 1; // this is a failure case.
     }
 
-    if (!http_session->chp_finished && http_session->chp_candidate)
+    if ( !hsession->chp_finished && hsession->chp_candidate )
     {
-        for (i = 0; i < NUMBER_OF_PTYPES; i++)
+        char* user = nullptr;
+
+        for (unsigned i = 0; i < NUMBER_OF_PTYPES; i++)
         {
-            if ( !ptype_scan_counts[i] )
+            if ( !hsession->ptype_scan_counts[i] )
                 continue;
 
             if ( chp_buffers[i] && chp_buffer_lengths[i] )
             {
-                found_in_buffer = 0;
-                ret = scan_chp((PatternType)i, chp_buffers[i], chp_buffer_lengths[i], chp_matches[i], version, &user,
-                               &chp_rewritten[i], &found_in_buffer, http_session, p);
+                int found_in_buffer = 0;
+                AppId ret = scan_chp((PatternType)i, chp_buffers[i], chp_buffer_lengths[i], chp_matches[i], version, &user,
+                               &chp_rewritten[i], &found_in_buffer, hsession, p);
                 chp_matches[i] = nullptr; // freed by scanCHP()
-                http_session->total_found += found_in_buffer;
-                if (!ret || found_in_buffer < http_session->ptype_req_counts[i])
+                hsession->total_found += found_in_buffer;
+                if (!ret || found_in_buffer < hsession->ptype_req_counts[i])
                 {
                     // No match at all or the required matches for the field was NOT made
-                    if (!http_session->num_matches)
+                    if (!hsession->num_matches)
                     {
                         // num_matches == 0 means: all must succeed
                         // give up early
-                        http_session->chp_candidate = 0;
+                        hsession->chp_candidate = 0;
                         break;
                     }
                 }
             }
             else
             {
-                if (!http_session->num_matches)
+                if ( !hsession->num_matches )
                 {
                     // num_matches == 0 means: all must succeed  give up early
-                    http_session->chp_candidate = 0;
+                    hsession->chp_candidate = 0;
                     break;
                 }
             }
 
             // Decrement the expected scan count toward 0.
-            ptype_scan_counts[i] = 0;
-            http_session->num_scans--;
+            hsession->ptype_scan_counts[i] = 0;
+            hsession->num_scans--;
             // if we have reached the end of the list of scans (which have something to do), then num_scans == 0
-            if (http_session->num_scans == 0)
+            if (hsession->num_scans == 0)
             {
                 // we finished the last scan
                 // either the num_matches value was zero and we failed early-on or we need to check for the min.
-                if (http_session->num_matches &&
-                                http_session->total_found < http_session->num_matches)
+                if (hsession->num_matches &&
+                                hsession->total_found < hsession->num_matches)
                 {
                     // There was a minimum scans match count (num_matches != 0)
                     // And we did not reach that minimum
-                    http_session->chp_candidate = 0;
+                    hsession->chp_candidate = 0;
                     break;
                 }
                 // All required matches were met.
-                http_session->chp_finished = 1;
+                hsession->chp_finished = 1;
                 break;
             }
         }
 
-        if (!http_session->chp_candidate)
+        if ( !hsession->chp_candidate )
         {
-            http_session->chp_finished = 1;
-            if (*version)
+            hsession->chp_finished = 1;
+            if ( *version )
             {
                 snort_free(*version);
                 *version = nullptr;
             }
-            if (user)
+
+            if ( user )
             {
                 snort_free(user);
                 user = nullptr;
             }
-            for (i = 0; i < NUMBER_OF_PTYPES; i++)
+
+            for (unsigned i = 0; i < NUMBER_OF_PTYPES; i++)
             {
                 if (nullptr != chp_rewritten[i])
                 {
@@ -3356,77 +3350,78 @@ void AppIdSession::processCHP(char** version, Packet* p)
                     chp_rewritten[i] = nullptr;
                 }
             }
-            memset(ptype_scan_counts, 0, 7 * sizeof(ptype_scan_counts[0]));
+            memset(hsession->ptype_scan_counts, 0, NUMBER_OF_PTYPES * sizeof(int));
 
             // Make it possible for other detectors to run.
-            http_session->skip_simple_detect = false;
+            hsession->skip_simple_detect = false;
             return;
         }
-        if (http_session->chp_candidate && http_session->chp_finished)
+
+        if (hsession->chp_candidate && hsession->chp_finished)
         {
-            chp_final = http_session->chp_alt_candidate ? http_session->chp_alt_candidate :
-                            CHP_APPIDINSTANCE_TO_ID(http_session->chp_candidate);
+            AppId chp_final = hsession->chp_alt_candidate ? hsession->chp_alt_candidate
+                            : CHP_APPIDINSTANCE_TO_ID(hsession->chp_candidate);
 
-            if (http_session->app_type_flags & APP_TYPE_SERVICE)
+            if (hsession->app_type_flags & APP_TYPE_SERVICE)
                 set_service_appid_data(chp_final, nullptr, version);
 
-            if (http_session->app_type_flags & APP_TYPE_CLIENT)
+            if (hsession->app_type_flags & APP_TYPE_CLIENT)
                 set_client_app_id_data(chp_final, version);
 
-            if (http_session->app_type_flags & APP_TYPE_PAYLOAD)
+            if ( hsession->app_type_flags & APP_TYPE_PAYLOAD )
                 set_payload_app_id_data((ApplicationId)chp_final, version);
 
-            if (*version)
+            if ( *version )
                 *version = nullptr;
-            if (user)
+
+            if ( user )
             {
                 username = user;
                 user = nullptr;
-                if (http_session->app_type_flags & APP_TYPE_SERVICE)
+                if (hsession->app_type_flags & APP_TYPE_SERVICE)
                     username_service = chp_final;
                 else
                     username_service = serviceAppId;
                 set_session_flags(APPID_SESSION_LOGIN_SUCCEEDED);
             }
 
-            for (i = 0; i < NUMBER_OF_PTYPES; i++)
-            {
-                if (nullptr != chp_rewritten[i])
+            for (unsigned i = 0; i < NUMBER_OF_PTYPES; i++)
+                if ( chp_rewritten[i] )
                 {
                     if (session_logging_enabled)
                         LogMessage("AppIdDbg %s rewritten %s: %s\n", session_logging_id,
                             httpFieldName[i], chp_rewritten[i]);
-                    if (http_session->new_field[i])
-                        snort_free(http_session->new_field[i]);
-                    http_session->new_field[i] = chp_rewritten[i];
-                    http_session->new_field_contents = true;
+                    if (hsession->new_field[i])
+                        snort_free(hsession->new_field[i]);
+                    hsession->new_field[i] = chp_rewritten[i];
+                    hsession->new_field_contents = true;
                     chp_rewritten[i] = nullptr;
                 }
-            }
-            http_session->chp_candidate = 0;
+
+            hsession->chp_candidate = 0;
             //if we're doing safesearch rewrites, we want to continue to hold the flow
-            if (!http_session->get_offsets_from_rebuilt)
-                http_session->chp_hold_flow = 0;
+            if (!hsession->get_offsets_from_rebuilt)
+                hsession->chp_hold_flow = 0;
             scan_flags &= ~SCAN_HTTP_VIA_FLAG;
             scan_flags &= ~SCAN_HTTP_USER_AGENT_FLAG;
             scan_flags &= ~SCAN_HTTP_HOST_URL_FLAG;
-            memset(ptype_scan_counts, 0, 7 * sizeof(ptype_scan_counts[0]));
+            memset(hsession->ptype_scan_counts, 0,
+                NUMBER_OF_PTYPES * sizeof(hsession->ptype_scan_counts[0]));
         }
         else /* if we have a candidate, but we're not finished */
         {
-            if (user)
+            if ( user )
             {
                 snort_free(user);
                 user = nullptr;
             }
-            for (i = 0; i < NUMBER_OF_PTYPES; i++)
-            {
+
+            for (unsigned i = 0; i < NUMBER_OF_PTYPES; i++)
                 if (nullptr != chp_rewritten[i])
                 {
                     snort_free(chp_rewritten[i]);
                     chp_rewritten[i] = nullptr;
                 }
-            }
         }
     }
 }
@@ -3445,8 +3440,7 @@ int AppIdSession::process_http_packet(int direction)
     AppId client_id = 0;
     AppId payload_id = 0;
 
-    httpSession* http_session = hsession;
-    if (!http_session)
+    if (!hsession)
     {
         clear_app_id_data();
         if (session_logging_enabled)
@@ -3458,10 +3452,10 @@ int AppIdSession::process_http_packet(int direction)
 
     // For fragmented HTTP headers, do not process if none of the fields are set.
     // These fields will get set when the HTTP header is reassembled.
-    if ((!http_session->useragent) && (!http_session->host) && (!http_session->referer) &&
-        (!http_session->uri))
+    if ((!hsession->useragent) && (!hsession->host) && (!hsession->referer) &&
+        (!hsession->uri))
     {
-        if (!http_session->skip_simple_detect)
+        if (!hsession->skip_simple_detect)
             clearMiscHttpFlags();
 
         return 0;
@@ -3470,10 +3464,10 @@ int AppIdSession::process_http_packet(int direction)
     if (direction == APP_ID_FROM_RESPONDER && !get_session_flags(
         APPID_SESSION_RESPONSE_CODE_CHECKED))
     {
-        if (http_session->response_code)
+        if (hsession->response_code)
         {
             set_session_flags(APPID_SESSION_RESPONSE_CODE_CHECKED);
-            if (http_session->response_code_buflen != RESPONSE_CODE_LENGTH)            {
+            if (hsession->response_code_buflen != RESPONSE_CODE_LENGTH)            {
                 /* received bad response code. Stop processing this session */
                 clear_app_id_data();
                 if (session_logging_enabled)
@@ -3483,7 +3477,7 @@ int AppIdSession::process_http_packet(int direction)
             }
         }
 #if RESPONSE_CODE_PACKET_THRESHHOLD
-        else if (++(http_session->response_code_packets) == RESPONSE_CODE_PACKET_THRESHHOLD)
+        else if (++(hsession->response_code_packets) == RESPONSE_CODE_PACKET_THRESHHOLD)
         {
             set_session_flags(APPID_SESSION_RESPONSE_CODE_CHECKED);
             /* didn't receive response code in first X packets. Stop processing this asd */
@@ -3495,11 +3489,11 @@ int AppIdSession::process_http_packet(int direction)
         }
 #endif
     }
-    char* host = http_session->host;
-    char* url = http_session->url;
-    char* via = http_session->via;
-    char* useragent = http_session->useragent;
-    char* referer = http_session->referer;
+    char* host = hsession->host;
+    char* url = hsession->url;
+    char* via = hsession->via;
+    char* useragent = hsession->useragent;
+    char* referer = hsession->referer;
     memset(&hmp, 0, sizeof(hmp));
 
     if (serviceAppId == APP_ID_NONE)
@@ -3507,12 +3501,12 @@ int AppIdSession::process_http_packet(int direction)
 
     if (session_logging_enabled)
         LogMessage("AppIdDbg %s chp_finished %d chp_hold_flow %d\n", session_logging_id,
-            http_session->chp_finished, http_session->chp_hold_flow);
+            hsession->chp_finished, hsession->chp_hold_flow);
 
-    if (!http_session->chp_finished || http_session->chp_hold_flow)
+    if (!hsession->chp_finished || hsession->chp_hold_flow)
         processCHP(&version, nullptr);
 
-    if (!http_session->skip_simple_detect)  // true if processCHP found match
+    if (!hsession->skip_simple_detect)  // true if processCHP found match
     {
         if (!get_session_flags(APPID_SESSION_APP_REINSPECT))
         {
@@ -3568,14 +3562,14 @@ int AppIdSession::process_http_packet(int direction)
 
             // Scan User-Agent for Browser types or Skype
             if ((scan_flags & SCAN_HTTP_USER_AGENT_FLAG) && client_app_id <= APP_ID_NONE
-                    && useragent && http_session->useragent_buflen)
+                    && useragent && hsession->useragent_buflen)
             {
                 if (version)
                 {
                     snort_free(version);
                     version = nullptr;
                 }
-                identify_user_agent((uint8_t*)useragent, http_session->useragent_buflen,
+                identify_user_agent((uint8_t*)useragent, hsession->useragent_buflen,
                     &service_id, &client_id, &version);
                 if (session_logging_enabled && service_id > APP_ID_NONE &&
                         service_id != APP_ID_HTTP && serviceAppId != service_id)
index 7819728f3edcb591daf6bbe00ea0196d47489046..25c132cc6f35beb174d62fd3cdb7daa39bc38d92 100644 (file)
@@ -180,6 +180,7 @@ struct httpSession
     sfip_t* xffAddr = nullptr;
     const char** xffPrecedence = nullptr;
     int numXffFields = 0;
+    int ptype_scan_counts[NUMBER_OF_PTYPES] = { 0 };
 
 #if RESPONSE_CODE_PACKET_THRESHHOLD
     unsigned response_code_packets = 0;
index 847913c0c310cde034eda647dc6aa9023c45384a..a40b651c82361aafadc52480b3349a834d5bfa68 100644 (file)
@@ -1104,7 +1104,7 @@ static char* normalize_userid(char* user)
 
 static void extractCHP(char* buf, int bs, int start, int psize, char* adata,  char** outbuf)
 {
-    char* begin = buf+start+psize;
+    char* begin = buf + start + psize;
     char* end = nullptr;
     char* tmp;
     int i, as;
index b79e58d7f1c470bdcce5c710c043aac44f297602..fea8b6073465fc7f8f13745f8ca7b7c601820350 100644 (file)
@@ -23,6 +23,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <stddef.h>
+#include <mutex>
 #include <sys/types.h>
 #include <netinet/in.h>
 #include <openssl/x509.h>
@@ -195,6 +196,7 @@ struct ServiceSslConfig
 };
 
 static THREAD_LOCAL ServiceSslConfig service_ssl_config;
+static std::mutex crypto_lib_mutex;
 
 #pragma pack()
 
@@ -503,7 +505,10 @@ static int parse_certificates(ServiceSSLData* ss)
                 success = 0;
                 break;
             }
+            crypto_lib_mutex.lock();
             cert = d2i_X509(nullptr, (const unsigned char**)&data, cert_len);
+            crypto_lib_mutex.unlock();
+
             len -= cert_len;    /* Above call increments data pointer already. */
             if (!cert)
             {
@@ -610,12 +615,13 @@ static int parse_certificates(ServiceSSLData* ss)
         ss->org_name_strlen    = org_name_tot_len - 1;       /* Minus terminator. */
 
 parse_certificates_clean:
-
         while (certs_head)
         {
             certs_curr = certs_head;
             certs_head = certs_head->next;
+            crypto_lib_mutex.lock();
             X509_free(certs_curr->cert);
+            crypto_lib_mutex.unlock();
             snort_free(certs_curr);
         }