Skip to content

Commit a2cdff5

Browse files
bukkasaundefined
bukka
authored and
saundefined
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 27e67cc commit a2cdff5

File tree

4 files changed

+203
-21
lines changed

4 files changed

+203
-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: 106 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3203,8 +3203,14 @@ PHP_FUNCTION(pg_escape_string)
32033203

32043204
to = zend_string_safe_alloc(ZSTR_LEN(from), 2, 0, 0);
32053205
if (link) {
3206+
int err;
32063207
pgsql = link->conn;
3207-
ZSTR_LEN(to) = PQescapeStringConn(pgsql, ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from), NULL);
3208+
ZSTR_LEN(to) = PQescapeStringConn(pgsql, ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from), &err);
3209+
if (err) {
3210+
zend_argument_value_error(ZEND_NUM_ARGS(), "Escaping string failed");
3211+
zend_string_efree(to);
3212+
RETURN_THROWS();
3213+
}
32083214
} else
32093215
{
32103216
ZSTR_LEN(to) = PQescapeString(ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from));
@@ -3247,6 +3253,10 @@ PHP_FUNCTION(pg_escape_bytea)
32473253
} else {
32483254
to = (char *)PQescapeBytea((unsigned char *)ZSTR_VAL(from), ZSTR_LEN(from), &to_len);
32493255
}
3256+
if (to == NULL) {
3257+
zend_argument_value_error(ZEND_NUM_ARGS(), "Escape failure");
3258+
RETURN_THROWS();
3259+
}
32503260

32513261
RETVAL_STRINGL(to, to_len-1); /* to_len includes additional '\0' */
32523262
PQfreemem(to);
@@ -4163,7 +4173,7 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
41634173
char *escaped;
41644174
smart_str querystr = {0};
41654175
size_t new_len;
4166-
int i, num_rows;
4176+
int i, num_rows, err;
41674177
zval elem;
41684178

41694179
ZEND_ASSERT(ZSTR_LEN(table_name) != 0);
@@ -4202,15 +4212,29 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
42024212
"WHERE a.attnum > 0 AND c.relname = '");
42034213
}
42044214
escaped = (char *)safe_emalloc(strlen(tmp_name2), 2, 1);
4205-
new_len = PQescapeStringConn(pg_link, escaped, tmp_name2, strlen(tmp_name2), NULL);
4215+
new_len = PQescapeStringConn(pg_link, escaped, tmp_name2, strlen(tmp_name2), &err);
4216+
if (err) {
4217+
php_error_docref(NULL, E_WARNING, "Escaping table name '%s' failed", ZSTR_VAL(table_name));
4218+
efree(src);
4219+
efree(escaped);
4220+
smart_str_free(&querystr);
4221+
return FAILURE;
4222+
}
42064223
if (new_len) {
42074224
smart_str_appendl(&querystr, escaped, new_len);
42084225
}
42094226
efree(escaped);
42104227

42114228
smart_str_appends(&querystr, "' AND n.nspname = '");
42124229
escaped = (char *)safe_emalloc(strlen(tmp_name), 2, 1);
4213-
new_len = PQescapeStringConn(pg_link, escaped, tmp_name, strlen(tmp_name), NULL);
4230+
new_len = PQescapeStringConn(pg_link, escaped, tmp_name, strlen(tmp_name), &err);
4231+
if (err) {
4232+
php_error_docref(NULL, E_WARNING, "Escaping table namespace '%s' failed", ZSTR_VAL(table_name));
4233+
efree(src);
4234+
efree(escaped);
4235+
smart_str_free(&querystr);
4236+
return FAILURE;
4237+
}
42144238
if (new_len) {
42154239
smart_str_appendl(&querystr, escaped, new_len);
42164240
}
@@ -4471,7 +4495,7 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
44714495
{
44724496
zend_string *field = NULL;
44734497
zval meta, *def, *type, *not_null, *has_default, *is_enum, *val, new_val;
4474-
int err = 0, skip_field;
4498+
int err = 0, escape_err = 0, skip_field;
44754499
php_pgsql_data_type data_type;
44764500

44774501
ZEND_ASSERT(pg_link != NULL);
@@ -4724,8 +4748,13 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
47244748
/* PostgreSQL ignores \0 */
47254749
str = zend_string_alloc(Z_STRLEN_P(val) * 2, 0);
47264750
/* better to use PGSQLescapeLiteral since PGescapeStringConn does not handle special \ */
4727-
ZSTR_LEN(str) = PQescapeStringConn(pg_link, ZSTR_VAL(str), Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
4728-
ZVAL_STR(&new_val, php_pgsql_add_quotes(str));
4751+
ZSTR_LEN(str) = PQescapeStringConn(pg_link, ZSTR_VAL(str),
4752+
Z_STRVAL_P(val), Z_STRLEN_P(val), &escape_err);
4753+
if (escape_err) {
4754+
err = 1;
4755+
} else {
4756+
ZVAL_STR(&new_val, php_pgsql_add_quotes(str));
4757+
}
47294758
zend_string_release_ex(str, false);
47304759
}
47314760
break;
@@ -4748,7 +4777,15 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
47484777
}
47494778
PGSQL_CONV_CHECK_IGNORE();
47504779
if (err) {
4751-
php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
4780+
if (escape_err) {
4781+
php_error_docref(NULL, E_NOTICE,
4782+
"String value escaping failed for PostgreSQL '%s' (%s)",
4783+
Z_STRVAL_P(type), ZSTR_VAL(field));
4784+
} else {
4785+
php_error_docref(NULL, E_NOTICE,
4786+
"Expects NULL, string, long or double value for PostgreSQL '%s' (%s)",
4787+
Z_STRVAL_P(type), ZSTR_VAL(field));
4788+
}
47524789
}
47534790
break;
47544791

@@ -5019,6 +5056,11 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
50195056
zend_string *tmp_zstr;
50205057

50215058
tmp = PQescapeByteaConn(pg_link, (unsigned char *)Z_STRVAL_P(val), Z_STRLEN_P(val), &to_len);
5059+
if (tmp == NULL) {
5060+
php_error_docref(NULL, E_NOTICE, "Escaping value failed for %s field (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
5061+
err = 1;
5062+
break;
5063+
}
50225064
tmp_zstr = zend_string_init((char *)tmp, to_len - 1, false); /* PQescapeBytea's to_len includes additional '\0' */
50235065
PQfreemem(tmp);
50245066

@@ -5097,6 +5139,12 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
50975139
zend_hash_update(Z_ARRVAL_P(result), field, &new_val);
50985140
} else {
50995141
char *escaped = PQescapeIdentifier(pg_link, ZSTR_VAL(field), ZSTR_LEN(field));
5142+
if (escaped == NULL) {
5143+
/* This cannot fail because of invalid string but only due to failed memory allocation */
5144+
php_error_docref(NULL, E_NOTICE, "Escaping field '%s' failed", ZSTR_VAL(field));
5145+
err = 1;
5146+
break;
5147+
}
51005148
add_assoc_zval(result, escaped, &new_val);
51015149
PQfreemem(escaped);
51025150
}
@@ -5175,7 +5223,7 @@ static bool do_exec(smart_str *querystr, ExecStatusType expect, PGconn *pg_link,
51755223
}
51765224
/* }}} */
51775225

5178-
static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const zend_string *table) /* {{{ */
5226+
static inline zend_result build_tablename(smart_str *querystr, PGconn *pg_link, const zend_string *table) /* {{{ */
51795227
{
51805228
/* schema.table should be "schema"."table" */
51815229
const char *dot = memchr(ZSTR_VAL(table), '.', ZSTR_LEN(table));
@@ -5185,6 +5233,10 @@ static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const z
51855233
smart_str_appendl(querystr, ZSTR_VAL(table), len);
51865234
} else {
51875235
char *escaped = PQescapeIdentifier(pg_link, ZSTR_VAL(table), len);
5236+
if (escaped == NULL) {
5237+
php_error_docref(NULL, E_NOTICE, "Failed to escape table name '%s'", ZSTR_VAL(table));
5238+
return FAILURE;
5239+
}
51885240
smart_str_appends(querystr, escaped);
51895241
PQfreemem(escaped);
51905242
}
@@ -5197,11 +5249,17 @@ static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const z
51975249
smart_str_appendl(querystr, after_dot, len);
51985250
} else {
51995251
char *escaped = PQescapeIdentifier(pg_link, after_dot, len);
5252+
if (escaped == NULL) {
5253+
php_error_docref(NULL, E_NOTICE, "Failed to escape table name '%s'", ZSTR_VAL(table));
5254+
return FAILURE;
5255+
}
52005256
smart_str_appendc(querystr, '.');
52015257
smart_str_appends(querystr, escaped);
52025258
PQfreemem(escaped);
52035259
}
52045260
}
5261+
5262+
return SUCCESS;
52055263
}
52065264
/* }}} */
52075265

@@ -5222,7 +5280,9 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
52225280
ZVAL_UNDEF(&converted);
52235281
if (zend_hash_num_elements(Z_ARRVAL_P(var_array)) == 0) {
52245282
smart_str_appends(&querystr, "INSERT INTO ");
5225-
build_tablename(&querystr, pg_link, table);
5283+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5284+
goto cleanup;
5285+
}
52265286
smart_str_appends(&querystr, " DEFAULT VALUES");
52275287

