Skip to content

Check session_create_id() input for null byte #14728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 6, 2024
Merged

Check session_create_id() input for null byte #14728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,17 +360,15 @@ PHPAPI zend_result php_session_valid_key(const char *key) /* {{{ */
size_t len;
const char *p;
char c;
zend_result ret = SUCCESS;

for (p = key; (c = *p); p++) {
/* valid characters are a..z,A..Z,0..9 */
/* valid characters are [a-z], [A-Z], [0-9], - (hyphen) and , (comma) */
if (!((c >= 'a' && c <= 'z')
|| (c >= 'A' && c <= 'Z')
|| (c >= '0' && c <= '9')
|| c == ','
|| c == '-')) {
ret = FAILURE;
break;
return FAILURE;
}
}

Expand All @@ -379,10 +377,10 @@ PHPAPI zend_result php_session_valid_key(const char *key) /* {{{ */
/* Somewhat arbitrary length limit here, but should be way more than
anyone needs and avoids file-level warnings later on if we exceed MAX_PATH */
if (len == 0 || len > PS_MAX_SID_LENGTH) {
ret = FAILURE;
return FAILURE;
}

return ret;
return SUCCESS;
}
/* }}} */

Expand Down Expand Up @@ -2374,7 +2372,7 @@ PHP_FUNCTION(session_create_id)
zend_string *prefix = NULL, *new_id;
smart_str id = {0};

if (zend_parse_parameters(ZEND_NUM_ARGS(), "|S", &prefix) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|P", &prefix) == FAILURE) {
RETURN_THROWS();
}

Expand Down
8 changes: 8 additions & 0 deletions ext/session/tests/session_create_id_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ echo "*** Testing session_create_id() : basic functionality ***\n";

// No session
var_dump(session_create_id());
var_dump(session_create_id(''));
var_dump(session_create_id(','));
var_dump(session_create_id('-'));
var_dump(session_create_id('0123456789'));
var_dump(session_create_id('ABCD'));

ini_set('session.use_strict_mode', true);
Expand All @@ -42,6 +46,10 @@ ob_end_flush();
--EXPECTF--
*** Testing session_create_id() : basic functionality ***
string(32) "%s"
string(32) "%s"
string(33) ",%s"
string(33) "-%s"
string(42) "0123456789%s"
string(36) "ABCD%s"
string(35) "XYZ%s"
string(0) ""
Expand Down
31 changes: 31 additions & 0 deletions ext/session/tests/session_create_id_invalid_prefix.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
--TEST--
Test session_create_id() function : invalid prefix
--INI--
session.save_handler=files
session.sid_length=32
--EXTENSIONS--
session
--SKIPIF--
<?php include('skipif.inc'); ?>
--FILE--
<?php

var_dump(session_create_id('_'));
var_dump(session_create_id('%'));
var_dump(session_create_id("AB\0CD"));


?>
Done
--EXPECTF--
Warning: session_create_id(): Prefix cannot contain special characters. Only the A-Z, a-z, 0-9, "-", and "," characters are allowed in %s on line %d
bool(false)

Warning: session_create_id(): Prefix cannot contain special characters. Only the A-Z, a-z, 0-9, "-", and "," characters are allowed in %s on line %d
bool(false)

Fatal error: Uncaught ValueError: session_create_id(): Argument #1 ($prefix) must not contain any null bytes in %s:%d
Stack trace:
#0 %s(5): session_create_id('AB\x00CD')
#1 {main}
thrown in %s