Skip to content

Commit

Permalink
[#8805] ysql: Import Fix incautious handling of possibly-miscoded str…
Browse files Browse the repository at this point in the history
…ings in client code.

Summary:
Commit was 89a5499ef9e477beb97d742d4df6fc8f601d87d5

Commit message was:
```
An incorrectly-encoded multibyte character near the end of a string
could cause various processing loops to run past the string's
terminating NUL, with results ranging from no detectable issue to
a program crash, depending on what happens to be in the following
memory.

This isn't an issue in the server, because we take care to verify
the encoding of strings before doing any interesting processing
on them.  However, that lack of care leaked into client-side code
which shouldn't assume that anyone has validated the encoding of
its input.

Although this is certainly a bug worth fixing, the PG security team
elected not to regard it as a security issue, primarily because
any untrusted text should be sanitized by PQescapeLiteral or
the like before being incorporated into a SQL or psql command.
(If an app fails to do so, the same technique can be used to
cause SQL injection, with probably much more dire consequences
than a mere client-program crash.)  Those functions were already
made proof against this class of problem, cf CVE-2006-2313.

To fix, invent PQmblenBounded() which is like PQmblen() except it
won't return more than the number of bytes remaining in the string.
In HEAD we can make this a new libpq function, as PQmblen() is.
It seems imprudent to change libpq's API in stable branches though,
so in the back branches define PQmblenBounded as a macro in the files
that need it.  (Note that just changing PQmblen's behavior would not
be a good idea; notably, it would completely break the escaping
functions' defense against this exact problem.  So we just want a
version for those callers that don't have any better way of handling
this issue.)

Per private report from houjingyi.  Back-patch to all supported branches.
```

Test Plan: Build Yugabyte DB and verify there is no test regression.

Reviewers: jason

Reviewed By: jason

Subscribers: yql

Differential Revision: https://phabricator.dev.yugabyte.com/D11895
  • Loading branch information
tedyu committed Jun 16, 2021
1 parent 455b506 commit bdb3fd0
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 21 deletions.
26 changes: 14 additions & 12 deletions src/postgres/src/bin/psql/common.c
Expand Up @@ -29,6 +29,8 @@
#include "fe_utils/mbprint.h"


#define PQmblenBounded(s, e) strnlen(s, PQmblen(s, e))

static bool DescribeQuery(const char *query, double *elapsed_msec);
static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
static bool command_no_begin(const char *query);
Expand Down Expand Up @@ -1980,7 +1982,7 @@ skip_white_space(const char *query)

while (*query)
{
int mblen = PQmblen(query, pset.encoding);
int mblen = PQmblenBounded(query, pset.encoding);

/*
* Note: we assume the encoding is a superset of ASCII, so that for
Expand Down Expand Up @@ -2017,7 +2019,7 @@ skip_white_space(const char *query)
query++;
break;
}
query += PQmblen(query, pset.encoding);
query += PQmblenBounded(query, pset.encoding);
}
}
else if (cnestlevel > 0)
Expand Down Expand Up @@ -2052,7 +2054,7 @@ command_no_begin(const char *query)
*/
wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

/*
* Transaction control commands. These should include every keyword that
Expand Down Expand Up @@ -2083,7 +2085,7 @@ command_no_begin(const char *query)

wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

if (wordlen == 11 && pg_strncasecmp(query, "transaction", 11) == 0)
return true;
Expand Down Expand Up @@ -2117,7 +2119,7 @@ command_no_begin(const char *query)

wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

if (wordlen == 8 && pg_strncasecmp(query, "database", 8) == 0)
return true;
Expand All @@ -2133,7 +2135,7 @@ command_no_begin(const char *query)

wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);
}

if (wordlen == 5 && pg_strncasecmp(query, "index", 5) == 0)
Expand All @@ -2144,7 +2146,7 @@ command_no_begin(const char *query)

wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

if (wordlen == 12 && pg_strncasecmp(query, "concurrently", 12) == 0)
return true;
Expand All @@ -2161,7 +2163,7 @@ command_no_begin(const char *query)

wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

/* ALTER SYSTEM isn't allowed in xacts */
if (wordlen == 6 && pg_strncasecmp(query, "system", 6) == 0)
Expand All @@ -2184,7 +2186,7 @@ command_no_begin(const char *query)

wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

if (wordlen == 8 && pg_strncasecmp(query, "database", 8) == 0)
return true;
Expand All @@ -2202,7 +2204,7 @@ command_no_begin(const char *query)

wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

if (wordlen == 12 && pg_strncasecmp(query, "concurrently", 12) == 0)
return true;
Expand All @@ -2222,7 +2224,7 @@ command_no_begin(const char *query)

wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

if (wordlen == 3 && pg_strncasecmp(query, "all", 3) == 0)
return true;
Expand Down Expand Up @@ -2258,7 +2260,7 @@ is_select_command(const char *query)
*/
wordlen = 0;
while (isalpha((unsigned char) query[wordlen]))
wordlen += PQmblen(&query[wordlen], pset.encoding);
wordlen += PQmblenBounded(&query[wordlen], pset.encoding);

if (wordlen == 6 && pg_strncasecmp(query, "select", 6) == 0)
return true;
Expand Down
4 changes: 3 additions & 1 deletion src/postgres/src/bin/psql/psqlscanslash.l
Expand Up @@ -27,6 +27,8 @@
%{
#include "fe_utils/psqlscan_int.h"

#define PQmblenBounded(s, e) strnlen(s, PQmblen(s, e))

/*
* We must have a typedef YYSTYPE for yylex's first argument, but this lexer
* doesn't presently make use of that argument, so just declare it as int.
Expand Down Expand Up @@ -752,7 +754,7 @@ dequote_downcase_identifier(char *str, bool downcase, int encoding)
{
if (downcase && !inquotes)
*cp = pg_tolower((unsigned char) *cp);
cp += PQmblen(cp, encoding);
cp += PQmblenBounded(cp, encoding);
}
}
}
Expand Down
8 changes: 5 additions & 3 deletions src/postgres/src/bin/psql/stringutils.c
Expand Up @@ -12,6 +12,8 @@
#include "common.h"
#include "stringutils.h"

#define PQmblenBounded(s, e) strnlen(s, PQmblen(s, e))


/*
* Replacement for strtok() (a.k.a. poor man's flex)
Expand Down Expand Up @@ -143,7 +145,7 @@ strtokx(const char *s,
/* okay, we have a quoted token, now scan for the closer */
char thisquote = *p++;

for (; *p; p += PQmblen(p, encoding))
for (; *p; p += PQmblenBounded(p, encoding))
{
if (*p == escape && p[1] != '\0')
p++; /* process escaped anything */
Expand Down Expand Up @@ -262,7 +264,7 @@ strip_quotes(char *source, char quote, char escape, int encoding)
else if (c == escape && src[1] != '\0')
src++; /* process escaped character */

i = PQmblen(src, encoding);
i = PQmblenBounded(src, encoding);
while (i--)
*dst++ = *src++;
}
Expand Down Expand Up @@ -322,7 +324,7 @@ quote_if_needed(const char *source, const char *entails_quote,
else if (strchr(entails_quote, c))
need_quotes = true;

i = PQmblen(src, encoding);
i = PQmblenBounded(src, encoding);
while (i--)
*dst++ = *src++;
}
Expand Down
4 changes: 3 additions & 1 deletion src/postgres/src/bin/psql/tab-complete.c
Expand Up @@ -60,6 +60,8 @@ extern char *filename_completion_function();
#define completion_matches rl_completion_matches
#endif

#define PQmblenBounded(s, e) strnlen(s, PQmblen(s, e))

/* word break characters */
#define WORD_BREAKS "\t\n@$><=;|&{() "

Expand Down Expand Up @@ -4119,7 +4121,7 @@ _complete_from_query(const char *simple_query,
while (*pstr)
{
char_length++;
pstr += PQmblen(pstr, pset.encoding);
pstr += PQmblenBounded(pstr, pset.encoding);
}

/* Free any prior result */
Expand Down
4 changes: 3 additions & 1 deletion src/postgres/src/bin/scripts/common.c
Expand Up @@ -22,6 +22,8 @@
#include "fe_utils/string_utils.h"


#define PQmblenBounded(s, e) strnlen(s, PQmblen(s, e))

static PGcancel *volatile cancelConn = NULL;
bool CancelRequested = false;

Expand Down Expand Up @@ -304,7 +306,7 @@ split_table_columns_spec(const char *spec, int encoding,
cp++;
}
else
cp += PQmblen(cp, encoding);
cp += PQmblenBounded(cp, encoding);
}
*table = pg_strdup(spec);
(*table)[cp - spec] = '\0'; /* no strndup */
Expand Down
3 changes: 3 additions & 0 deletions src/postgres/src/fe_utils/print.c
Expand Up @@ -3529,6 +3529,9 @@ strlen_max_width(unsigned char *str, int *target_width, int encoding)
curr_width += char_width;

str += PQmblen((char *) str, encoding);

if (str > end) /* Don't overrun invalid string */
str = end;
}

*target_width = curr_width;
Expand Down
3 changes: 2 additions & 1 deletion src/postgres/src/interfaces/libpq/fe-print.c
Expand Up @@ -36,6 +36,7 @@
#include "libpq-fe.h"
#include "libpq-int.h"

#define PQmblenBounded(s, e) strnlen(s, PQmblen(s, e))

static void do_field(const PQprintOpt *po, const PGresult *res,
const int i, const int j, const int fs_len,
Expand Down Expand Up @@ -365,7 +366,7 @@ do_field(const PQprintOpt *po, const PGresult *res,
/* Detect whether field contains non-numeric data */
char ch = '0';

for (p = pval; *p; p += PQmblen(p, res->client_encoding))
for (p = pval; *p; p += PQmblenBounded(p, res->client_encoding))
{
ch = *p;
if (!((ch >= '0' && ch <= '9') ||
Expand Down
6 changes: 4 additions & 2 deletions src/postgres/src/interfaces/libpq/fe-protocol3.c
Expand Up @@ -41,6 +41,8 @@
((id) == 'T' || (id) == 'D' || (id) == 'd' || (id) == 'V' || \
(id) == 'E' || (id) == 'N' || (id) == 'A')

#define PQmblenBounded(s, e) strnlen(s, PQmblen(s, e))


static void handleSyncLoss(PGconn *conn, char id, int msgLength);
static int getRowDescriptions(PGconn *conn, int msgLength);
Expand Down Expand Up @@ -1261,7 +1263,7 @@ reportErrorPosition(PQExpBuffer msg, const char *query, int loc, int encoding)
if (w <= 0)
w = 1;
scroffset += w;
qoffset += pg_encoding_mblen(encoding, &wquery[qoffset]);
qoffset += PQmblenBounded(&wquery[qoffset], encoding);
}
else
{
Expand Down Expand Up @@ -1329,7 +1331,7 @@ reportErrorPosition(PQExpBuffer msg, const char *query, int loc, int encoding)
* width.
*/
scroffset = 0;
for (; i < msg->len; i += pg_encoding_mblen(encoding, &msg->data[i]))
for (; i < msg->len; i += PQmblenBounded(&msg->data[i], encoding))
{
int w = pg_encoding_dsplen(encoding, &msg->data[i]);

Expand Down

0 comments on commit bdb3fd0

Please sign in to comment.