Repository navigation
Conversation
|
The LiveOverflow video of php2shell shows a leak via the PHP session cookie: 
This seems like it shouldn't be possible. PHP does validate the session ID, but apparently then it has already been used in a cookie. While looking into this I found this bug where the session id is only validated up unto the first null byte. After this is fixed, perhaps it is possible to move the validation up so that an invalid identifier is not used in the cookie value. |
arnaud-lb
left a comment
arnaud-lb
left a comment
There was a problem hiding this comment.
This is a good idea.
This seems like it shouldn't be possible
It's normally not possible, but in the pre-patched PHP version PS(id) can be uninitialized (therefore not validated) under some conditions.
iliaal
left a comment
iliaal
left a comment
There was a problem hiding this comment.
Good idea, in my tests setcookie was main leak vector that makes php2shell work
| * ps_modules appropriately */ | ||
| PHPAPI zend_result php_session_valid_key(const char *key) /* {{{ */ | ||
| * ps_modules appropriately. */ | ||
| static zend_result php_session_valid_key_len(const char *key, size_t key_len) /* {{{ */ |
There was a problem hiding this comment.
| static zend_result php_session_valid_key_len(const char *key, size_t key_len) /* {{{ */ | |
| static zend_result php_session_valid_key_len(const char *key, size_t key_len) |
nit: Please remove those markers in functions you touch.
There was a problem hiding this comment.
Done. If this is a policy it would be nice if it is documented somewhere.
| return FAILURE; | ||
| } | ||
| /* Validate the full zend_string, including embedded NUL bytes. */ | ||
| PHPAPI zend_result php_session_valid_key_ex(const zend_string *key) /* {{{ */ |
There was a problem hiding this comment.
I don't think PHPAPI should be added in older versions. Would be nice to have follow up with cleanup on master.
There was a problem hiding this comment.
Normally I'd agree, but the original php2shell fix already introduced some new "APIs", and this seems harmless (imho)
There was a problem hiding this comment.
My understanding as an RM is adding functions doesn't break ABI, but when it does get merged, you can't change or remove it, so it's less needing approval to break ABI and more scrutiny to make sure we don't need to break ABI again. But this matches other patterns I see (zend_string variant on _ex), so it makes sense to me at least.
There was a problem hiding this comment.
That's why I think it's not necessary to be included in older versions, and in master old one should be deprecated in favor of new one. It's not so important though, but I also don't think that php2shell should be treated as excuse to not care about API hygiene.
No description provided.