Check session_create_id() input for null byte #14728
Conversation
ext/session/session.c
Outdated
| if(memchr(ZSTR_VAL(string), '\0', ZSTR_LEN(string)) != NULL) { | ||
| return FAILURE; | ||
| } |
There was a problem hiding this comment.
Just use the zend_str_has_nul_byte() function.
ext/session/session.c
Outdated
| } | ||
|
|
||
| if (prefix && ZSTR_LEN(prefix)) { | ||
| if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE) { | ||
| if (php_session_valid_key(prefix) == FAILURE) { |
There was a problem hiding this comment.
You could move the nul-byte check into the parameter parsing using the P specifier.
There was a problem hiding this comment.
I'm sorry but I don't know what is P specifier so I chose your other idea.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the explanation. Which way should I choose then? I'm open to the choice.
There was a problem hiding this comment.
I would move it into the ZPP check, as then it throws a ValueError instead of a warning.
There was a problem hiding this comment.
Thanks. It's a much cleaner solution and simpler. Applied changes
ext/session/session.c
Outdated
| /* E_ERROR raised for security reason. */ | ||
| php_error_docref(NULL, E_WARNING, "Prefix cannot contain special characters. Only the A-Z, a-z, 0-9, \"-\", and \",\" characters are allowed"); | ||
| RETURN_FALSE; |
There was a problem hiding this comment.
Also this comment is wrong. Not sure how this happened, and if the E_ERROR got demoted would be good to know why. Not that I will be much of an issue if we promote this to a ValueError.
There was a problem hiding this comment.
I tried to track it but even in 2020 there were incorrect comments: f293e6b#diff-22481788aee01357f8e628c9191632d127a78a2ea20cfbf9c3354cdc420d4ff2L2277
There was a problem hiding this comment.
Yeah, well that's less than ideal.
ext/session/session.c
Outdated
| @@ -2379,7 +2373,7 @@ PHP_FUNCTION(session_create_id) | |||
| } | |||
|
|
|||
| if (prefix && ZSTR_LEN(prefix)) { | |||
| if (php_session_valid_key(ZSTR_VAL(prefix)) == FAILURE) { | |||
| if (php_session_valid_key(prefix) == FAILURE) { | |||
| /* E_ERROR raised for security reason. */ | |||
| php_error_docref(NULL, E_WARNING, "Prefix cannot contain special characters. Only the A-Z, a-z, 0-9, \"-\", and \",\" characters are allowed"); | |||
There was a problem hiding this comment.
Maybe the warning should use [A-Z] to show character ranges like a regex? Not sure about this and can be done in a follow-up PR.
3841447
to
844856c
Compare
|
I'd like to see a test where you use |
The function
session_create_id()allows only selected characters amongst which there is no NULL byte, however the validation doesn't check for it.This PR checks for a NULL byte in the function
php_session_valid_keyused by the PHP functionsession_create_id().