Skip to content
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.

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

php: Fix memory corruption for uwsgi_cache_* #2108

Merged
merged 1 commit into from Jan 19, 2020

Conversation

aszlig
Copy link
Contributor

@aszlig aszlig commented Dec 14, 2019

Ah the joys of variadic arguments in C...

So, when using zend_parse_parameters(), PHP internally loops through the type specifiers and accordingly uses va_arg() to get the corresponding argument.

Since the arguments are expected to be pointers to the corresponding values, the size of them does matter, because PHP simply writes to the corresponding address with a size of size_t.

If we for example pass a pointer to a 32bit integer and PHP writes 64 bits, we have an overflow of 4 bytes.

From README.PARAMETER_PARSING_API in the PHP source tree:

Please note that since version 7 PHP uses zend_long as integer type
and zend_string with size_t as length, so make sure you pass
zend_longs to "l" and size_t to strings length (i.e. for "s" you need
to pass char * and size_t), not the other way round!

Both mistakes might cause memory corruptions and segfaults:

  1. char str;
    long str_len; /
    XXX THIS IS WRONG!! Use size_t instead. */
    zend_parse_parameters(ZEND_NUM_ARGS(), "s", &str, &str_len)

  2. int num; /* XXX THIS IS WRONG!! Use zend_long instead. */
    zend_parse_parameters(ZEND_NUM_ARGS(), "l", &num)

To fix this, I changed the types accordingly to use size_t and zend_long if the PHP major version is >= 7.

plugins/php/php_plugin.c Outdated Show resolved Hide resolved
plugins/php/php_plugin.c Outdated Show resolved Hide resolved
Ah the joys of variadic arguments in C...

So, when using zend_parse_parameters(), PHP internally loops through the
type specifiers and accordingly uses va_arg() to get the corresponding
argument.

Since the arguments are expected to be pointers to the corresponding
values, the size of them *does* matter, because PHP simply writes to the
corresponding address with a size of size_t.

If we for example pass a pointer to a 32bit integer and PHP writes 64
bits, we have an overflow of 4 bytes.

From README.PARAMETER_PARSING_API in the PHP source tree:

> Please note that since version 7 PHP uses zend_long as integer type
> and zend_string with size_t as length, so make sure you pass
> zend_longs to "l" and size_t to strings length (i.e. for "s" you need
> to pass char * and size_t), not the other way round!
>
> Both mistakes might cause memory corruptions and segfaults:
> 1)
>   char *str;
>   long str_len; /* XXX THIS IS WRONG!! Use size_t instead. */
>   zend_parse_parameters(ZEND_NUM_ARGS(), "s", &str, &str_len)
>
> 2)
>   int num; /* XXX THIS IS WRONG!! Use zend_long instead. */
>   zend_parse_parameters(ZEND_NUM_ARGS(), "l", &num)

To fix this, I changed the types accordingly to use size_t and
zend_long if the PHP major version is >= 7.

Signed-off-by: aszlig <aszlig@nix.build>
@xrmx xrmx merged commit e18f6e1 into unbit:master Jan 19, 2020
@aszlig
Copy link
Contributor Author

aszlig commented Apr 29, 2022

@xrmx: Seeing that this issue still persists in version 2.0.20, could you please cherry-pick this to uwsgi-2.0? Alternatively I could open a new pull request based on uwsgi-2.0 if that's preferred in your workflow.

@xrmx
Copy link
Collaborator

xrmx commented Apr 29, 2022

@aszlig backported here xrmx@a5c1e28 , will merge here once the ci finishes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants