Skip to content

Commit 2e37a4d

Browse files
bukkaSakiTakamachi
bukka
authored and
SakiTakamachi
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 9234b0d commit 2e37a4d

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
@@ -367,11 +367,15 @@ static zend_string* pgsql_handle_quoter(pdo_dbh_t *dbh, const zend_string *unquo
367367
zend_string *quoted_str;
368368
pdo_pgsql_db_handle *H = (pdo_pgsql_db_handle *)dbh->driver_data;
369369
size_t tmp_len;
370+
int err;
370371

371372
switch (paramtype) {
372373
case PDO_PARAM_LOB:
373374
/* escapedlen returned by PQescapeBytea() accounts for trailing 0 */
374375
escaped = PQescapeByteaConn(H->server, (unsigned char *)ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), &tmp_len);
376+
if (escaped == NULL) {
377+
return NULL;
378+
}
375379
quotedlen = tmp_len + 1;
376380
quoted = emalloc(quotedlen + 1);
377381
memcpy(quoted+1, escaped, quotedlen-2);
@@ -383,7 +387,11 @@ static zend_string* pgsql_handle_quoter(pdo_dbh_t *dbh, const zend_string *unquo
383387
default:
384388
quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3);
385389
quoted[0] = '\'';
386-
quotedlen = PQescapeStringConn(H->server, quoted + 1, ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), NULL);
390+
quotedlen = PQescapeStringConn(H->server, quoted + 1, ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), &err);
391+
if (err) {
392+
efree(quoted);
393+
return NULL;
394+
}
387395
quoted[quotedlen + 1] = '\'';
388396
quoted[quotedlen + 2] = '\0';
389397
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
@@ -3528,8 +3528,14 @@ PHP_FUNCTION(pg_escape_string)
35283528

35293529
to = zend_string_safe_alloc(ZSTR_LEN(from), 2, 0, 0);
35303530
if (link) {
3531+
int err;
35313532
pgsql = link->conn;
3532-
ZSTR_LEN(to) = PQescapeStringConn(pgsql, ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from), NULL);
3533+
ZSTR_LEN(to) = PQescapeStringConn(pgsql, ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from), &err);
3534+
if (err) {
3535+
zend_argument_value_error(ZEND_NUM_ARGS(), "Escaping string failed");
3536+
zend_string_efree(to);
3537+
RETURN_THROWS();
3538+
}
35333539
} else
35343540
{
35353541
ZSTR_LEN(to) = PQescapeString(ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from));
@@ -3575,6 +3581,10 @@ PHP_FUNCTION(pg_escape_bytea)
35753581
} else {
35763582
to = (char *)PQescapeBytea((unsigned char *)ZSTR_VAL(from), ZSTR_LEN(from), &to_len);
35773583
}
3584+
if (to == NULL) {
3585+
zend_argument_value_error(ZEND_NUM_ARGS(), "Escape failure");
3586+
RETURN_THROWS();
3587+
}
35783588

35793589
RETVAL_STRINGL(to, to_len-1); /* to_len includes additional '\0' */
35803590
PQfreemem(to);
@@ -4523,7 +4533,7 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
45234533
char *escaped;
45244534
smart_str querystr = {0};
45254535
size_t new_len, len;
4526-
int i, num_rows;
4536+
int i, num_rows, err;
45274537
zval elem;
45284538