52285288
goto no_values;
@@ -5238,7 +5298,9 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
52385298
}
52395299

52405300
smart_str_appends(&querystr, "INSERT INTO ");
5241-
build_tablename(&querystr, pg_link, table);
5301+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5302+
goto cleanup;
5303+
}
52425304
smart_str_appends(&querystr, " (");
52435305

52445306
ZEND_HASH_FOREACH_STR_KEY(Z_ARRVAL_P(var_array), fld) {
@@ -5248,6 +5310,10 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
52485310
}
52495311
if (opt & PGSQL_DML_ESCAPE) {
52505312
tmp = PQescapeIdentifier(pg_link, ZSTR_VAL(fld), ZSTR_LEN(fld) + 1);
5313+
if (tmp == NULL) {
5314+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s'", ZSTR_VAL(fld));
5315+
goto cleanup;
5316+
}
52515317
smart_str_appends(&querystr, tmp);
52525318
PQfreemem(tmp);
52535319
} else {
@@ -5259,15 +5325,19 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
52595325
smart_str_appends(&querystr, ") VALUES (");
52605326

52615327
/* make values string */
5262-
ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(var_array), val) {
5328+
ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(var_array), fld, val) {
52635329
/* we can avoid the key_type check here, because we tested it in the other loop */
52645330
switch (Z_TYPE_P(val)) {
52655331
case IS_STRING:
52665332
if (opt & PGSQL_DML_ESCAPE) {
5267-
size_t new_len;
5268-
char *tmp;
5269-
tmp = (char *)safe_emalloc(Z_STRLEN_P(val), 2, 1);
5270-
new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5333+
int error;
5334+
char *tmp = safe_emalloc(Z_STRLEN_P(val), 2, 1);
5335+
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), &error);
5336+
if (error) {
5337+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s' value", ZSTR_VAL(fld));
5338+
efree(tmp);
5339+
goto cleanup;
5340+
}
52715341
smart_str_appendc(&querystr, '\'');
52725342
smart_str_appendl(&querystr, tmp, new_len);
52735343
smart_str_appendc(&querystr, '\'');
@@ -5423,6 +5493,10 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr,
54235493
}
54245494
if (opt & PGSQL_DML_ESCAPE) {
54255495
char *tmp = PQescapeIdentifier(pg_link, ZSTR_VAL(fld), ZSTR_LEN(fld) + 1);
5496+
if (tmp == NULL) {
5497+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s'", ZSTR_VAL(fld));
5498+
return -1;
5499+
}
54265500
smart_str_appends(querystr, tmp);
54275501
PQfreemem(tmp);
54285502
} else {
@@ -5438,8 +5512,14 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr,
54385512
switch (Z_TYPE_P(val)) {
54395513
case IS_STRING:
54405514
if (opt & PGSQL_DML_ESCAPE) {
5515+
int error;
54415516
char *tmp = (char *)safe_emalloc(Z_STRLEN_P(val), 2, 1);
5442-
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5517+
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), &error);
5518+
if (error) {
5519+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s' value", ZSTR_VAL(fld));
5520+
efree(tmp);
5521+
return -1;
5522+
}
54435523
smart_str_appendc(querystr, '\'');
54445524
smart_str_appendl(querystr, tmp, new_len);
54455525
smart_str_appendc(querystr, '\'');
@@ -5507,7 +5587,9 @@ PHP_PGSQL_API zend_result php_pgsql_update(PGconn *pg_link, const zend_string *t
55075587
}
55085588

55095589
smart_str_appends(&querystr, "UPDATE ");
5510-
build_tablename(&querystr, pg_link, table);
5590+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5591+
goto cleanup;
5592+
}
55115593
smart_str_appends(&querystr, " SET ");
55125594

55135595
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(var_array), 0, ",", 1, opt))
@@ -5610,7 +5692,9 @@ PHP_PGSQL_API zend_result php_pgsql_delete(PGconn *pg_link, const zend_string *t
56105692
}
56115693

56125694
smart_str_appends(&querystr, "DELETE FROM ");
5613-
build_tablename(&querystr, pg_link, table);
5695+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5696+
goto cleanup;
5697+
}
56145698
smart_str_appends(&querystr, " WHERE ");
56155699

56165700
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(ids_array), 1, " AND ", sizeof(" AND ")-1, opt))
@@ -5750,7 +5834,9 @@ PHP_PGSQL_API zend_result php_pgsql_select(PGconn *pg_link, const zend_string *t
57505834
}
57515835

57525836
smart_str_appends(&querystr, "SELECT * FROM ");
5753-
build_tablename(&querystr, pg_link, table);
5837+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5838+
goto cleanup;
5839+
}
57545840
smart_str_appends(&querystr, " WHERE ");
57555841

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

0 commit comments

Comments
 (0)