[PATCH] dlr_mysql.c needs to use mysql_escape_string() from mysql API

Stipe Tolj <[email protected]>
Newsgroups gmane.comp.mobile.kannel.devel
Organization tolj.org system architecture
Message-ID <[email protected]> 
Hi list,

we have a possible bug situation in the gw/dlr_mysql.c module, especially for 
the dlr_mysql_add() function:

We perform an INSERT into the table space with the values provided in dlr_entry 
struct. The field entry->source is the source address and if we used an 
alphanumeric value here containing any SQL "administrative chars" (i.e. ') we 
run into an mysql error. We need to ensure that all values passed from the 
"outside" (via smsbox HTTP interface) to the SQL creation format is passed via 
mysql's mysql_escape_string() function, ensuring such chars are escaped.

We had such a patch posted by:

   From: Peter Christensen
   Subject: Re: dlr_mysql_add and internal charset
   Date: Tue, 10 Jan 2006 07:44:02 -0800
   URL: http://www.mail-archive.com/[email protected]/msg05381.html

but it was actually never applied. This is a re-write of Peter's patch, making a 
dbpool_mysql_escape_string() wrapper function available in the 
gwlib/dbpool_mysql.c and using it in gw/dlr_mysql.c.

Please review and vote for committing to CVS.

Stipe

-------------------------------------------------------------------
Kölner Landstrasse 419
40589 DÃŒsseldorf, NRW, Germany

tolj.org system architecture      Kannel Software Foundation (KSF)
http://www.tolj.org/              http://www.kannel.org/

mailto:st_{at}_tolj.org           mailto:stolj_{at}_kannel.org
-------------------------------------------------------------------
mysql_escape_string.diff (text/plain, 8.1 KB) 
### Eclipse Workspace Patch 1.0
#P gateway-cvs-head
Index: gw/dlr_mysql.c
===================================================================
RCS file: /home/cvs/gateway/gw/dlr_mysql.c,v
retrieving revision 1.14
diff -u -r1.14 dlr_mysql.c
--- gw/dlr_mysql.c	9 Jan 2008 20:06:58 -0000	1.14
+++ gw/dlr_mysql.c	9 Aug 2008 20:29:01 -0000
@@ -83,6 +83,11 @@
  */
 static struct dlr_db_fields *fields = NULL;
 
+/*
+ * Function wrapper for mysql_escape_string().
+ */
+char *dbpool_mysql_escape_string(const char *string); 
+
 
 static void mysql_update(const Octstr *sql)
 {
@@ -143,6 +148,26 @@
 static void dlr_mysql_add(struct dlr_entry *entry)
 {
     Octstr *sql;
+    char *safe_ts, *safe_src, *safe_url, *safe_smsc;
+    char *safe_dst, *safe_srv, *safe_boxcid;
+    
+    safe_ts = safe_src = safe_url = safe_smsc = safe_dst = safe_srv = safe_boxcid = NULL;
+
+    /* ensure we handle only strings passed via mysql_escape_string() */
+    if ((safe_ts = dbpool_mysql_escape_string(octstr_get_cstr(entry->timestamp))) == NULL)
+        goto done;
+    if ((safe_src = dbpool_mysql_escape_string(octstr_get_cstr(entry->source))) == NULL) 
+        goto done;
+    if ((safe_url = dbpool_mysql_escape_string(octstr_get_cstr(entry->url))) == NULL)
+        goto done;
+    if ((safe_smsc = dbpool_mysql_escape_string(octstr_get_cstr(entry->smsc))) == NULL)
+        goto done;
+    if ((safe_dst = dbpool_mysql_escape_string(octstr_get_cstr(entry->destination))) == NULL)
+        goto done;
+    if ((safe_srv = dbpool_mysql_escape_string(octstr_get_cstr(entry->service))) == NULL)
+        goto done;
+    if ((safe_boxcid = dbpool_mysql_escape_string(octstr_get_cstr(entry->boxc_id))) == NULL)
+        goto done;
 
     sql = octstr_format("INSERT INTO %s (%s, %s, %s, %s, %s, %s, %s, %s, %s) VALUES "
                         "('%s', '%s', '%s', '%s', '%s', '%s', '%d', '%s', '%d');",
@@ -152,14 +177,24 @@
                         octstr_get_cstr(fields->field_serv), octstr_get_cstr(fields->field_url),
                         octstr_get_cstr(fields->field_mask), octstr_get_cstr(fields->field_boxc),
                         octstr_get_cstr(fields->field_status),
-                        octstr_get_cstr(entry->smsc), octstr_get_cstr(entry->timestamp), octstr_get_cstr(entry->source),
-                        octstr_get_cstr(entry->destination), octstr_get_cstr(entry->service), octstr_get_cstr(entry->url),
-                        entry->mask, octstr_get_cstr(entry->boxc_id), 0);
+                        safe_smsc, safe_ts, safe_src,
+                        safe_dst, safe_srv, safe_url,
+                        entry->mask, safe_boxcid, 0);
 
 
     mysql_update(sql);
 
     octstr_destroy(sql);
+
+done:
+    gw_free(safe_ts);
+    gw_free(safe_src);
+    gw_free(safe_url);
+    gw_free(safe_smsc);
+    gw_free(safe_dst);
+    gw_free(safe_srv);
+    gw_free(safe_boxcid);
+    
     dlr_entry_destroy(entry);
 }
 
@@ -169,31 +204,41 @@
     Octstr *sql;
     MYSQL_RES *result;
     MYSQL_ROW row;
+    char *safe_smsc, *safe_ts, *safe_dst;
+
+    safe_smsc = safe_ts = safe_dst = NULL;
+    
+    /* ensure we handle only strings passed via mysql_escape_string() */
+    if ((safe_ts = dbpool_mysql_escape_string(octstr_get_cstr(ts))) == NULL)
+        goto done;
+    if ((safe_smsc = dbpool_mysql_escape_string(octstr_get_cstr(smsc))) == NULL) 
+        goto done;
+    if ((safe_dst = dbpool_mysql_escape_string(octstr_get_cstr(dst))) == NULL)
+        goto done;
 
     sql = octstr_format("SELECT %s, %s, %s, %s, %s, %s FROM %s WHERE %s='%s' AND %s='%s';",
                         octstr_get_cstr(fields->field_mask), octstr_get_cstr(fields->field_serv),
                         octstr_get_cstr(fields->field_url), octstr_get_cstr(fields->field_src),
                         octstr_get_cstr(fields->field_dst), octstr_get_cstr(fields->field_boxc),
                         octstr_get_cstr(fields->table), octstr_get_cstr(fields->field_smsc),
-                        octstr_get_cstr(smsc), octstr_get_cstr(fields->field_ts), octstr_get_cstr(ts));
-
+                        safe_smsc, octstr_get_cstr(fields->field_ts), safe_ts);
 
     result = mysql_select(sql);
     octstr_destroy(sql);
 
     if (result == NULL) {
-        return NULL;
+        goto done;
     }
     if (mysql_num_rows(result) < 1) {
         debug("dlr.mysql", 0, "no rows found");
         mysql_free_result(result);
-        return NULL;
+        goto done;
     }
     row = mysql_fetch_row(result);
     if (!row) {
         debug("dlr.mysql", 0, "rows found but could not load them");
         mysql_free_result(result);
-        return NULL;
+        goto done;
     }
 
     debug("dlr.mysql", 0, "Found entry, row[0]=%s, row[1]=%s, row[2]=%s, row[3]=%s, row[4]=%s row[5]=%s",
@@ -211,38 +256,74 @@
 
     mysql_free_result(result);
 
+done:
+    gw_free(safe_ts);
+    gw_free(safe_smsc);
+    gw_free(safe_dst);
+
     return res;
 }
 
 static void dlr_mysql_remove(const Octstr *smsc, const Octstr *ts, const Octstr *dst)
 {
-    Octstr *sql;
+    Octstr *sql = NULL;
+    char *safe_smsc, *safe_ts, *safe_dst;
+
+    safe_smsc = safe_ts = safe_dst = NULL;
+    
+    /* ensure we handle only strings passed via mysql_escape_string() */
+    if ((safe_ts = dbpool_mysql_escape_string(octstr_get_cstr(ts))) == NULL)
+        goto done;
+    if ((safe_smsc = dbpool_mysql_escape_string(octstr_get_cstr(smsc))) == NULL) 
+        goto done;
+    if ((safe_dst = dbpool_mysql_escape_string(octstr_get_cstr(dst))) == NULL)
+        goto done;
 
     debug("dlr.mysql", 0, "removing DLR from database");
     sql = octstr_format("DELETE FROM %s WHERE %s='%s' AND %s='%s' LIMIT 1;",
                         octstr_get_cstr(fields->table), octstr_get_cstr(fields->field_smsc),
-                        octstr_get_cstr(smsc), octstr_get_cstr(fields->field_ts), octstr_get_cstr(ts));
-
+                        safe_smsc, octstr_get_cstr(fields->field_ts), safe_ts);
 
     mysql_update(sql);
 
     octstr_destroy(sql);