45294539
ZEND_ASSERT(ZSTR_LEN(table_name) != 0);
@@ -4562,7 +4572,14 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
45624572
}
45634573
len = strlen(tmp_name2);
45644574
escaped = (char *)safe_emalloc(len, 2, 1);
4565-
new_len = PQescapeStringConn(pg_link, escaped, tmp_name2, len, NULL);
4575+
new_len = PQescapeStringConn(pg_link, escaped, tmp_name2, len, &err);
4576+
if (err) {
4577+
php_error_docref(NULL, E_WARNING, "Escaping table name '%s' failed", ZSTR_VAL(table_name));
4578+
efree(src);
4579+
efree(escaped);
4580+
smart_str_free(&querystr);
4581+
return FAILURE;
4582+
}
45664583
if (new_len) {
45674584
smart_str_appendl(&querystr, escaped, new_len);
45684585
}
@@ -4571,7 +4588,14 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
45714588
smart_str_appends(&querystr, "' AND n.nspname = '");
45724589
len = strlen(tmp_name);
45734590
escaped = (char *)safe_emalloc(len, 2, 1);
4574-
new_len = PQescapeStringConn(pg_link, escaped, tmp_name, len, NULL);
4591+
new_len = PQescapeStringConn(pg_link, escaped, tmp_name, len, &err);
4592+
if (err) {
4593+
php_error_docref(NULL, E_WARNING, "Escaping table namespace '%s' failed", ZSTR_VAL(table_name));
4594+
efree(src);
4595+
efree(escaped);
4596+
smart_str_free(&querystr);
4597+
return FAILURE;
4598+
}
45754599
if (new_len) {
45764600
smart_str_appendl(&querystr, escaped, new_len);
45774601
}
@@ -4826,7 +4850,7 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
48264850
{
48274851
zend_string *field = NULL;
48284852
zval meta, *def, *type, *not_null, *has_default, *is_enum, *val, new_val;
4829-
int err = 0, skip_field;
4853+
int err = 0, escape_err = 0, skip_field;
48304854
php_pgsql_data_type data_type;
48314855

48324856
ZEND_ASSERT(pg_link != NULL);
@@ -5072,8 +5096,13 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
50725096
/* PostgreSQL ignores \0 */
50735097
str = zend_string_alloc(Z_STRLEN_P(val) * 2, 0);
50745098
/* better to use PGSQLescapeLiteral since PGescapeStringConn does not handle special \ */
5075-
ZSTR_LEN(str) = PQescapeStringConn(pg_link, ZSTR_VAL(str), Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5076-
ZVAL_STR(&new_val, php_pgsql_add_quotes(str));
5099+
ZSTR_LEN(str) = PQescapeStringConn(pg_link, ZSTR_VAL(str),
5100+
Z_STRVAL_P(val), Z_STRLEN_P(val), &escape_err);
5101+
if (escape_err) {
5102+
err = 1;
5103+
} else {
5104+
ZVAL_STR(&new_val, php_pgsql_add_quotes(str));
5105+
}
50775106
zend_string_release_ex(str, false);
50785107
}
50795108
break;
@@ -5096,7 +5125,14 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
50965125
}
50975126
PGSQL_CONV_CHECK_IGNORE();
50985127
if (err) {
5099-
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));
5128+
if (escape_err) {
5129+
php_error_docref(NULL, E_NOTICE,
5130+
"String value escaping failed for PostgreSQL '%s' (%s)",
5131+
Z_STRVAL_P(type), ZSTR_VAL(field));
5132+
} else {
5133+
zend_type_error("%s(): Field \"%s\" must be of type string|null, %s given",
5134+
get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));
5135+
}
51005136
}
51015137
break;
51025138

@@ -5330,6 +5366,11 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
53305366
zend_string *tmp_zstr;
53315367

