Skip to content

Commit 9376aee

Browse files
bukkabukka
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 cac8f7f commit 9376aee

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
@@ -354,11 +354,15 @@ static zend_string* pgsql_handle_quoter(pdo_dbh_t *dbh, const zend_string *unquo
354354
zend_string *quoted_str;
355355
pdo_pgsql_db_handle *H = (pdo_pgsql_db_handle *)dbh->driver_data;
356356
size_t tmp_len;
357+
int err;
357358

358359
switch (paramtype) {
359360
case PDO_PARAM_LOB:
360361
/* escapedlen returned by PQescapeBytea() accounts for trailing 0 */
361362
escaped = PQescapeByteaConn(H->server, (unsigned char *)ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), &tmp_len);
363+
if (escaped == NULL) {
364+
return NULL;
365+
}
362366
quotedlen = tmp_len + 1;
363367
quoted = emalloc(quotedlen + 1);
364368
memcpy(quoted+1, escaped, quotedlen-2);
@@ -370,7 +374,11 @@ static zend_string* pgsql_handle_quoter(pdo_dbh_t *dbh, const zend_string *unquo
370374
default:
371375
quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3);
372376
quoted[0] = '\'';
373-
quotedlen = PQescapeStringConn(H->server, quoted + 1, ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), NULL);
377+
quotedlen = PQescapeStringConn(H->server, quoted + 1, ZSTR_VAL(unquoted), ZSTR_LEN(unquoted), &err);
378+
if (err) {
379+
efree(quoted);
380+
return NULL;
381+
}
374382
quoted[quotedlen + 1] = '\'';
375383
quoted[quotedlen + 2] = '\0';
376384
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
@@ -3297,8 +3297,14 @@ PHP_FUNCTION(pg_escape_string)
32973297

32983298
to = zend_string_safe_alloc(ZSTR_LEN(from), 2, 0, 0);
32993299
if (link) {
3300+
int err;
33003301
pgsql = link->conn;
3301-
ZSTR_LEN(to) = PQescapeStringConn(pgsql, ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from), NULL);
3302+
ZSTR_LEN(to) = PQescapeStringConn(pgsql, ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from), &err);
3303+
if (err) {
3304+
zend_argument_value_error(ZEND_NUM_ARGS(), "Escaping string failed");
3305+
zend_string_efree(to);
3306+
RETURN_THROWS();
3307+
}
33023308
} else
33033309
{
33043310
ZSTR_LEN(to) = PQescapeString(ZSTR_VAL(to), ZSTR_VAL(from), ZSTR_LEN(from));
@@ -3341,6 +3347,10 @@ PHP_FUNCTION(pg_escape_bytea)
33413347
} else {
33423348
to = (char *)PQescapeBytea((unsigned char *)ZSTR_VAL(from), ZSTR_LEN(from), &to_len);
33433349
}
3350+
if (to == NULL) {
3351+
zend_argument_value_error(ZEND_NUM_ARGS(), "Escape failure");
3352+
RETURN_THROWS();
3353+
}
33443354

33453355
RETVAL_STRINGL(to, to_len-1); /* to_len includes additional '\0' */
33463356
PQfreemem(to);
@@ -4257,7 +4267,7 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
42574267
char *escaped;
42584268
smart_str querystr = {0};
42594269
size_t new_len;
4260-
int i, num_rows;
4270+
int i, num_rows, err;
42614271
zval elem;
42624272

42634273
ZEND_ASSERT(ZSTR_LEN(table_name) != 0);
@@ -4296,15 +4306,29 @@ PHP_PGSQL_API zend_result php_pgsql_meta_data(PGconn *pg_link, const zend_string
42964306
"WHERE a.attnum > 0 AND c.relname = '");
42974307
}
42984308
escaped = (char *)safe_emalloc(strlen(tmp_name2), 2, 1);
4299-
new_len = PQescapeStringConn(pg_link, escaped, tmp_name2, strlen(tmp_name2), NULL);
4309+
new_len = PQescapeStringConn(pg_link, escaped, tmp_name2, strlen(tmp_name2), &err);
4310+
if (err) {
4311+
php_error_docref(NULL, E_WARNING, "Escaping table name '%s' failed", ZSTR_VAL(table_name));
4312+
efree(src);
4313+
efree(escaped);
4314+
smart_str_free(&querystr);
4315+
return FAILURE;
4316+
}
43004317
if (new_len) {
43014318
smart_str_appendl(&querystr, escaped, new_len);
43024319
}
43034320
efree(escaped);
43044321

