Skip to content

Commit 545d153

Browse files
bukkaericmann
bukka
authored and
ericmann
committed
Fix GHSA-hrwm-9436-5mv3: pgsql escaping no error checks
This adds error checks for escape function is pgsql and pdo_pgsql extensions. It prevents possibility of storing not properly escaped data which could potentially lead to some security issues.
1 parent cf0c397 commit 545d153

File tree

4 files changed

+202
-21
lines changed

4 files changed

+202
-21
lines changed

‎ext/pdo_pgsql/pgsql_driver.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -353,11 +353,15 @@ static zend_string* pgsql_handle_quoter(pdo_dbh_t *dbh, const zend_string *unquo
353353
zend_string *quoted_str;
354354
pdo_pgsql_db_handle *H = (pdo_pgsql_db_handle *)dbh->driver_data;
355355
size_t tmp_len;
356+
int err;
356357

357358
switch (paramtype) {
358359
case PDO_PARAM_LOB:
359360
/* escapedlen returned by PQescapeBytea() accounts for trailing 0 */
360361
escaped = PQescapeByteaConn(H->server, (unsigned char *)ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), &tmp_len);
362+
if (escaped == NULL) {
363+
return NULL;
364+
}
361365
quotedlen = tmp_len + 1;
362366
quoted = emalloc(quotedlen + 1);
363367
memcpy(quoted+1, escaped, quotedlen-2);
@@ -369,7 +373,11 @@ static zend_string* pgsql_handle_quoter(pdo_dbh_t *dbh, const zend_string *unquo
369373
default:
370374
quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3);
371375
quoted[0] = '\'';
372-
quotedlen = PQescapeStringConn(H->server, quoted + 1, ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), NULL);
376+
quotedlen = PQescapeStringConn(H->server, quoted + 1, ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), &err);
377+
if (err) {
378+
efree(quoted);
379+
return NULL;
380+
}
373381
quoted[quotedlen + 1] = '\'';
374382
quoted[quotedlen + 2] = '\0';
375383
quotedlen += 2;

‎ext/pdo_pgsql/tests/ghsa-hrwm-9436-5mv3.phpt

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
--TEST--
2+
#GHSA-hrwm-9436-5mv3: pdo_pgsql extension does not check for errors during escaping
3+
--EXTENSIONS--
4+
pdo
5+
pdo_pgsql
6+
--SKIPIF--
7+
<?php
8+
require_once dirname(__FILE__) . '/../../../ext/pdo/tests/pdo_test.inc';
9+
require_once dirname(__FILE__) . '/config.inc';
10+
PDOTest::skip();
11+
?>
12+
--FILE--
13+
<?php
14+
require_once dirname(__FILE__) . '/../../../ext/pdo/tests/pdo_test.inc';
15+
require_once dirname(__FILE__) . '/config.inc';
16+
$db = PDOTest::test_factory(dirname(__FILE__) . '/common.phpt');
17+
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
18+
19+
$invalid = "ABC\xff\x30';";
20+
var_dump($db->quote($invalid));
21+
22+
?>
23+
--EXPECT--
24+
bool(false)

‎ext/pgsql/pgsql.c

Lines changed: 105 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3269,8 +3269,14 @@ PHP_FUNCTION(pg_escape_string)
32693269

32703270
to = zend_string_safe_alloc(ZSTR_LEN(from), 2, 0, 0);
32713271
if (link) {
3272+
int err;
32723273
pgsql = link->conn;
3273-
ZSTR_LEN(to) = PQescapeStringConn(pgsql, ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from), NULL);
3274+
ZSTR_LEN(to) = PQescapeStringConn(pgsql, ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from), &err);
3275+
if (err) {
3276+
zend_argument_value_error(ZEND_NUM_ARGS(), "Escaping string failed");
3277+
zend_string_efree(to);
3278+
RETURN_THROWS();
3279+
}
32743280
} else
32753281
{
32763282
ZSTR_LEN(to) = PQescapeString(ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from));
@@ -3313,6 +3319,10 @@ PHP_FUNCTION(pg_escape_bytea)
33133319
} else {
33143320
to = (char *)PQescapeBytea((unsigned char *)ZSTR_VAL(from), ZSTR_LEN(from), &to_len);
33153321
}
3322+
if (to == NULL) {
3323+
zend_argument_value_error(ZEND_NUM_ARGS(), "Escape failure");
3324+
RETURN_THROWS();
3325+
}
33163326