53325368
tmp = PQescapeByteaConn(pg_link, (unsigned char *)Z_STRVAL_P(val), Z_STRLEN_P(val), &to_len);
5369+
if (tmp == NULL) {
5370+
php_error_docref(NULL, E_NOTICE, "Escaping value failed for %s field (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
5371+
err = 1;
5372+
break;
5373+
}
53335374
tmp_zstr = zend_string_init((char *)tmp, to_len - 1, false); /* PQescapeBytea's to_len includes additional '\0' */
53345375
PQfreemem(tmp);
53355376

@@ -5406,6 +5447,12 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
54065447
zend_hash_update(Z_ARRVAL_P(result), field, &new_val);
54075448
} else {
54085449
char *escaped = PQescapeIdentifier(pg_link, ZSTR_VAL(field), ZSTR_LEN(field));
5450+
if (escaped == NULL) {
5451+
/* This cannot fail because of invalid string but only due to failed memory allocation */
5452+
php_error_docref(NULL, E_NOTICE, "Escaping field '%s' failed", ZSTR_VAL(field));
5453+
err = 1;
5454+
break;
5455+
}
54095456
add_assoc_zval(result, escaped, &new_val);
54105457
PQfreemem(escaped);
54115458
}
@@ -5488,7 +5535,7 @@ static bool do_exec(smart_str *querystr, ExecStatusType expect, PGconn *pg_link,
54885535
}
54895536
/* }}} */
54905537

5491-
static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const zend_string *table) /* {{{ */
5538+
static inline zend_result build_tablename(smart_str *querystr, PGconn *pg_link, const zend_string *table) /* {{{ */
54925539
{
54935540
/* schema.table should be "schema"."table" */
54945541
const char *dot = memchr(ZSTR_VAL(table), '.', ZSTR_LEN(table));
@@ -5498,6 +5545,10 @@ static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const z
54985545
smart_str_appendl(querystr, ZSTR_VAL(table), len);
54995546
} else {
55005547
char *escaped = PQescapeIdentifier(pg_link, ZSTR_VAL(table), len);
5548+
if (escaped == NULL) {
5549+
php_error_docref(NULL, E_NOTICE, "Failed to escape table name '%s'", ZSTR_VAL(table));
5550+
return FAILURE;
5551+
}
55015552
smart_str_appends(querystr, escaped);
55025553
PQfreemem(escaped);
55035554
}
@@ -5510,11 +5561,17 @@ static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const z
55105561
smart_str_appendl(querystr, after_dot, len);
55115562
} else {
55125563
char *escaped = PQescapeIdentifier(pg_link, after_dot, len);
5564+
if (escaped == NULL) {
5565+
php_error_docref(NULL, E_NOTICE, "Failed to escape table name '%s'", ZSTR_VAL(table));
5566+
return FAILURE;
5567+
}
55135568
smart_str_appendc(querystr, '.');
55145569
smart_str_appends(querystr, escaped);
55155570
PQfreemem(escaped);
55165571
}
55175572
}
5573+
5574+
return SUCCESS;
55185575
}
55195576
/* }}} */
55205577

@@ -5535,7 +5592,9 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
55355592
ZVAL_UNDEF(&converted);
55365593
if (zend_hash_num_elements(Z_ARRVAL_P(var_array)) == 0) {
55375594
smart_str_appends(&querystr, "INSERT INTO ");
5538-
build_tablename(&querystr, pg_link, table);
5595+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5596+
goto cleanup;
5597+
}
55395598
smart_str_appends(&querystr, " DEFAULT VALUES");
55405599

55415600
goto no_values;
@@ -5551,7 +5610,9 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
55515610
}
55525611

55535612
smart_str_appends(&querystr, "INSERT INTO ");
5554-
build_tablename(&querystr, pg_link, table);
5613+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5614+
goto cleanup;
5615+
}
55555616
smart_str_appends(&querystr, " (");
55565617

