Re: [ dspam-Bug Tracker-3023888 ] Wrong escaping of bytea (signature data) with postgresql 8.4

Stevan Bajić <[email protected]>
Newsgroups gmane.mail.spam.dspam.devel
Message-ID <[email protected]> 
On Mon, 23 Aug 2010 11:28:45 +0200
Stevan Bajić <[email protected]> wrote:

> On Mon, 23 Aug 2010 10:56:19 +0200
> Marcin Mirosław <[email protected]> wrote:
> 
> > W dniu 23.08.2010 09:53, Stevan Bajić pisze:
> > > Okay. Let's say I want to fix the PostgreSQL driver to take care of that setting.
> > > So either I have to check for 'standard_conforming_strings' and look if it is set to 'on' or I write every text with the E'' notation and don't care about 'standard_conforming_strings' bein 'on' or 'off'. 
> > 
> > Sorry, i didn't read all thread. "standard_conforming_strings=on/off"
> > can be set in any moment of connection to pgsql, let dspam set it on/off
> > at the start, and go on with proper escaping.
> >
> Most data is just INT, BIGINT or DATE.
> 
> In three tables we use VARCHAR:
> - In 'dspam_signature_data' for 'signature'
> - In 'dspam_preferences' for 'preference' and 'value'
> - In 'dspam_virtual_uids' for 'username'
> 
I made a quick patch that is escaping the above VARCHAR's. I have not tested the patch nor compiled DSPAM with the patch. I don't know if the patch is working or not? Can any one using DSPAM GIT and PostgreSQL try if the patch is working as expected?


> In one table we use BYTEA:
> - In 'dspam_signature_data' for 'data'
> 
'data' in 'dspam_signature_data' is anyway escaped when writing and unescaped when read. So currently there is no need to do anything regarding escaping/unescaping. Right?


> So the only place where we need to escape/unescape is the places where we use VARCHAR and BYTEA.
> 
> We could avoid escaping the VARCHAR for the table dspam_signature_data since that 'signature' will probably never have a character in it that would need escaping. In the 'dspam_preferences' table the 'preference' will too not have a character that needs escaping. But 'value' from 'dspam_preferences' could have characters that would need escaping/unescaping and the 'username' from 'dspam_virtual_uids' could have characters that need escaping/unescaping. The 'data' from 'dspam_signature_data' will probably always need to use E'' notation.
> 
> So IMHO the PostgreSQL driver should not fiddle around with 'standard_conforming_strings' and either enforce everywhere the E'' notation (would slow down the driver because of escaping/unescaping) or just use escaping/unescaping where we anyway can't go without (dspam_signature_data.data, dspam_virtual_uids.username and dspam_preferences.value).
> 
> What do you think about this approach?
> 
> > Regards,
> > Marcin
> > 
> -- 
> Kind Regards from Switzerland,
> 
> Stevan Bajić
> 
> 
> > -- 
> > xmpp (jabber): marcin  [at]  mejor.pl
> > www: http://blog.mejor.pl/
> > 
> 
-- 
Kind Regards from Switzerland,

Stevan Bajić

------------------------------------------------------------------------------
This SF.net email is sponsored by 

Make an app they can't live without
Enter the BlackBerry Developer Challenge
http://p.sf.net/sfu/RIM-dev2dev

_______________________________________________
Dspam-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/dspam-devel
3023888.patch (application/octet-stream, 12.4 KB) 
diff --git a/src/pgsql_drv.c b/src/pgsql_drv.c
index ab0fcc1..f5133c0 100644
--- a/src/pgsql_drv.c
+++ b/src/pgsql_drv.c
@@ -1,4 +1,4 @@
-/* $Id: pgsql_drv.c,v 1.748 2010/08/04 23:54:52 sbajic Exp $ */
+/* $Id: pgsql_drv.c,v 1.749 2010/08/23 11:49:47 sbajic Exp $ */
 
 /*
  DSPAM
@@ -1390,6 +1390,9 @@ _ds_get_signature (DSPAM_CTX * CTX, struct _ds_spam_signature *SIG,
   char query[256];
   PGresult *result;
   int uid = -1;
+  char *sig_esc = NULL;
+  int pgerror;
+  size_t pgsize;
 
   if (s->dbh == NULL)
   {
@@ -1451,10 +1454,24 @@ _ds_get_signature (DSPAM_CTX * CTX, struct _ds_spam_signature *SIG,
     uid = (int) p->pw_uid;
   }
 
+  /* escape the signature */
+  sig_esc = malloc(strlen(signature)*2+1);
+  if (sig_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    return EFAILURE;
+  }
+  pgsize = PQescapeStringConn (s->dbh, sig_esc, signature, strlen(signature), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_ds_get_signature: unable to escape signature '%s'", signature);
+    free(sig_esc);
+    return EFAILURE;
+  }
+
   snprintf (query, sizeof (query),
-            "SELECT data,length FROM dspam_signature_data WHERE uid=%d AND signature='%s'",
-            (int) uid, signature);
+            "SELECT data,length FROM dspam_signature_data WHERE uid=%d AND signature=E'%s'",
+            (int) uid, sig_esc);
 
+  free(sig_esc);
   result = PQexec(s->dbh, query);
   if ( !result || (PQresultStatus(result) != PGRES_TUPLES_OK && PQresultStatus(result) != PGRES_NONFATAL_ERROR) )
   {
@@ -1517,6 +1534,9 @@ _ds_set_signature (DSPAM_CTX * CTX, struct _ds_spam_signature *SIG,
   PGresult *result;
   struct passwd *p;
   char *name;
+  char *sig_esc = NULL;
+  int pgerror;
+  size_t pgsize;
 
   if (s->dbh == NULL)
   {
@@ -1552,9 +1572,23 @@ _ds_set_signature (DSPAM_CTX * CTX, struct _ds_spam_signature *SIG,
     mem = PQescapeBytea(SIG->data, SIG->length, &length);
   }
 
+  /* escape the signature */
+  sig_esc = malloc(strlen(signature)*2+1);
+  if (sig_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    return EFAILURE;
+  }
+  pgsize = PQescapeStringConn (s->dbh, sig_esc, signature, strlen(signature), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_ds_get_signature: unable to escape signature '%s'", signature);
+    free(sig_esc);
+    return EFAILURE;
+  }
+
   snprintf (scratch, sizeof (scratch),
-            "INSERT INTO dspam_signature_data (uid,signature,length,created_on,data) VALUES (%d,'%s',%lu,CURRENT_DATE,E'",
-            (int) p->pw_uid, signature, (unsigned long) SIG->length);
+            "INSERT INTO dspam_signature_data (uid,signature,length,created_on,data) VALUES (%d,E'%s',%lu,CURRENT_DATE,E'",
+            (int) p->pw_uid, sig_esc, (unsigned long) SIG->length);
+  free(sig_esc);
   buffer_cat (query, scratch);
   buffer_cat (query, (const char *) mem);
   buffer_cat (query, "')");
@@ -1583,6 +1617,9 @@ _ds_delete_signature (DSPAM_CTX * CTX, const char *signature)
   char *name;
   char query[256];
   PGresult *result;
+  char *sig_esc = NULL;
+  int pgerror;
+  size_t pgsize;
 
   if (s->dbh == NULL)
   {
@@ -1605,10 +1642,24 @@ _ds_delete_signature (DSPAM_CTX * CTX, const char *signature)
     return EINVAL;
   }
 
+  /* escape the signature */
+  sig_esc = malloc(strlen(signature)*2+1);
+  if (sig_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    return EFAILURE;
+  }
+  pgsize = PQescapeStringConn (s->dbh, sig_esc, signature, strlen(signature), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_ds_get_signature: unable to escape signature '%s'", signature);
+    free(sig_esc);
+    return EFAILURE;
+  }
+
   snprintf (query, sizeof (query),
-            "DELETE FROM dspam_signature_data WHERE uid=%d AND signature='%s'",
-            (int) p->pw_uid, signature);
+            "DELETE FROM dspam_signature_data WHERE uid=%d AND signature=E'%s'",
+            (int) p->pw_uid, sig_esc);
 
+  free(sig_esc);
   result = PQexec(s->dbh, query);
   if ( !result || (PQresultStatus(result) != PGRES_COMMAND_OK && PQresultStatus(result) != PGRES_NONFATAL_ERROR) )
   {
@@ -1629,6 +1680,9 @@ _ds_verify_signature (DSPAM_CTX * CTX, const char *signature)
   char *name;
   char query[256];
   PGresult *result;
+  char *sig_esc = NULL;
+  int pgerror;
+  size_t pgsize;
 
   if (s->dbh == NULL)
   {
@@ -1651,10 +1705,24 @@ _ds_verify_signature (DSPAM_CTX * CTX, const char *signature)
     return EINVAL;
   }
 
+  /* escape the signature */
+  sig_esc = malloc(strlen(signature)*2+1);
+  if (sig_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    return EFAILURE;
+  }
+  pgsize = PQescapeStringConn (s->dbh, sig_esc, signature, strlen(signature), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_ds_get_signature: unable to escape signature '%s'", signature);
+    free(sig_esc);
+    return EFAILURE;
+  }
+
   snprintf (query, sizeof (query),
-            "SELECT signature FROM dspam_signature_data WHERE uid=%d AND signature='%s'",
-            (int) p->pw_uid, signature);
+            "SELECT signature FROM dspam_signature_data WHERE uid=%d AND signature=E'%s'",
+            (int) p->pw_uid, sig_esc);
 
+  free(sig_esc);
   result = PQexec(s->dbh, query);
   if ( !result || (PQresultStatus(result) != PGRES_TUPLES_OK && PQresultStatus(result) != PGRES_NONFATAL_ERROR) )
   {
@@ -2129,12 +2197,15 @@ _pgsql_drv_getpwnam (DSPAM_CTX * CTX, const char *name)
   char query[512];
   PGresult *result;
   char *virtual_table, *virtual_uid, *virtual_username;
+  char *name_esc = NULL;
+  int pgerror;
+  size_t pgsize;
 
   if (s->p_getpwnam.pw_name != NULL)
   {
     /* cache the last name queried */
     if (name != NULL && !strcmp (s->p_getpwnam.pw_name, name)) {
-      LOGDEBUG("_pgsql_drv_getpwnam returning cached name %s.", name);
+      LOGDEBUG("_pgsql_drv_getpwnam: returning cached name '%s'", name);
       return &s->p_getpwnam;
     }
 
@@ -2154,10 +2225,24 @@ _pgsql_drv_getpwnam (DSPAM_CTX * CTX, const char *name)
     "PgSQLVirtualUsernameField")) ==NULL)
   { virtual_username = "username"; }
 
+  /* escape the user name */
+  name_esc = malloc(strlen(name)*2+1);
+  if (name_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    return NULL;
+  }
+  pgsize = PQescapeStringConn (s->dbh, name_esc, name, strlen(name), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_pgsql_drv_getpwnam: unable to escape user name '%s'", name);
+    free(name_esc);
+    return NULL;
+  }
+
   snprintf (query, sizeof (query),
-            "SELECT %s FROM %s WHERE %s='%s'",
-            virtual_uid, virtual_table, virtual_username, name);
+            "SELECT %s FROM %s WHERE %s=E'%s'",
+            virtual_uid, virtual_table, virtual_username, name_esc);
 
+  free(name_esc);
   result = PQexec(s->dbh, query);
   if ( !result || (PQresultStatus(result) != PGRES_TUPLES_OK && PQresultStatus(result) != PGRES_NONFATAL_ERROR) )
   {
@@ -2346,6 +2431,9 @@ _pgsql_drv_setpwnam (DSPAM_CTX * CTX, const char *name)
   char *virtual_table, *virtual_uid, *virtual_username;
   struct _pgsql_drv_storage *s = (struct _pgsql_drv_storage *) CTX->storage;
   PGresult *result;
+  char *name_esc = NULL;
+  int pgerror;
+  size_t pgsize;
 
   if ((virtual_table
     = _ds_read_attribute(CTX->config->attributes, "PgSQLVirtualTable"))==NULL)
@@ -2367,9 +2455,24 @@ _pgsql_drv_setpwnam (DSPAM_CTX * CTX, const char *name)
   }
 #endif
 
+  /* escape the user name */
+  name_esc = malloc(strlen(name)*2+1);
+  if (name_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    return NULL;
+  }
+  pgsize = PQescapeStringConn (s->dbh, name_esc, name, strlen(name), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_pgsql_drv_setpwnam: unable to escape user name '%s'", name);
+    free(name_esc);
+    return NULL;
+  }
+
   snprintf (query, sizeof (query),
-            "INSERT INTO %s (%s, %s) VALUES (default, '%s')",
-            virtual_table, virtual_uid, virtual_username, name);
+            "INSERT INTO %s (%s, %s) VALUES (default, E'%s')",
+            virtual_table, virtual_uid, virtual_username, name_esc);
+
+  free(name_esc);
 
   /* we need to fail, to prevent a potential loop - even if it was inserted
    * by another process */