43054322
smart_str_appends(&querystr, "' AND n.nspname = '");
43064323
escaped = (char *)safe_emalloc(strlen(tmp_name), 2, 1);
4307-
new_len = PQescapeStringConn(pg_link, escaped, tmp_name, strlen(tmp_name), NULL);
4324+
new_len = PQescapeStringConn(pg_link, escaped, tmp_name, strlen(tmp_name), &err);
4325+
if (err) {
4326+
php_error_docref(NULL, E_WARNING, "Escaping table namespace '%s' failed", ZSTR_VAL(table_name));
4327+
efree(src);
4328+
efree(escaped);
4329+
smart_str_free(&querystr);
4330+
return FAILURE;
4331+
}
43084332
if (new_len) {
43094333
smart_str_appendl(&querystr, escaped, new_len);
43104334
}
@@ -4565,7 +4589,7 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
45654589
{
45664590
zend_string *field = NULL;
45674591
zval meta, *def, *type, *not_null, *has_default, *is_enum, *val, new_val;
4568-
int err = 0, skip_field;
4592+
int err = 0, escape_err = 0, skip_field;
45694593
php_pgsql_data_type data_type;
45704594

45714595
ZEND_ASSERT(pg_link != NULL);
@@ -4818,8 +4842,13 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
48184842
/* PostgreSQL ignores \0 */
48194843
str = zend_string_alloc(Z_STRLEN_P(val) * 2, 0);
48204844
/* better to use PGSQLescapeLiteral since PGescapeStringConn does not handle special \ */
4821-
ZSTR_LEN(str) = PQescapeStringConn(pg_link, ZSTR_VAL(str), Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
4822-
ZVAL_STR(&new_val, php_pgsql_add_quotes(str));
4845+
ZSTR_LEN(str) = PQescapeStringConn(pg_link, ZSTR_VAL(str),
4846+
Z_STRVAL_P(val), Z_STRLEN_P(val), &escape_err);
4847+
if (escape_err) {
4848+
err = 1;
4849+
} else {
4850+
ZVAL_STR(&new_val, php_pgsql_add_quotes(str));
4851+
}
48234852
zend_string_release_ex(str, false);
48244853
}
48254854
break;
@@ -4842,7 +4871,15 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
48424871
}
48434872
PGSQL_CONV_CHECK_IGNORE();
48444873
if (err) {
4845-
php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
4874+
if (escape_err) {
4875+
php_error_docref(NULL, E_NOTICE,
4876+
"String value escaping failed for PostgreSQL '%s' (%s)",
4877+
Z_STRVAL_P(type), ZSTR_VAL(field));
4878+
} else {
4879+
php_error_docref(NULL, E_NOTICE,
4880+
"Expects NULL, string, long or double value for PostgreSQL '%s' (%s)",
4881+
Z_STRVAL_P(type), ZSTR_VAL(field));
4882+
}
48464883
}
48474884
break;
48484885

@@ -5113,6 +5150,11 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
51135150
zend_string *tmp_zstr;
51145151