33173327
RETVAL_STRINGL(to, to_len-1); /* to_len includes additional '\0' */
33183328
PQfreemem(to);
@@ -4245,7 +4255,7 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
42454255
char *escaped;
42464256
smart_str querystr = {0};
42474257
size_t new_len;
4248-
int i, num_rows;
4258+
int i, num_rows, err;
42494259
zval elem;
42504260

42514261
ZEND_ASSERT(ZSTR_LEN(table_name) != 0);
@@ -4283,15 +4293,29 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
42834293
"WHERE a.attnum > 0 AND c.relname = '");
42844294
}
42854295
escaped = (char *)safe_emalloc(strlen(tmp_name2), 2, 1);
4286-
new_len = PQescapeStringConn(pg_link, escaped, tmp_name2, strlen(tmp_name2), NULL);
4296+
new_len = PQescapeStringConn(pg_link, escaped, tmp_name2, strlen(tmp_name2), &err);
4297+
if (err) {
4298+
php_error_docref(NULL, E_WARNING, "Escaping table name '%s' failed", ZSTR_VAL(table_name));
4299+
efree(src);
4300+
efree(escaped);
4301+
smart_str_free(&querystr);
4302+
return FAILURE;
4303+
}
42874304
if (new_len) {
42884305
smart_str_appendl(&querystr, escaped, new_len);
42894306
}
42904307
efree(escaped);
42914308

42924309
smart_str_appends(&querystr, "' AND n.nspname = '");
42934310
escaped = (char *)safe_emalloc(strlen(tmp_name), 2, 1);
4294-
new_len = PQescapeStringConn(pg_link, escaped, tmp_name, strlen(tmp_name), NULL);
4311+
new_len = PQescapeStringConn(pg_link, escaped, tmp_name, strlen(tmp_name), &err);
4312+
if (err) {
4313+
php_error_docref(NULL, E_WARNING, "Escaping table namespace '%s' failed", ZSTR_VAL(table_name));
4314+
efree(src);
4315+
efree(escaped);
4316+
smart_str_free(&querystr);
4317+
return FAILURE;
4318+
}
42954319
if (new_len) {
42964320
smart_str_appendl(&querystr, escaped, new_len);
42974321
}
@@ -4552,7 +4576,7 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
45524576
{
45534577
zend_string *field = NULL;
45544578
zval meta, *def, *type, *not_null, *has_default, *is_enum, *val, new_val;
4555-
int err = 0, skip_field;
4579+
int err = 0, escape_err = 0, skip_field;
45564580
php_pgsql_data_type data_type;
45574581

45584582
ZEND_ASSERT(pg_link != NULL);
@@ -4804,8 +4828,13 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
48044828
/* PostgreSQL ignores \0 */
48054829
str = zend_string_alloc(Z_STRLEN_P(val) * 2, 0);
48064830
/* better to use PGSQLescapeLiteral since PGescapeStringConn does not handle special \ */
4807-
ZSTR_LEN(str) = PQescapeStringConn(pg_link, ZSTR_VAL(str), Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
4808-
ZVAL_STR(&new_val, php_pgsql_add_quotes(str));
4831+
ZSTR_LEN(str) = PQescapeStringConn(pg_link, ZSTR_VAL(str),
4832+
Z_STRVAL_P(val), Z_STRLEN_P(val), &escape_err);
4833+
if (escape_err) {
4834+
err = 1;
4835+
} else {
4836+
ZVAL_STR(&new_val, php_pgsql_add_quotes(str));
4837+
}
48094838
zend_string_release_ex(str, false);
48104839
}
48114840
break;
@@ -4828,7 +4857,14 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
48284857
}
48294858
PGSQL_CONV_CHECK_IGNORE();
48304859
if (err) {
4831-
zend_type_error("%s(): Field \"%s\" must be of type string|null, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));
4860+
if (escape_err) {
4861+
php_error_docref(NULL, E_NOTICE,
4862+
"String value escaping failed for PostgreSQL '%s' (%s)",
4863+
Z_STRVAL_P(type), ZSTR_VAL(field));
4864+
} else {
4865+
zend_type_error("%s(): Field \"%s\" must be of type string|null, %s given",
4866+
get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));
4867+
}
48324868
}
48334869
break;
48344870

