Guard against empty buffer in gets_fromFile()'s check for a newline.

tglsfdc · tglsfdc · commit 67fb608fe3b0 · 2016-07-28T18:57:39.000-04:00
Per the fgets() specification, it cannot return without reading some data
unless it reports EOF or error.  So the code here assumed that the data
buffer would necessarily be nonempty when we go to check for a newline
having been read.  However, Agostino Sarubbo noticed that this could fail
to be true if the first byte of the data is a NUL (\0).  The fgets() API
doesn't really work for embedded NULs, which is something I don't feel
any great need for us to worry about since we generally don't allow NULs
in SQL strings anyway.  But we should not access off the end of our own
buffer if the case occurs.  Normally this would just be a harmless read,
but if you were unlucky the byte before the buffer would contain '\n'
and we'd overwrite it with '\0', and if you were really unlucky that
might be valuable data and psql would crash.

Agostino reported this to pgsql-security, but after discussion we concluded
that it isn't worth treating as a security bug; if you can control the
input to psql you can do far more interesting things than just maybe-crash
it.  Nonetheless, it is a bug, so back-patch to all supported versions.
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
@@ -218,7 +218,7 @@ gets_fromFile(FILE *source)
 		}
 
 		/* EOL? */
-		if (buffer->data[buffer->len - 1] == '\n')
+		if (buffer->len > 0 && buffer->data[buffer->len - 1] == '\n')
 		{
 			buffer->data[buffer->len - 1] = '\0';
 			return pg_strdup(buffer->data);

Original file line number	Diff line number	Diff line change
`@@ -218,7 +218,7 @@ gets_fromFile(FILE *source)`
`218`	`218`	`}`
`219`	`219`
`220`	`220`	`/* EOL? */`
`221`		`- if (buffer->data[buffer->len - 1] == '\n')`
	`221`	`+ if (buffer->len > 0 && buffer->data[buffer->len - 1] == '\n')`
`222`	`222`	`{`
`223`	`223`	`buffer->data[buffer->len - 1] = '\0';`
`224`	`224`	`return pg_strdup(buffer->data);`