netfilter buglog - Oct 2017 - [Bug 1193] New: Incorrect malloc for SQL statements and missing strings length check

https://bugzilla.netfilter.org/show_bug.cgi?id=1193

            Bug ID: 1193
           Summary: Incorrect malloc for SQL statements and missing
                    strings length check
           Product: ulogd
           Version: SVN (please provide timestamp)
          Hardware: All
                OS: All
            Status: NEW
          Severity: major
          Priority: P5
         Component: ulogd
          Assignee: netfilter-buglog at lists.netfilter.org
          Reporter: jean at phpnet.org

I developed a filter module for ulogd2 similar to the PWSNIFF module that is
getting the hostname and URI of HTTP GET/POST requests from raw packets and i
was experiencing segfaults when long strings were passed to escape_string() as
i am inserting these values on a database.

sql_createstmt() allocates 100 bytes per key, no mater what their type and
content are.
The segfaults are caused by the fact that there is no check on the length of
strings : Values can be way longer than 100 chars and escaped values might even
be twice the length of the original string.

In my usecase case, strings can be quite long and might be triggered by users,
making it easy for end-users to crash ulogd.


This patch makes the allocation more "dynamic" for integers and safer
for
strings :

  - Integers are now reserving only the maximum possible number of bytes they
could use (eg. ULOGD_RET_INT32 lowest value is -2147483648 which is 11
characters long : it will now only allocates 11 bytes for those keys instead of
100)

  - For strings, SQL_STRINGSIZE now defines the max length of values (before
being escaped), values longer than SQL_STRINGSIZE will be set to NULL and the
double of SQL_STRINGSIZE is malloc'd in case all characters would have to be
escaped (eg. a value consisting exclusively of quotes will be twice as long
after being returned by escape_string())



It might be better to fully abort the query generation if a string is longer
than SQL_STRINGSIZE instead of setting its value to NULL but i am unsure on how
to do that properly.


Patch on the 2.0.5 codebase :
--- util/db.c    2014-03-23 16:30:50.000000000 +0100
+++ util/db.c    2017-10-02 18:09:02.069746918 +0200
@@ -57,7 +57,8 @@
 }

 #define SQL_INSERTTEMPL   "SELECT P(Y)"
-#define SQL_VALSIZE    100
+/* Maximum string length (non-escaped), will be replaced with NULL if longer
*/
+#define SQL_STRINGSIZE    255

 /* create the static part of our insert statement */
 static int sql_createstmt(struct ulogd_pluginstance *upi)
@@ -78,13 +79,35 @@
     for (i = 0; i < upi->input.num_keys; i++) {
         if (upi->input.keys[i].flags & ULOGD_KEYF_INACTIVE)
             continue;
+
+        struct ulogd_key *key = upi->input.keys[i].u.source;
+        short key_length = 4;
+
         /* we need space for the key and a comma, as well as
-         * enough space for the values */
-        size += strlen(upi->input.keys[i].name) + 1 + SQL_VALSIZE;
+         * enough space for the values (and quotes around strings) */
+        if(key->type == ULOGD_RET_STRING) {
+            /* SQL_STRINGSIZE is the max (VAR)CHAR length, *2 in case every of
its characters would be escaped and +3 for the quotes around the string and the
comma at the end */
+            ulogd_log(ULOGD_DEBUG, "allocating %d bytes for string %s of
type
%s", (SQL_STRINGSIZE * 2) + 3, key->name, type_to_string(key->type));
+            size += (SQL_STRINGSIZE * 2) + 3;
+        } else {
+            /* key_length is the maximum strlen for the specified integer type
(ex: ULOGD_RET_INT32 lowest value is -2147483648 which is 11 characters long)
*/
+            key_length = ulogd_key_size(key);
+            if(key_length < 1) {
+                /* ulogd_key_size() returns -1 for key types it does not know
*/
+                key_length = SQL_STRINGSIZE;
+                ulogd_log(ULOGD_ERROR, "%s key length cannot be
determined,
forced to %hd bytes", upi->input.keys[i].name, key_length);
+            } else {
+                key_length = 10*key_length*8/33+2;
+            }
+            ulogd_log(ULOGD_DEBUG, "allocating %hd bytes for int %s of
type
%s", key_length, upi->input.keys[i].name, type_to_string(key->type));
+
+            /* +1 for the comma at the end */
+            size += key_length + 1;
+        }
     }
     size += strlen(procedure);

-    ulogd_log(ULOGD_DEBUG, "allocating %u bytes for statement\n",
size);
+    ulogd_log(ULOGD_DEBUG, "allocating a total of %u bytes for the
statement\n", size);

     mi->stmt = (char *) malloc(size);
     if (!mi->stmt) {
@@ -373,14 +396,20 @@
             sprintf(stmt_ins, "'%d',", res->u.value.b);
             break;
         case ULOGD_RET_STRING:
-            *(stmt_ins++) = '\'';
             if (res->u.value.ptr) {
-                stmt_ins +-                di->driver->escape_string(upi,
stmt_ins,
-                              res->u.value.ptr,
-                            strlen(res->u.value.ptr));
+                if(strlen(res->u.value.ptr) > SQL_STRINGSIZE) {
+                    ulogd_log(ULOGD_ERROR, "The string for the key %s is
too
long (>%d chars), value is set to NULL", upi->input.keys[i].name,
SQL_STRINGSIZE);
+                    stmt_ins += sprintf(stmt_ins, "NULL,");
+                } else {
+                    /* the string is escaped and put between quotes */
+                    *(stmt_ins++) = '\'';
+                    stmt_ins += di->driver->escape_string(upi, stmt_ins,
res->u.value.ptr, strlen(res->u.value.ptr));
+                    stmt_ins += sprintf(stmt_ins, "\',");
+                }
+            } else {
+                ulogd_log(ULOGD_NOTICE, "No string passed for the key %s,
setting the value to NULL", upi->input.keys[i].name);
+                stmt_ins += sprintf(stmt_ins, "NULL,");
             }
-            sprintf(stmt_ins, "',");
             break;
         case ULOGD_RET_RAWSTR:
             sprintf(stmt_ins, "%s,", (char *) res->u.value.ptr);

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20171019/0f6cd1f8/attachment.html>