@@ -5103,6 +5139,11 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
51035139
zend_string *tmp_zstr;
51045140

51055141
tmp = PQescapeByteaConn(pg_link, (unsigned char *)Z_STRVAL_P(val), Z_STRLEN_P(val), &to_len);
5142+
if (tmp == NULL) {
5143+
php_error_docref(NULL, E_NOTICE, "Escaping value failed for %s field (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
5144+
err = 1;
5145+
break;
5146+
}
51065147
tmp_zstr = zend_string_init((char *)tmp, to_len - 1, false); /* PQescapeBytea's to_len includes additional '\0' */
51075148
PQfreemem(tmp);
51085149

@@ -5181,6 +5222,12 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
51815222
zend_hash_update(Z_ARRVAL_P(result), field, &new_val);
51825223
} else {
51835224
char *escaped = PQescapeIdentifier(pg_link, ZSTR_VAL(field), ZSTR_LEN(field));
5225+
if (escaped == NULL) {
5226+
/* This cannot fail because of invalid string but only due to failed memory allocation */
5227+
php_error_docref(NULL, E_NOTICE, "Escaping field '%s' failed", ZSTR_VAL(field));
5228+
err = 1;
5229+
break;
5230+
}
51845231
add_assoc_zval(result, escaped, &new_val);
51855232
PQfreemem(escaped);
51865233
}
@@ -5259,7 +5306,7 @@ static bool do_exec(smart_str *querystr, ExecStatusType expect, PGconn *pg_link,
52595306
}
52605307
/* }}} */
52615308

5262-
static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const zend_string *table) /* {{{ */
5309+
static inline zend_result build_tablename(smart_str *querystr, PGconn *pg_link, const zend_string *table) /* {{{ */
52635310
{
52645311
/* schema.table should be "schema"."table" */
52655312
const char *dot = memchr(ZSTR_VAL(table), '.', ZSTR_LEN(table));
@@ -5269,6 +5316,10 @@ static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const z
52695316
smart_str_appendl(querystr, ZSTR_VAL(table), len);
52705317
} else {
52715318
char *escaped = PQescapeIdentifier(pg_link, ZSTR_VAL(table), len);
5319+
if (escaped == NULL) {
5320+
php_error_docref(NULL, E_NOTICE, "Failed to escape table name '%s'", ZSTR_VAL(table));
5321+
return FAILURE;
5322+
}
52725323
smart_str_appends(querystr, escaped);
52735324
PQfreemem(escaped);
52745325
}
@@ -5281,11 +5332,17 @@ static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const z
52815332
smart_str_appendl(querystr, after_dot, len);
52825333
} else {
52835334
char *escaped = PQescapeIdentifier(pg_link, after_dot, len);
5335+
if (escaped == NULL) {
5336+
php_error_docref(NULL, E_NOTICE, "Failed to escape table name '%s'", ZSTR_VAL(table));
5337+
return FAILURE;
5338+
}
52845339
smart_str_appendc(querystr, '.');
52855340
smart_str_appends(querystr, escaped);
52865341
PQfreemem(escaped);
52875342
}
52885343
}
5344+
5345+
return SUCCESS;
52895346
}
52905347
/* }}} */
52915348

@@ -5306,7 +5363,9 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
53065363
ZVAL_UNDEF(&converted);
53075364
if (zend_hash_num_elements(Z_ARRVAL_P(var_array)) == 0) {
53085365
smart_str_appends(&querystr, "INSERT INTO ");
5309-
build_tablename(&querystr, pg_link, table);
5366+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5367+
goto cleanup;
5368+
}
53105369
smart_str_appends(&querystr, " DEFAULT VALUES");
53115370

53125371
goto no_values;
@@ -5322,7 +5381,9 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
53225381
}
53235382

53245383
smart_str_appends(&querystr, "INSERT INTO ");
5325-
build_tablename(&querystr, pg_link, table);
5384+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5385+
goto cleanup;
5386+
}
53265387
smart_str_appends(&querystr, " (");
53275388