+
+done:
+    gw_free(safe_ts);
+    gw_free(safe_smsc);
+    gw_free(safe_dst);
 }
 
 static void dlr_mysql_update(const Octstr *smsc, const Octstr *ts, const Octstr *dst, int status)
 {
     Octstr *sql;
+    char *safe_smsc, *safe_ts, *safe_dst;
+
+    safe_smsc = safe_ts = safe_dst = NULL;
+    
+    /* ensure we handle only strings passed via mysql_escape_string() */
+    if ((safe_ts = dbpool_mysql_escape_string(octstr_get_cstr(ts))) == NULL)
+        goto done;
+    if ((safe_smsc = dbpool_mysql_escape_string(octstr_get_cstr(smsc))) == NULL) 
+        goto done;
+    if ((safe_dst = dbpool_mysql_escape_string(octstr_get_cstr(dst))) == NULL)
+        goto done;
 
     debug("dlr.mysql", 0, "updating DLR status in database");
     sql = octstr_format("UPDATE %s SET %s=%d WHERE %s='%s' AND %s='%s' LIMIT 1;",
                         octstr_get_cstr(fields->table),
                         octstr_get_cstr(fields->field_status), status,
-                        octstr_get_cstr(fields->field_smsc), octstr_get_cstr(smsc),
-                        octstr_get_cstr(fields->field_ts), octstr_get_cstr(ts));
+                        octstr_get_cstr(fields->field_smsc), safe_smsc,
+                        octstr_get_cstr(fields->field_ts), safe_ts);
 
     mysql_update(sql);
 
     octstr_destroy(sql);
+
+done:
+    gw_free(safe_ts);
+    gw_free(safe_smsc);
+    gw_free(safe_dst);
 }
 
 
Index: gwlib/dbpool_mysql.c
===================================================================
RCS file: /home/cvs/gateway/gwlib/dbpool_mysql.c,v
retrieving revision 1.8
diff -u -r1.8 dbpool_mysql.c
--- gwlib/dbpool_mysql.c	9 Jan 2008 20:06:55 -0000	1.8
+++ gwlib/dbpool_mysql.c	9 Aug 2008 20:29:02 -0000
@@ -67,6 +67,22 @@
 #include <mysql.h>
 
 
+char *dbpool_mysql_escape_string(const char *string) 
+{
+    unsigned long int size, length;
+    char *buffer;
+    length = strlen(string);
+    size = (strlen(string) << 1) + 1;
+    if ((buffer = (char*)gw_malloc(size)) == NULL) {
+        error(0, "MYSQL: Error allocating buffer for string");
+        return NULL;
+    } else {
+        mysql_escape_string(buffer, string, length);
+        return buffer;
+    }
+}
+
+
 static void *mysql_open_conn(const DBConf *db_conf)
 {
     MYSQL *mysql = NULL;
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.