[asterisk-commits] cdr pgsql: Fix buffer overflow calling libpq (asterisk[14])

[SVN commits to the Asterisk project]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/5366 )

Change subject: cdr_pgsql: Fix buffer overflow calling libpq
......................................................................


cdr_pgsql: Fix buffer overflow calling libpq

Implement the same buffer size checking done in cel_pgsql.

ASTERISK-26896 #close
Reported by: twisted

Change-Id: Iaacfa1f1de7cb1e9414d121850d2d8c2888f3f48
---
M cdr/cdr_pgsql.c
M cel/cel_pgsql.c
2 files changed, 36 insertions(+), 23 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/cdr/cdr_pgsql.c b/cdr/cdr_pgsql.c
index ea38cc9..a0f0ce7 100644
--- a/cdr/cdr_pgsql.c
+++ b/cdr/cdr_pgsql.c
@@ -204,6 +204,7 @@
 	struct ast_tm tm;
 	char *pgerror;
 	PGresult *result;
+	int res = -1;
 
 	ast_mutex_lock(&pgsql_lock);
 
@@ -233,13 +234,14 @@
 	if (connected) {
 		struct columns *cur;
 		struct ast_str *sql = ast_str_create(maxsize), *sql2 = ast_str_create(maxsize2);
-		char buf[257], escapebuf[513], *value;
+		char buf[257];
+		char *escapebuf = NULL, *value;
 		char *separator = "";
+		size_t bufsize = 513;
 
-		if (!sql || !sql2) {
-			ast_free(sql);
-			ast_free(sql2);
-			return -1;
+		escapebuf = ast_malloc(bufsize);
+		if (!escapebuf || !sql || !sql2) {
+			goto ast_log_cleanup;
 		}
 
 		ast_str_set(&sql, 0, "INSERT INTO %s (", table);
@@ -360,10 +362,28 @@
 					}
 				/* XXX Might want to handle dates, times, and other misc fields here XXX */
 				} else {
-					if (value)
+					if (value) {
+						size_t required_size = strlen(value) * 2 + 1;
+
+						/* If our argument size exceeds our buffer, grow it,
+						 * as PQescapeStringConn() expects the buffer to be
+						 * adequitely sized and does *NOT* do size checking.
+						 */
+						if (required_size > bufsize) {
+							char *tmpbuf = ast_realloc(escapebuf, required_size);
+
+							if (!tmpbuf) {
+								AST_RWLIST_UNLOCK(&psql_columns);
+								goto ast_log_cleanup;
+							}
+
+							escapebuf = tmpbuf;
+							bufsize = required_size;
+						}
 						PQescapeStringConn(conn, escapebuf, value, strlen(value), NULL);
-					else
+					} else {
 						escapebuf[0] = '\0';
+					}
 					LENGTHEN_BUF2(strlen(escapebuf) + 3);
 					ast_str_append(&sql2, 0, "%s'%s'", separator, escapebuf);
 				}
@@ -397,10 +417,7 @@
 				PQfinish(conn);
 				conn = NULL;
 				connected = 0;
-				ast_mutex_unlock(&pgsql_lock);
-				ast_free(sql);
-				ast_free(sql2);
-				return -1;
+				goto ast_log_cleanup;
 			}
 		}
 		result = PQexec(conn, ast_str_buffer(sql));
@@ -421,23 +438,17 @@
 					pgerror = PQresultErrorMessage(result);
 					ast_log(LOG_ERROR, "HARD ERROR!  Attempted reconnection failed.  DROPPING CALL RECORD!\n");
 					ast_log(LOG_ERROR, "Reason: %s\n", pgerror);
-				}  else {
+				} else {
 					/* Second try worked out ok */
 					totalrecords++;
 					records++;
-					ast_mutex_unlock(&pgsql_lock);
-					PQclear(result);
-					return 0;
+					res = 0;
 				}
 			}
-			ast_mutex_unlock(&pgsql_lock);
-			PQclear(result);
-			ast_free(sql);
-			ast_free(sql2);
-			return -1;
 		} else {
 			totalrecords++;
 			records++;
+			res = 0;
 		}
 		PQclear(result);
 
@@ -449,11 +460,14 @@
 			maxsize2 = ast_str_strlen(sql2);
 		}
 
+ast_log_cleanup:
+		ast_free(escapebuf);
 		ast_free(sql);
 		ast_free(sql2);
 	}
+
 	ast_mutex_unlock(&pgsql_lock);
-	return 0;
+	return res;
 }
 
 /* This function should be called without holding the pgsql_columns lock */
diff --git a/cel/cel_pgsql.c b/cel/cel_pgsql.c
index 7ce76d2..4d61709 100644
--- a/cel/cel_pgsql.c
+++ b/cel/cel_pgsql.c
@@ -322,6 +322,7 @@
 							char *tmpbuf = ast_realloc(escapebuf, required_size);
 
 							if (!tmpbuf) {
+								AST_RWLIST_UNLOCK(&psql_columns);
 								goto ast_log_cleanup;
 							}
 
@@ -382,8 +383,6 @@
 					ast_log(LOG_ERROR, "Reason: %s\n", pgerror);
 				}
 			}
-			PQclear(result);
-			goto ast_log_cleanup;
 		}
 		PQclear(result);
 

-- 
To view, visit https://gerrit.asterisk.org/5366
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Iaacfa1f1de7cb1e9414d121850d2d8c2888f3f48
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>