[Asterisk-code-review] cel pgsql.c: Fix buffer overflow calling libpq (asterisk[13])

[Joshua Colp]

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

Change subject: cel_pgsql.c: Fix buffer overflow calling libpq
......................................................................


cel_pgsql.c: Fix buffer overflow calling libpq

PQEscapeStringConn() expects the buffer passed in to be an
adequitely sized buffer to write out the escaped SQL value string
into.  It is possible, for large values (such as large values to
Dial with a lot of devices) to have more than our 512+1 byte
allocation and thus cause libpq to create a buffer overrun.

glibc will nicely ABRT asterisk for you, citing a stack smash.

Let's only allocate it to be as large as needed:
If we have a value, then (strlen(value) * 2) + 1 (as recommended
by libpq), and if we have none, just one byte to hold our null
will do.

ASTERISK-26896 #close

Change-Id: If611c734292618ed68dde17816d09dd16667dea2
---
M cel/cel_pgsql.c
1 file changed, 22 insertions(+), 2 deletions(-)

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



diff --git a/cel/cel_pgsql.c b/cel/cel_pgsql.c
index 61e3c8d..4ab3878 100644
--- a/cel/cel_pgsql.c
+++ b/cel/cel_pgsql.c
@@ -184,11 +184,14 @@
 	if (connected) {
 		struct columns *cur;
 		struct ast_str *sql = ast_str_create(maxsize), *sql2 = ast_str_create(maxsize2);
-		char buf[257], escapebuf[513];
+		char buf[257];
+		char *escapebuf = NULL;
 		const char *value;
 		int first = 1;
+		size_t bufsize = 513;
 
-		if (!sql || !sql2) {
+		escapebuf = ast_malloc(bufsize);
+		if (!escapebuf || !sql || !sql2) {
 			goto ast_log_cleanup;
 		}
 
@@ -312,6 +315,22 @@
 					/* XXX Might want to handle dates, times, and other misc fields here XXX */
 				} else {
 					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) {
+								goto ast_log_cleanup;
+							}
+
+							escapebuf = tmpbuf;
+							bufsize = required_size;
+						}
 						PQescapeStringConn(conn, escapebuf, value, strlen(value), NULL);
 					} else {
 						escapebuf[0] = '\0';
@@ -374,6 +393,7 @@
 ast_log_cleanup:
 		ast_free(sql);
 		ast_free(sql2);
+		ast_free(escapebuf);
 	}
 
 	ast_mutex_unlock(&pgsql_lock);

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

Gerrit-MessageType: merged
Gerrit-Change-Id: If611c734292618ed68dde17816d09dd16667dea2
Gerrit-PatchSet: 5
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Josh Roberson <josh at asteriasgi.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Josh Roberson <josh at asteriasgi.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>