53285389
ZEND_HASH_FOREACH_STR_KEY(Z_ARRVAL_P(var_array), fld) {
@@ -5332,6 +5393,10 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
53325393
}
53335394
if (opt & PGSQL_DML_ESCAPE) {
53345395
tmp = PQescapeIdentifier(pg_link, ZSTR_VAL(fld), ZSTR_LEN(fld) + 1);
5396+
if (tmp == NULL) {
5397+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s'", ZSTR_VAL(fld));
5398+
goto cleanup;
5399+
}
53355400
smart_str_appends(&querystr, tmp);
53365401
PQfreemem(tmp);
53375402
} else {
@@ -5343,15 +5408,19 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
53435408
smart_str_appends(&querystr, ") VALUES (");
53445409

53455410
/* make values string */
5346-
ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(var_array), val) {
5411+
ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(var_array), fld, val) {
53475412
/* we can avoid the key_type check here, because we tested it in the other loop */
53485413
switch (Z_TYPE_P(val)) {
53495414
case IS_STRING:
53505415
if (opt & PGSQL_DML_ESCAPE) {
5351-
size_t new_len;
5352-
char *tmp;
5353-
tmp = (char *)safe_emalloc(Z_STRLEN_P(val), 2, 1);
5354-
new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5416+
int error;
5417+
char *tmp = safe_emalloc(Z_STRLEN_P(val), 2, 1);
5418+
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), &error);
5419+
if (error) {
5420+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s' value", ZSTR_VAL(fld));
5421+
efree(tmp);
5422+
goto cleanup;
5423+
}
53555424
smart_str_appendc(&querystr, '\'');
53565425
smart_str_appendl(&querystr, tmp, new_len);
53575426
smart_str_appendc(&querystr, '\'');
@@ -5507,6 +5576,10 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr,
55075576
}
55085577
if (opt & PGSQL_DML_ESCAPE) {
55095578
char *tmp = PQescapeIdentifier(pg_link, ZSTR_VAL(fld), ZSTR_LEN(fld) + 1);
5579+
if (tmp == NULL) {
5580+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s'", ZSTR_VAL(fld));
5581+
return -1;
5582+
}
55105583
smart_str_appends(querystr, tmp);
55115584
PQfreemem(tmp);
55125585
} else {
@@ -5522,8 +5595,14 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr,
55225595
switch (Z_TYPE_P(val)) {
55235596
case IS_STRING:
55245597
if (opt & PGSQL_DML_ESCAPE) {
5598+
int error;
55255599
char *tmp = (char *)safe_emalloc(Z_STRLEN_P(val), 2, 1);
5526-
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5600+
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), &error);
5601+
if (error) {
5602+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s' value", ZSTR_VAL(fld));
5603+
efree(tmp);
5604+
return -1;
5605+
}
55275606
smart_str_appendc(querystr, '\'');
55285607
smart_str_appendl(querystr, tmp, new_len);
55295608
smart_str_appendc(querystr, '\'');
@@ -5591,7 +5670,9 @@ PHP_PGSQL_API zend_result php_pgsql_update(PGconn *pg_link, const zend_string *t
55915670
}
55925671

55935672
smart_str_appends(&querystr, "UPDATE ");
5594-
build_tablename(&querystr, pg_link, table);
5673+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5674+
goto cleanup;
5675+
}
55955676
smart_str_appends(&querystr, " SET ");
55965677

55975678
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(var_array), 0, ",", 1, opt))
@@ -5694,7 +5775,9 @@ PHP_PGSQL_API zend_result php_pgsql_delete(PGconn *pg_link, const zend_string *t
56945775
}
56955776

56965777
smart_str_appends(&querystr, "DELETE FROM ");
5697-
build_tablename(&querystr, pg_link, table);
5778+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5779+
goto cleanup;
5780+
}
56985781
smart_str_appends(&querystr, " WHERE ");
56995782

57005783
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(ids_array), 1, " AND ", sizeof(" AND ")-1, opt))
@@ -5834,7 +5917,9 @@ PHP_PGSQL_API zend_result php_pgsql_select(PGconn *pg_link, const zend_string *t
58345917
}
58355918

58365919
smart_str_appends(&querystr, "SELECT * FROM ");
5837-
build_tablename(&querystr, pg_link, table);
5920+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5921+
goto cleanup;
5922+
}
58385923
smart_str_appends(&querystr, " WHERE ");
58395924

58405925
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(ids_array), 1, " AND ", sizeof(" AND ")-1, opt))

0 commit comments

Comments
 (0)