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

Fix potential OOB when checking for trailing spaces #17471

Closed
wants to merge 2 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jan 14, 2025

If path_len is zero, we must not access path, let alone try to subtract -1 from it.

Since path and path_len are supposed to come from a zend_string, this is not a security issue.


Note that I'm not 100% sure whether the patch is correct, since php_win32_check_trailing_space apparently does not only check for trailing spaces, but also for leading spaces.

If `path_len` is zero, we must not access `path`, let alone try to
subtract `-1` from it.

Since `path` and `path_len` are supposed to come from a `zend_string`,
this is not a security issue.
@cmb69 cmb69 marked this pull request as draft January 14, 2025 15:15
@cmb69
Copy link
Member Author

cmb69 commented Jan 14, 2025

Need to check the failing tests.

@cmb69
Copy link
Member Author

cmb69 commented Jan 15, 2025

php-src/win32/winutil.c

Lines 57 to 71 in 7512685

int php_win32_check_trailing_space(const char * path, const size_t path_len)
{/*{{{*/
if (path_len > MAXPATHLEN - 1) {
return 1;
}
if (path) {
if (path[0] == ' ' || path[path_len - 1] == ' ') {
return 0;
} else {
return 1;
}
} else {
return 0;
}
}/*}}}*/

Okay, classic C style function which checks if there is a leading or trailing space. So far, so good. However, why would it return zero if path == NULL, but one, if the path is too long? The commit message isn't particularly revealing about the handling of the edge cases.

Anyhow, I'm mostly concerned about avoiding the OOB reads, and like to target PHP-8.3, so I'm trying to stick closely to the current behavior.

@cmb69 cmb69 marked this pull request as ready for review January 15, 2025 14:50
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me as a backport fix.

@cmb69 cmb69 closed this in ed8b111 Jan 15, 2025
@cmb69 cmb69 deleted the cmb/php_win32_check_trailing_space branch January 15, 2025 23:04
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.

2 participants