@henningw commented on this pull request.

Some generic comments, which should be easy to fix:

• Good code structure and good error handling
• rename the "security." file as well to secfilter., including the doc sources
• rename the security_rpc.* files as well to secfilter_rpc.*
• Fix all the remaining security strings in code and logs

Some more detailed comments, which are harder to fix:

I would suggest to unify all the different check_sqli* functions into one, that is then called with a simple pseudo-variable. This way you save a lot of implementation effort, and your function can easily work with all kind of headers. These different functions are in the end almost identical. I have also added some more review comments to the respective functions. If you don't like this for some reason, then at least unify the functions that the common parts are not redundant, e.g. implement a common sqli_check utility function which is then called from all the different functions. If you need more input about this approach, let me know.

I did not understand how the w_check* functions actually work. You seem to not do something with the SIP msg structure, or did I understood it wrong?

What happens if you start the module, it loads data from the database and strdup all strings. Then you execute a reload RPC comment, it will load the data again and again strdup all strings, or I am wrong? This way you will have a big memory leak. You need to cleanup in the reload the string memory that you allocated earlier.

There is also a logic error related to the memory handling. If you allocate the arrays in shm memory, you can't allocate the strings with malloc (via strdup). There are utility functions in the core to copy a string to shm memory.

A last comment, If I understood it correctly, your arrays are allocated with a fixed size of 1024. Then you should detect this in the db_load function, and stop loading after the arrays are full (with an error). Otherwise you will overrun the memory area. You should also document this limitation in the README.

Again, if you need some more feedback or some hints, we can also discuss on our sr-dev developer list.

In src/modules/secfilter/secfilter_hdr.c:

> + */
+
+#include <string.h>
+
+#include "../../core/dprint.h"
+#include "../../core/mem/mem.h"
+#include "../../core/parser/parse_uri.h"
+#include "../../core/parser/parse_param.h"
+#include "../../core/parser/parse_from.h"
+#include "../../core/parser/parse_to.h"
+#include "../../core/parser/contact/parse_contact.h"
+#include "../../core/sr_module.h"
+#include "secfilter.h"
+
+
+int count_chars(char *val, char c);

forward definition not needed here

In src/modules/secfilter/secfilter_hdr.c:

> +
+			if (strstr(val, "'") || strstr(val, "\"") || strstr(val, "--") || strstr(val, "=") || strstr(val, "#") || strstr(val, "+") || strstr(val, "%27") || strstr(val, "%24") || strstr(val, "%60"))
+			{
+				LM_ERR("Possible SQLi detected in to domain (%s)\n", val);
+				pkg_free(val);
+				return 0;
+			}
+	        }
+	}
+
+	if (val) pkg_free(val);
+	return 1;
+}
+
+/* Search for illegal characters in From header */
+int check_sqli_from(struct sip_msg *msg)

unify to one function, that uses $fn, $fd PVs in the kamailio configuration

In src/modules/secfilter/secfilter_hdr.c:

> +			if (strstr(val, "'") || strstr(val, "\"") || strstr(val, "--") || strstr(val, "=") || strstr(val, "#") || strstr(val, "+") || strstr(val, "%27") || strstr(val, "%24") || strstr(val, "%60"))
+			{
+				LM_ERR("Possible SQLi detected in from domain (%s)\n", val);
+				pkg_free(val);
+				return 0;
+			}
+	        }
+	}
+
+	if (val) pkg_free(val);
+	return 1;
+}
+
+
+/* Search for illegal characters in Contact header */
+int check_sqli_contact(struct sip_msg *msg)

See above, could be done with PVs and transformations: $ct{uri.domain}, $ct{uri.user}

In src/modules/secfilter/secfilter_hdr.c:

> +int count_chars(char *val, char c)
+{
+    int cont = 0;
+    int i;
+    
+    for (i = 0; i < strlen(val); i++)
+    {     
+        if (val[i] == c) cont++;
+    }
+
+    return cont;
+}
+
+
+/* Search for illegal characters in User-agent header */
+int check_sqli_ua(struct sip_msg *msg)

See below, PV $ua

In src/modules/secfilter/secfilter_hdr.c:

> +	val[msg->user_agent->body.len] = '\0';
+	
+	if (strstr(val, "'") || strstr(val, "\"") || strstr(val, "--") || strstr(val, "=") || strstr(val, "#") || strstr(val, "%27") || strstr(val, "%24") || strstr(val, "%60"))
+	{
+		LM_ERR("User-agent header (%s) has illegal characters. Posible SQLi\n", val);
+		pkg_free(val);
+		return 0;
+	}
+
+	if (val) pkg_free(val);
+	return 1;
+}
+
+
+/* Search for illegal characters in To header */
+int check_sqli_to(struct sip_msg *msg)

See below, there is a PV also for To header $tu

In src/modules/secfilter/security.c:

> +};
+
+/* Exported module parameters */
+static param_export_t params[]={
+        {"db_url",          PARAM_STRING, &db_url},
+        {"table_name",      PARAM_STR, &table_name },
+        {"action_col",      PARAM_STR, &action_col },
+        {"type_col",        PARAM_STR, &type_col },
+        {"data_col",        PARAM_STR, &data_col },
+        {"dst_exact_match", PARAM_INT, &dst_exact_match },
+        {0, 0, 0}
+};
+
+/* Module exports definition */
+struct module_exports exports={
+	"security",		/* module name */

Old name

In src/modules/secfilter/security.c:

> +void uppercase(char *sPtr)
+{
+	while(*sPtr != '\0')
+	{
+		*sPtr = toupper((unsigned char) *sPtr);
+		++sPtr;
+	}
+}
+
+
+/* Module init function */
+static int mod_init(void)
+{
+	int i;
+
+	LM_INFO("SECURITY module init\n");

Name

In src/modules/secfilter/security.c:

> +
+
+/* Module child init function */
+static int child_init(int rank)
+{
+        if (rank==PROC_INIT || rank==PROC_MAIN || rank==PROC_TCP_MAIN)
+                return 0; /* do nothing for the main process */
+
+	return 1;
+}
+
+
+/* Module destroy function */
+static void mod_destroy(void)
+{
+	LM_INFO("SECURITY module destroy\n");

Name

In src/modules/secfilter/security_rpc.c:

> +#include <string.h>
+
+#include "security.h"
+
+
+/* RPC commands */
+
+/* Add blacklist values to database */
+void rpc_add_bl(rpc_t *rpc, void *ctx)
+{
+	char *type;
+	char *value;
+
+        if(rpc->scan(ctx, "ss", (char*)(&type), (char*)(&value))<2)
+        {
+	        rpc->fault(ctx, 0, "Invalid Parameters. Usage: security.add_bl type value\n     Example: security.add_bl user sipvicious");

Name

In src/modules/secfilter/security_rpc.c:

> +		{
+			rpc->rpl_printf(ctx, "Error insert values in blacklist table");
+		}
+	}
+}
+
+
+/* Add whitelist values to database */
+void rpc_add_wl(rpc_t *rpc, void *ctx)
+{
+	char *type;
+	char *value;
+
+        if(rpc->scan(ctx, "ss", (char*)(&type), (char*)(&value))<2)
+        {
+	        rpc->fault(ctx, 0, "Invalid Parameters. Usage: security.add_wl type value\n     Example: security.add_wl user trusted_user");

Name

In src/modules/secfilter/secfilter.h:

> @@ -0,0 +1,64 @@
+#ifndef _SECURITY_H

Name

In src/modules/secfilter/security.c:

> +static int w_check_ua(struct sip_msg* msg, char *val);
+static int w_check_domain(struct sip_msg* msg, char *val);
+static int w_check_country(struct sip_msg* msg, char *val);
+static int w_check_user(struct sip_msg* msg, char *val);
+static int w_check_ip(struct sip_msg* msg, char *val);
+static int w_check_dst(struct sip_msg* msg, char *val);
+static int w_check_sqli(struct sip_msg* msg);
+
+/* Database variables */
+db_func_t db_funcs;       /* Database API functions */
+db1_con_t* db_handle=0;   /* Database connection handle */
+
+/* Exported module parameters - default values */
+int dst_exact_match = 1;
+str db_url = {NULL, 0};
+str table_name = str_init("security");

table name

In src/modules/secfilter/security.c:

> +
+/* RPC exported commands */
+static const char *rpc_reload_doc[2] = {
+	"Reload values from database", NULL};
+
+static const char *rpc_print_doc[2] = {
+	"Print values from database", NULL};
+
+static const char *rpc_add_bl_doc[2] = {
+	"Add new values to blacklist", NULL};
+
+static const char *rpc_add_wl_doc[2] = {
+	"Add new values to whitelist", NULL};
+
+rpc_export_t security_rpc[] = {
+        { "security.reload",  rpc_reload, rpc_reload_doc, 0},

Command names

In src/modules/secfilter/security_hdr.c:

> +#include "../../core/dprint.h"
+#include "../../core/mem/mem.h"
+#include "../../core/parser/parse_uri.h"
+#include "../../core/parser/parse_param.h"
+#include "../../core/parser/parse_from.h"
+#include "../../core/parser/parse_to.h"
+#include "../../core/parser/contact/parse_contact.h"
+#include "../../core/sr_module.h"
+#include "security.h"
+
+
+int count_chars(char *val, char c);
+
+
+/* Count chars of a string */
+int count_chars(char *val, char c)

add a prefix here as well, there is already a count_chars() in another module

In src/modules/secfilter/security_hdr.c:

> + */
+
+#include <string.h>
+
+#include "../../core/dprint.h"
+#include "../../core/mem/mem.h"
+#include "../../core/parser/parse_uri.h"
+#include "../../core/parser/parse_param.h"
+#include "../../core/parser/parse_from.h"
+#include "../../core/parser/parse_to.h"
+#include "../../core/parser/contact/parse_contact.h"
+#include "../../core/sr_module.h"
+#include "security.h"
+
+
+int count_chars(char *val, char c);

forward definition not needed here

In src/modules/secfilter/secfilter_hdr.c:

> +#include "../../core/dprint.h"
+#include "../../core/mem/mem.h"
+#include "../../core/parser/parse_uri.h"
+#include "../../core/parser/parse_param.h"
+#include "../../core/parser/parse_from.h"
+#include "../../core/parser/parse_to.h"
+#include "../../core/parser/contact/parse_contact.h"
+#include "../../core/sr_module.h"
+#include "secfilter.h"
+
+
+int count_chars(char *val, char c);
+
+
+/* Count chars of a string */
+int count_chars(char *val, char c)

redundant implementation, move this to a header file

In src/modules/secfilter/secfilter_db.c:

> +		LM_DBG("No data found in database\n");
+		if (db_res!=NULL && db_funcs.free_result(db_handle, db_res) < 0)
+			LM_DBG("Failed to free the result\n");
+		db_funcs.close(db_handle);
+		return 0;
+	}
+
+	/* Add values to array */
+	for (i=0; i<rows; i++)
+	{
+		/* Blacklist */
+		if (action == 0)
+		{
+			if (!strcmp(ctype, "ua"))
+			{
+			        sec_bl_ua_list[*sec_nblUa]=strtok(strdup((char*)RES_ROWS(db_res)[i].values[0].val.string_val), "\n"); 

strdup works with malloc, we need here shm_malloc as well

In src/modules/secfilter/secfilter_db.c:

> +
+	/* Add values to array */
+	for (i=0; i<rows; i++)
+	{
+		/* Blacklist */
+		if (action == 0)
+		{
+			if (!strcmp(ctype, "ua"))
+			{
+			        sec_bl_ua_list[*sec_nblUa]=strtok(strdup((char*)RES_ROWS(db_res)[i].values[0].val.string_val), "\n"); 
+			        uppercase(sec_bl_ua_list[*sec_nblUa]);
+		        	*sec_nblUa = *sec_nblUa + 1;
+		        }
+		        if (!strcmp(ctype, "country"))
+			{
+			        sec_bl_country_list[*sec_nblCountry]=strtok(strdup((char*)RES_ROWS(db_res)[i].values[0].val.string_val), "\n"); 

See above, for all strupd() calls

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.