51155152
tmp = PQescapeByteaConn(pg_link, (unsigned char *)Z_STRVAL_P(val), Z_STRLEN_P(val), &to_len);
5153+
if (tmp == NULL) {
5154+
php_error_docref(NULL, E_NOTICE, "Escaping value failed for %s field (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
5155+
err = 1;
5156+
break;
5157+
}
51165158
tmp_zstr = zend_string_init((char *)tmp, to_len - 1, false); /* PQescapeBytea's to_len includes additional '\0' */
51175159
PQfreemem(tmp);
51185160

@@ -5191,6 +5233,12 @@ PHP_PGSQL_API zend_result php_pgsql_convert(PGconn *pg_link, const zend_string *
51915233
zend_hash_update(Z_ARRVAL_P(result), field, &new_val);
51925234
} else {
51935235
char *escaped = PQescapeIdentifier(pg_link, ZSTR_VAL(field), ZSTR_LEN(field));
5236+
if (escaped == NULL) {
5237+
/* This cannot fail because of invalid string but only due to failed memory allocation */
5238+
php_error_docref(NULL, E_NOTICE, "Escaping field '%s' failed", ZSTR_VAL(field));
5239+
err = 1;
5240+
break;
5241+
}
51945242
add_assoc_zval(result, escaped, &new_val);
51955243
PQfreemem(escaped);
51965244
}
@@ -5269,7 +5317,7 @@ static bool do_exec(smart_str *querystr, ExecStatusType expect, PGconn *pg_link,
52695317
}
52705318
/* }}} */
52715319

5272-
static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const zend_string *table) /* {{{ */
5320+
static inline zend_result build_tablename(smart_str *querystr, PGconn *pg_link, const zend_string *table) /* {{{ */
52735321
{
52745322
/* schema.table should be "schema"."table" */
52755323
const char *dot = memchr(ZSTR_VAL(table), '.', ZSTR_LEN(table));
@@ -5279,6 +5327,10 @@ static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const z
52795327
smart_str_appendl(querystr, ZSTR_VAL(table), len);
52805328
} else {
52815329
char *escaped = PQescapeIdentifier(pg_link, ZSTR_VAL(table), len);
5330+
if (escaped == NULL) {
5331+
php_error_docref(NULL, E_NOTICE, "Failed to escape table name '%s'", ZSTR_VAL(table));
5332+
return FAILURE;
5333+
}
52825334
smart_str_appends(querystr, escaped);
52835335
PQfreemem(escaped);
52845336
}
@@ -5291,11 +5343,17 @@ static inline void build_tablename(smart_str *querystr, PGconn *pg_link, const z
52915343
smart_str_appendl(querystr, after_dot, len);
52925344
} else {
52935345
char *escaped = PQescapeIdentifier(pg_link, after_dot, len);
5346+
if (escaped == NULL) {
5347+
php_error_docref(NULL, E_NOTICE, "Failed to escape table name '%s'", ZSTR_VAL(table));
5348+
return FAILURE;
5349+
}
52945350
smart_str_appendc(querystr, '.');
52955351
smart_str_appends(querystr, escaped);
52965352
PQfreemem(escaped);
52975353
}
52985354
}
5355+
5356+
return SUCCESS;
52995357
}
53005358
/* }}} */
53015359

@@ -5316,7 +5374,9 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
53165374
ZVAL_UNDEF(&converted);
53175375
if (zend_hash_num_elements(Z_ARRVAL_P(var_array)) == 0) {
53185376
smart_str_appends(&querystr, "INSERT INTO ");
5319-
build_tablename(&querystr, pg_link, table);
5377+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5378+
goto cleanup;
5379+
}
53205380
smart_str_appends(&querystr, " DEFAULT VALUES");
53215381

53225382
goto no_values;
@@ -5332,7 +5392,9 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
53325392
}
53335393

53345394
smart_str_appends(&querystr, "INSERT INTO ");
5335-
build_tablename(&querystr, pg_link, table);
5395+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5396+
goto cleanup;
5397+
}
53365398
smart_str_appends(&querystr, " (");
53375399