@@ -2697,12 +2800,16 @@ int _ds_pref_set (
   DSPAM_CTX *CTX;
   int uid;
   PGresult *result;
+  char *pref_esc = NULL;
+  char *val_esc = NULL;
+  int pgerror;
+  size_t pgsize;
 
   CTX = _pgsql_drv_init_tools(home, config, dbh, DSM_PROCESS);
   if (CTX == NULL)
   {
     LOG (LOG_WARNING, "_ds_pref_set: unable to initialize tools context");
-    return EFAILURE;
+    goto FAIL;
   }
 
   s = (struct _pgsql_drv_storage *) CTX->storage;
@@ -2714,8 +2821,7 @@ int _ds_pref_set (
     {
       LOGDEBUG ("_ds_pref_set: unable to _pgsql_drv_getpwnam(%s)",
               CTX->username);
-      dspam_destroy(CTX);
-      return EFAILURE;
+      goto FAIL;
     } else {
       uid = (int) p->pw_uid;
     }
@@ -2723,8 +2829,32 @@ int _ds_pref_set (
     uid = 0; /* Default Preferences */
   }
 
+  /* escape the preference name */
+  pref_esc = malloc(strlen(preference)*2+1);
+  if (pref_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    goto FAIL;
+  }
+  pgsize = PQescapeStringConn (s->dbh, pref_esc, preference, strlen(preference), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_ds_pref_set: unable to escape preference '%s'", preference);
+    goto FAIL;
+  }
+
+  /* escape the preference value */
+  val_esc = malloc(strlen(value)*2+1);
+  if (val_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    goto FAIL;
+  }
+  pgsize = PQescapeStringConn (s->dbh, val_esc, value, strlen(value), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_ds_pref_set: unable to escape preference value '%s'", value);
+    goto FAIL;
+  }
+
   snprintf(query, sizeof(query), "DELETE FROM dspam_preferences"
-    " WHERE uid=%d AND preference='%s'", (int) uid, preference);
+    " WHERE uid=%d AND preference=E'%s'", (int) uid, pref_esc);
 
   result = PQexec(s->dbh, query);
   if ( !result || (PQresultStatus(result) != PGRES_COMMAND_OK && PQresultStatus(result) != PGRES_NONFATAL_ERROR) )
@@ -2737,8 +2867,10 @@ int _ds_pref_set (
   if (result) PQclear(result);
 
   snprintf(query, sizeof(query), "INSERT INTO dspam_preferences"
-    " (uid,preference,value) VALUES (%d,'%s','%s')", (int) uid, preference, value);
+    " (uid,preference,value) VALUES (%d,E'%s',E'%s')", (int) uid, pref_esc, val_esc);
 
+  free(pref_esc);
+  free(val_esc);
   result = PQexec(s->dbh, query);
   if ( !result || (PQresultStatus(result) != PGRES_COMMAND_OK && PQresultStatus(result) != PGRES_NONFATAL_ERROR) )
   {
@@ -2754,7 +2886,9 @@ int _ds_pref_set (
 
 FAIL:
   LOGDEBUG("_ds_pref_set: failed");
-  dspam_destroy(CTX);
+  if (pref_esc) free(pref_esc);
+  if (val_esc) free(val_esc);
+  if (CTX) dspam_destroy(CTX);
   return EFAILURE;
 }
 
@@ -2771,11 +2905,14 @@ int _ds_pref_del (
   DSPAM_CTX *CTX;
   int uid;
   PGresult *result;
+  char *pref_esc = NULL;
+  int pgerror;
+  size_t pgsize;
 
   CTX = _pgsql_drv_init_tools(home, config, dbh, DSM_TOOLS);
   if (CTX == NULL) {
     LOG (LOG_WARNING, "_ds_pref_del: unable to initialize tools context");
-    return EFAILURE;
+    goto FAIL;
   }
 
   s = (struct _pgsql_drv_storage *) CTX->storage;
@@ -2787,8 +2924,7 @@ int _ds_pref_del (
     {
       LOGDEBUG ("_ds_pref_del: unable to _pgsql_drv_getpwnam(%s)",
               username);
-      dspam_destroy(CTX);
-      return EFAILURE;
+      goto FAIL;
     } else {
       uid = (int) p->pw_uid;
     }
@@ -2796,9 +2932,22 @@ int _ds_pref_del (
     uid = 0; /* Default Preferences */
   }
 
+  /* escape the preference name */
+  pref_esc = malloc(strlen(preference)*2+1);
+  if (pref_esc == NULL) {
+    LOG(LOG_CRIT, ERR_MEM_ALLOC);
+    goto FAIL;
+  }
+  pgsize = PQescapeStringConn (s->dbh, pref_esc, preference, strlen(preference), &pgerror);
+  if (pgsize == 0 || pgerror != 0) {
+    LOGDEBUG ("_ds_pref_del: unable to escape preference '%s'", preference);
+    goto FAIL;
+  }
+
   snprintf(query, sizeof(query), "DELETE FROM dspam_preferences"
-    " WHERE uid=%d AND preference='%s'", (int) uid, preference);
+    " WHERE uid=%d AND preference=E'%s'", (int) uid, pref_esc);
 
+  free(pref_esc);
   result = PQexec(s->dbh, query);
   if ( !result || (PQresultStatus(result) != PGRES_COMMAND_OK && PQresultStatus(result) != PGRES_NONFATAL_ERROR) )
   {
@@ -2813,7 +2962,8 @@ int _ds_pref_del (
 
 FAIL:
   LOGDEBUG("_ds_pref_del: failed");
-  dspam_destroy(CTX);
+  if (pref_esc) free(pref_esc);
+  if (CTX) dspam_destroy(CTX);
   return EFAILURE;
 }
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.