55575618
ZEND_HASH_FOREACH_STR_KEY(Z_ARRVAL_P(var_array), fld) {
@@ -5561,6 +5622,10 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
55615622
}
55625623
if (opt & PGSQL_DML_ESCAPE) {
55635624
tmp = PQescapeIdentifier(pg_link, ZSTR_VAL(fld), ZSTR_LEN(fld) + 1);
5625+
if (tmp == NULL) {
5626+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s'", ZSTR_VAL(fld));
5627+
goto cleanup;
5628+
}
55645629
smart_str_appends(&querystr, tmp);
55655630
PQfreemem(tmp);
55665631
} else {
@@ -5572,15 +5637,19 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
55725637
smart_str_appends(&querystr, ") VALUES (");
55735638

55745639
/* make values string */
5575-
ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(var_array), val) {
5640+
ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(var_array), fld, val) {
55765641
/* we can avoid the key_type check here, because we tested it in the other loop */
55775642
switch (Z_TYPE_P(val)) {
55785643
case IS_STRING:
55795644
if (opt & PGSQL_DML_ESCAPE) {
5580-
size_t new_len;
5581-
char *tmp;
5582-
tmp = (char *)safe_emalloc(Z_STRLEN_P(val), 2, 1);
5583-
new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5645+
int error;
5646+
char *tmp = safe_emalloc(Z_STRLEN_P(val), 2, 1);
5647+
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), &error);
5648+
if (error) {
5649+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s' value", ZSTR_VAL(fld));
5650+
efree(tmp);
5651+
goto cleanup;
5652+
}
55845653
smart_str_appendc(&querystr, '\'');
55855654
smart_str_appendl(&querystr, tmp, new_len);
55865655
smart_str_appendc(&querystr, '\'');
@@ -5738,6 +5807,10 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr,
57385807
}
57395808
if (opt & PGSQL_DML_ESCAPE) {
57405809
char *tmp = PQescapeIdentifier(pg_link, ZSTR_VAL(fld), ZSTR_LEN(fld) + 1);
5810+
if (tmp == NULL) {
5811+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s'", ZSTR_VAL(fld));
5812+
return -1;
5813+
}
57415814
smart_str_appends(querystr, tmp);
57425815
PQfreemem(tmp);
57435816
} else {
@@ -5753,8 +5826,14 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr,
57535826
switch (Z_TYPE_P(val)) {
57545827
case IS_STRING:
57555828
if (opt & PGSQL_DML_ESCAPE) {
5829+
int error;
57565830
char *tmp = (char *)safe_emalloc(Z_STRLEN_P(val), 2, 1);
5757-
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5831+
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), &error);
5832+
if (error) {
5833+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s' value", ZSTR_VAL(fld));
5834+
efree(tmp);
5835+
return -1;
5836+
}
57585837
smart_str_appendc(querystr, '\'');
57595838
smart_str_appendl(querystr, tmp, new_len);
57605839
smart_str_appendc(querystr, '\'');
@@ -5822,7 +5901,9 @@ PHP_PGSQL_API zend_result php_pgsql_update(PGconn *pg_link, const zend_string *t
58225901
}
58235902

58245903
smart_str_appends(&querystr, "UPDATE ");
5825-
build_tablename(&querystr, pg_link, table);
5904+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5905+
goto cleanup;
5906+
}
58265907
smart_str_appends(&querystr, " SET ");
58275908

58285909
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(var_array), 0, ",", 1, opt))
@@ -5928,7 +6009,9 @@ PHP_PGSQL_API zend_result php_pgsql_delete(PGconn *pg_link, const zend_string *t
59286009
}
59296010

59306011
smart_str_appends(&querystr, "DELETE FROM ");
5931-
build_tablename(&querystr, pg_link, table);
6012+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
6013+
goto cleanup;
6014+
}
59326015
smart_str_appends(&querystr, " WHERE ");
59336016

59346017
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(ids_array), 1, " AND ", sizeof(" AND ")-1, opt))
@@ -6072,7 +6155,9 @@ PHP_PGSQL_API zend_result php_pgsql_select(PGconn *pg_link, const zend_string *t
60726155
}
60736156

60746157
smart_str_appends(&querystr, "SELECT * FROM ");
6075-
build_tablename(&querystr, pg_link, table);
6158+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
6159+
goto cleanup;
6160+
}
60766161

60776162
if (is_valid_ids_array) {
60786163
smart_str_appends(&querystr, " WHERE ");

0 commit comments

Comments
 (0)