Skip to content

Commit 2ab18e1

Browse files
GirgiasGirgias
committed
Convert session_name from char to zend_string
Also fixed a bug where nul bytes where not properly checked
1 parent 24f7fbc commit 2ab18e1

File tree

5 files changed

+57
-40
lines changed

5 files changed

+57
-40
lines changed

‎ext/session/php_session.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ typedef struct _php_session_rfc1867_progress {
141141

142142
typedef struct _php_ps_globals {
143143
char *save_path;
144-
char *session_name;
144+
zend_string *session_name;
145145
zend_string *id;
146146
zend_string *extern_referer_chk;
147147
zend_string *cache_limiter;

‎ext/session/session.c

Lines changed: 51 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -390,7 +390,7 @@ static zend_result php_session_initialize(void) /* {{{ */
390390
}
391391

392392
/* Open session handler first */
393-
if (PS(mod)->s_open(&PS(mod_data), PS(save_path), PS(session_name)) == FAILURE
393+
if (PS(mod)->s_open(&PS(mod_data), PS(save_path), ZSTR_VAL(PS(session_name))) == FAILURE
394394
/* || PS(mod_data) == NULL */ /* FIXME: open must set valid PS(mod_data) with success */
395395
) {
396396
php_session_abort();
@@ -648,24 +648,41 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */
648648
SESSION_CHECK_ACTIVE_STATE;
649649
SESSION_CHECK_OUTPUT_STATE;
650650

651-
/* Numeric session.name won't work at all */
652-
if ((!ZSTR_LEN(new_value) || is_numeric_string(ZSTR_VAL(new_value), ZSTR_LEN(new_value), NULL, NULL, 0))) {
653-
int err_type;
651+
int err_type;
654652

655-
if (stage == ZEND_INI_STAGE_RUNTIME || stage == ZEND_INI_STAGE_ACTIVATE || stage == ZEND_INI_STAGE_STARTUP) {
656-
err_type = E_WARNING;
657-
} else {
658-
err_type = E_ERROR;
659-
}
653+
if (stage == ZEND_INI_STAGE_RUNTIME || stage == ZEND_INI_STAGE_ACTIVATE || stage == ZEND_INI_STAGE_STARTUP) {
654+
err_type = E_WARNING;
655+
} else {
656+
err_type = E_ERROR;
657+
}
660658

659+
if (ZSTR_LEN(new_value) == 0) {
660+
/* Do not output error when restoring ini options. */
661+
if (stage != ZEND_INI_STAGE_DEACTIVATE) {
662+
php_error_docref(NULL, err_type, "session.name \"%s\" cannot be empty", ZSTR_VAL(new_value));
663+
}
664+
return FAILURE;
665+
}
666+
/* Nul bytes are not allowed */
667+
if (ZSTR_LEN(new_value) != strlen(ZSTR_VAL(new_value))) {
668+
/* Do not output error when restoring ini options. */
669+
if (stage != ZEND_INI_STAGE_DEACTIVATE) {
670+
php_error_docref(NULL, err_type, "session.name \"%s\" cannot contain nul bytes", ZSTR_VAL(new_value));
671+
}
672+
return FAILURE;
673+
}
674+
/* Numeric session.name won't work at all */
675+
if (is_numeric_str_function(new_value, NULL, NULL)) {
661676
/* Do not output error when restoring ini options. */
662677
if (stage != ZEND_INI_STAGE_DEACTIVATE) {
663678
php_error_docref(NULL, err_type, "session.name \"%s\" cannot be numeric or empty", ZSTR_VAL(new_value));
664679
}
665680
return FAILURE;
666681
}
667682

668-
return OnUpdateStringUnempty(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
683+
zend_string **p = (zend_string **) ZEND_INI_GET_ADDR();
684+
*p = new_value;
685+
return SUCCESS;
669686
}
670687
/* }}} */
671688

@@ -1251,9 +1268,10 @@ static void php_session_remove_cookie(void) {
12511268
size_t session_cookie_len;
12521269
size_t len = sizeof("Set-Cookie")-1;
12531270

1254-
ZEND_ASSERT(strpbrk(PS(session_name), "=,; \t\r\n\013\014") == NULL);
1255-
spprintf(&session_cookie, 0, "Set-Cookie: %s=", PS(session_name));
1271+
ZEND_ASSERT(strpbrk(ZSTR_VAL(PS(session_name)), "=,; \t\r\n\013\014") == NULL);
1272+
spprintf(&session_cookie, 0, "Set-Cookie: %s=", ZSTR_VAL(PS(session_name)));
12561273

1274+
// TODO Manually compute from known information?
12571275
session_cookie_len = strlen(session_cookie);
12581276
current = l->head;
12591277
while (current) {
@@ -1298,8 +1316,9 @@ static zend_result php_session_send_cookie(void) /* {{{ */
12981316
return FAILURE;
12991317
}
13001318

1319+
// TODO need to Check for nul byte?
13011320
/* Prevent broken Set-Cookie header, because the session_name might be user supplied */
1302-
if (strpbrk(PS(session_name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */
1321+
if (strpbrk(ZSTR_VAL(PS(session_name)), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */
13031322
php_error_docref(NULL, E_WARNING, "session.name cannot contain any of the following '=,; \\t\\r\\n\\013\\014'");
13041323
return FAILURE;
13051324
}
@@ -1308,7 +1327,7 @@ static zend_result php_session_send_cookie(void) /* {{{ */
13081327
e_id = php_url_encode(ZSTR_VAL(PS(id)), ZSTR_LEN(PS(id)));
13091328

13101329
smart_str_appendl(&ncookie, "Set-Cookie: ", sizeof("Set-Cookie: ")-1);
1311-
smart_str_appendl(&ncookie, PS(session_name), strlen(PS(session_name)));
1330+
smart_str_append(&ncookie, PS(session_name));
13121331
smart_str_appendc(&ncookie, '=');
13131332
smart_str_appendl(&ncookie, ZSTR_VAL(e_id), ZSTR_LEN(e_id));
13141333

@@ -1434,7 +1453,7 @@ PHPAPI zend_result php_session_reset_id(void) /* {{{ */
14341453
if (PS(define_sid)) {
14351454
smart_str var = {0};
14361455

1437-
smart_str_appends(&var, PS(session_name));
1456+
smart_str_append(&var, PS(session_name));
14381457
smart_str_appendc(&var, '=');
14391458
smart_str_appends(&var, ZSTR_VAL(PS(id)));
14401459
smart_str_0(&var);
@@ -1462,18 +1481,15 @@ PHPAPI zend_result php_session_reset_id(void) /* {{{ */
14621481
(data = zend_hash_str_find(&EG(symbol_table), "_COOKIE", sizeof("_COOKIE") - 1))) {
14631482
ZVAL_DEREF(data);
14641483
if (Z_TYPE_P(data) == IS_ARRAY &&
1465-
(ppid = zend_hash_str_find(Z_ARRVAL_P(data), PS(session_name), strlen(PS(session_name))))) {
1484+
(ppid = zend_hash_find(Z_ARRVAL_P(data), PS(session_name)))) {
14661485
ZVAL_DEREF(ppid);
14671486
apply_trans_sid = 0;
14681487
}
14691488
}
14701489
}
14711490
if (apply_trans_sid) {
1472-
zend_string *sname;
1473-
sname = zend_string_init(PS(session_name), strlen(PS(session_name)), 0);
1474-
php_url_scanner_reset_session_var(sname, 1); /* This may fail when session name has changed */
1475-
zend_string_release_ex(sname, 0);
1476-
php_url_scanner_add_session_var(PS(session_name), strlen(PS(session_name)), ZSTR_VAL(PS(id)), ZSTR_LEN(PS(id)), 1);
1491+
php_url_scanner_reset_session_var(PS(session_name), 1); /* This may fail when session name has changed */
1492+
php_url_scanner_add_session_var(ZSTR_VAL(PS(session_name)), ZSTR_LEN(PS(session_name)), ZSTR_VAL(PS(id)), ZSTR_LEN(PS(id)), 1);
14771493
}
14781494
return SUCCESS;
14791495
}
@@ -1485,7 +1501,6 @@ PHPAPI zend_result php_session_start(void) /* {{{ */
14851501
zval *ppid;
14861502
zval *data;
14871503
char *p, *value;
1488-
size_t lensess;
14891504

14901505
switch (PS(session_status)) {
14911506
case php_session_active:
@@ -1520,8 +1535,6 @@ PHPAPI zend_result php_session_start(void) /* {{{ */
15201535
PS(send_cookie) = PS(use_cookies) || PS(use_only_cookies);
15211536
}
15221537

1523-
lensess = strlen(PS(session_name));
1524-
15251538
/*
15261539
* Cookies are preferred, because initially cookie and get
15271540
* variables will be available.
@@ -1533,7 +1546,7 @@ PHPAPI zend_result php_session_start(void) /* {{{ */
15331546
if (!PS(id)) {
15341547
if (PS(use_cookies) && (data = zend_hash_str_find(&EG(symbol_table), "_COOKIE", sizeof("_COOKIE") - 1))) {
15351548
ZVAL_DEREF(data);
1536-
if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_str_find(Z_ARRVAL_P(data), PS(session_name), lensess))) {
1549+
if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_find(Z_ARRVAL_P(data), PS(session_name)))) {
15371550
ppid2sid(ppid);
15381551
PS(send_cookie) = 0;
15391552
PS(define_sid) = 0;
@@ -1543,13 +1556,13 @@ PHPAPI zend_result php_session_start(void) /* {{{ */
15431556
if (!PS(use_only_cookies)) {
15441557
if (!PS(id) && (data = zend_hash_str_find(&EG(symbol_table), "_GET", sizeof("_GET") - 1))) {
15451558
ZVAL_DEREF(data);
1546-
if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_str_find(Z_ARRVAL_P(data), PS(session_name), lensess))) {
1559+
if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_find(Z_ARRVAL_P(data), PS(session_name)))) {
15471560
ppid2sid(ppid);
15481561
}
15491562
}
15501563
if (!PS(id) && (data = zend_hash_str_find(&EG(symbol_table), "_POST", sizeof("_POST") - 1))) {
15511564
ZVAL_DEREF(data);
1552-
if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_str_find(Z_ARRVAL_P(data), PS(session_name), lensess))) {
1565+
if (Z_TYPE_P(data) == IS_ARRAY && (ppid = zend_hash_find(Z_ARRVAL_P(data), PS(session_name)))) {
15531566
ppid2sid(ppid);
15541567
}
15551568
}
@@ -1559,11 +1572,11 @@ PHPAPI zend_result php_session_start(void) /* {{{ */
15591572
if (!PS(id) && zend_is_auto_global(ZSTR_KNOWN(ZEND_STR_AUTOGLOBAL_SERVER)) == SUCCESS &&
15601573
(data = zend_hash_str_find(Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]), "REQUEST_URI", sizeof("REQUEST_URI") - 1)) &&
15611574
Z_TYPE_P(data) == IS_STRING &&
1562-
(p = strstr(Z_STRVAL_P(data), PS(session_name))) &&
1563-
p[lensess] == '='
1575+
(p = strstr(Z_STRVAL_P(data), ZSTR_VAL(PS(session_name)))) &&
1576+
p[ZSTR_LEN(PS(session_name))] == '='
15641577
) {
15651578
char *q;
1566-
p += lensess + 1;
1579+
p += ZSTR_LEN(PS(session_name));
15671580
if ((q = strpbrk(p, "/?\\"))) {
15681581
PS(id) = zend_string_init(p, q - p, 0);
15691582
}
@@ -1644,7 +1657,7 @@ static zend_result php_session_reset(void) /* {{{ */
16441657
PHPAPI void session_adapt_url(const char *url, size_t url_len, char **new_url, size_t *new_len) /* {{{ */
16451658
{
16461659
if (APPLY_TRANS_SID && (PS(session_status) == php_session_active)) {
1647-
*new_url = php_url_scanner_adapt_single_url(url, url_len, PS(session_name), ZSTR_VAL(PS(id)), new_len, 1);
1660+
*new_url = php_url_scanner_adapt_single_url(url, url_len, ZSTR_VAL(PS(session_name)), ZSTR_VAL(PS(id)), new_len, 1);
16481661
}
16491662
}
16501663
/* }}} */
@@ -1865,7 +1878,8 @@ PHP_FUNCTION(session_name)
18651878
RETURN_FALSE;
18661879
}
18671880

1868-
RETVAL_STRING(PS(session_name));
1881+
// TODO Prevent duplication???
1882+
RETVAL_STR(zend_string_dup(PS(session_name), false));
18691883

18701884
if (name) {
18711885
ini_name = zend_string_init("session.name", sizeof("session.name") - 1, 0);
@@ -2240,7 +2254,7 @@ PHP_FUNCTION(session_regenerate_id)
22402254
zend_string_release_ex(PS(id), 0);
22412255
PS(id) = NULL;
22422256

2243-
if (PS(mod)->s_open(&PS(mod_data), PS(save_path), PS(session_name)) == FAILURE) {
2257+
if (PS(mod)->s_open(&PS(mod_data), PS(save_path), ZSTR_VAL(PS(session_name))) == FAILURE) {
22442258
PS(session_status) = php_session_none;
22452259
if (!EG(exception)) {
22462260
zend_throw_error(NULL, "Failed to open session: %s (path: %s)", PS(mod)->s_name, PS(save_path));
@@ -2906,7 +2920,7 @@ static bool early_find_sid_in(zval *dest, int where, php_session_rfc1867_progres
29062920
return 0;
29072921
}
29082922

2909-
if ((ppid = zend_hash_str_find(Z_ARRVAL(PG(http_globals)[where]), PS(session_name), progress->sname_len))
2923+
if ((ppid = zend_hash_find(Z_ARRVAL(PG(http_globals)[where]), PS(session_name)))
29102924
&& Z_TYPE_P(ppid) == IS_STRING) {
29112925
zval_ptr_dtor(dest);
29122926
ZVAL_COPY_DEREF(dest, ppid);
@@ -3014,7 +3028,8 @@ static zend_result php_session_rfc1867_callback(unsigned int event, void *event_
30143028
multipart_event_start *data = (multipart_event_start *) event_data;
30153029
progress = ecalloc(1, sizeof(php_session_rfc1867_progress));
30163030
progress->content_length = data->content_length;
3017-
progress->sname_len = strlen(PS(session_name));
3031+
// TODO Remove field?
3032+
progress->sname_len = ZSTR_LEN(PS(session_name));
30183033
PS(rfc1867_progress) = progress;
30193034
}
30203035
break;
@@ -3036,7 +3051,7 @@ static zend_result php_session_rfc1867_callback(unsigned int event, void *event_
30363051
if (data->name && data->value && value_len) {
30373052
size_t name_len = strlen(data->name);
30383053

3039-
if (name_len == progress->sname_len && memcmp(data->name, PS(session_name), name_len) == 0) {
3054+
if (name_len == progress->sname_len && memcmp(data->name, ZSTR_VAL(PS(session_name)), name_len) == 0) {
30403055
zval_ptr_dtor(&progress->sid);
30413056
ZVAL_STRINGL(&progress->sid, (*data->value), value_len);
30423057
} else if (name_len == strlen(PS(rfc1867_name)) && memcmp(data->name, PS(rfc1867_name), name_len + 1) == 0) {

‎ext/session/tests/bug66481.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,6 @@ var_dump(session_name("foo"));
1515
var_dump(session_name("bar"));
1616
?>
1717
--EXPECT--
18-
Warning: PHP Startup: session.name "" cannot be numeric or empty in Unknown on line 0
18+
Warning: PHP Startup: session.name "" cannot be empty in Unknown on line 0
1919
string(9) "PHPSESSID"
2020
string(3) "foo"

‎ext/session/tests/rfc1867_sid_post.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ var_dump($_SESSION["upload_progress_" . basename(__FILE__)]);
4747
session_destroy();
4848
?>
4949
--EXPECTF--
50-
string(%d) "rfc1867-sid-post"
50+
string(16) "rfc1867-sid-post"
5151
bool(true)
5252
array(2) {
5353
["file1"]=>

‎ext/session/tests/session_name_variation1.phpt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ ob_end_flush();
3838
?>
3939
--EXPECTF--
4040
*** Testing session_name() : variation ***
41+
42+
Warning: session_name(): session.name "" cannot contain nul bytes in %s on line %d
4143
string(9) "PHPSESSID"
4244
bool(true)
4345
string(9) "PHPSESSID"
@@ -51,7 +53,7 @@ string(1) " "
5153
bool(true)
5254
string(1) " "
5355

54-
Warning: session_name(): session.name "" cannot be numeric or empty in %s on line %d
56+
Warning: session_name(): session.name "" cannot be empty in %s on line %d
5557
string(1) " "
5658

5759
Warning: session_start(): session.name cannot contain any of the following '=,; \t\r\n\013\014' in %s on line %d

0 commit comments

Comments
 (0)