53385400
ZEND_HASH_FOREACH_STR_KEY(Z_ARRVAL_P(var_array), fld) {
@@ -5342,6 +5404,10 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
53425404
}
53435405
if (opt & PGSQL_DML_ESCAPE) {
53445406
tmp = PQescapeIdentifier(pg_link, ZSTR_VAL(fld), ZSTR_LEN(fld) + 1);
5407+
if (tmp == NULL) {
5408+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s'", ZSTR_VAL(fld));
5409+
goto cleanup;
5410+
}
53455411
smart_str_appends(&querystr, tmp);
53465412
PQfreemem(tmp);
53475413
} else {
@@ -5353,15 +5419,19 @@ PHP_PGSQL_API zend_result php_pgsql_insert(PGconn *pg_link, const zend_string *t
53535419
smart_str_appends(&querystr, ") VALUES (");
53545420

53555421
/* make values string */
5356-
ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(var_array), val) {
5422+
ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(var_array), fld, val) {
53575423
/* we can avoid the key_type check here, because we tested it in the other loop */
53585424
switch (Z_TYPE_P(val)) {
53595425
case IS_STRING:
53605426
if (opt & PGSQL_DML_ESCAPE) {
5361-
size_t new_len;
5362-
char *tmp;
5363-
tmp = (char *)safe_emalloc(Z_STRLEN_P(val), 2, 1);
5364-
new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5427+
int error;
5428+
char *tmp = safe_emalloc(Z_STRLEN_P(val), 2, 1);
5429+
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), &error);
5430+
if (error) {
5431+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s' value", ZSTR_VAL(fld));
5432+
efree(tmp);
5433+
goto cleanup;
5434+
}
53655435
smart_str_appendc(&querystr, '\'');
53665436
smart_str_appendl(&querystr, tmp, new_len);
53675437
smart_str_appendc(&querystr, '\'');
@@ -5517,6 +5587,10 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr,
55175587
}
55185588
if (opt & PGSQL_DML_ESCAPE) {
55195589
char *tmp = PQescapeIdentifier(pg_link, ZSTR_VAL(fld), ZSTR_LEN(fld) + 1);
5590+
if (tmp == NULL) {
5591+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s'", ZSTR_VAL(fld));
5592+
return -1;
5593+
}
55205594
smart_str_appends(querystr, tmp);
55215595
PQfreemem(tmp);
55225596
} else {
@@ -5532,8 +5606,14 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr,
55325606
switch (Z_TYPE_P(val)) {
55335607
case IS_STRING:
55345608
if (opt & PGSQL_DML_ESCAPE) {
5609+
int error;
55355610
char *tmp = (char *)safe_emalloc(Z_STRLEN_P(val), 2, 1);
5536-
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), NULL);
5611+
size_t new_len = PQescapeStringConn(pg_link, tmp, Z_STRVAL_P(val), Z_STRLEN_P(val), &error);
5612+
if (error) {
5613+
php_error_docref(NULL, E_NOTICE, "Failed to escape field '%s' value", ZSTR_VAL(fld));
5614+
efree(tmp);
5615+
return -1;
5616+
}
55375617
smart_str_appendc(querystr, '\'');
55385618
smart_str_appendl(querystr, tmp, new_len);
55395619
smart_str_appendc(querystr, '\'');
@@ -5601,7 +5681,9 @@ PHP_PGSQL_API zend_result php_pgsql_update(PGconn *pg_link, const zend_string *t
56015681
}
56025682

56035683
smart_str_appends(&querystr, "UPDATE ");
5604-
build_tablename(&querystr, pg_link, table);
5684+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5685+
goto cleanup;
5686+
}
56055687
smart_str_appends(&querystr, " SET ");
56065688

56075689
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(var_array), 0, ",", 1, opt))
@@ -5704,7 +5786,9 @@ PHP_PGSQL_API zend_result php_pgsql_delete(PGconn *pg_link, const zend_string *t
57045786
}
57055787

57065788
smart_str_appends(&querystr, "DELETE FROM ");
5707-
build_tablename(&querystr, pg_link, table);
5789+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5790+
goto cleanup;
5791+
}
57085792
smart_str_appends(&querystr, " WHERE ");
57095793

57105794
if (build_assignment_string(pg_link, &querystr, Z_ARRVAL_P(ids_array), 1, " AND ", sizeof(" AND ")-1, opt))
@@ -5844,7 +5928,9 @@ PHP_PGSQL_API zend_result php_pgsql_select(PGconn *pg_link, const zend_string *t
58445928
}
58455929

58465930
smart_str_appends(&querystr, "SELECT * FROM ");
5847-
build_tablename(&querystr, pg_link, table);
5931+
if (build_tablename(&querystr, pg_link, table) == FAILURE) {
5932+
goto cleanup;
5933+
}
58485934
smart_str_appends(&querystr, " WHERE ");
58495935

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

0 commit comments

Comments
 (0)