Skip to content

Implement null byte checks for hostnames and addresses - #24770

Open
SleepTheGod wants to merge 1 commit into
php:masterfrom
SleepTheGod:master
Open

SleepTheGod wants to merge 1 commit into
php:masterfrom
SleepTheGod:master

Conversation

@SleepTheGod

Copy link
Copy Markdown

Added checks to reject hostnames and addresses containing null bytes to mitigate CVE-2025-1220.

Added checks to reject hostnames and addresses containing null bytes to mitigate CVE-2025-1220.
@SleepTheGod
SleepTheGod requested a review from bukka as a code owner October 27, 2026 03:18
@SleepTheGod

Copy link
Copy Markdown
Author

all checks passed

@devnexen

devnexen commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

it would be nice to add a test that would trigger the issue without your fix ... if the issue still occur.

@ArtUkrainskiy

Copy link
Copy Markdown
Contributor

if the issue still occur

nope

I ran this on current master without the patch, and the null byte is already rejected everywhere:

fsockopen("127.0.0.1\0evil", 80)          => The hostname must not contain null bytes
stream_socket_client("tcp://127.0.0.1\0evil:80") => The hostname must not contain null bytes
gethostbyname / dns_get_record            => ValueError: must not contain any null bytes

That's already fixed by 5ef0dc7 (GHSA-3cr5-j632-f35r = CVE-2025-1220). The new check in php_network_getaddresses can't fire anyway, since host is a C string and strlen stops at the null.

@SleepTheGod

Copy link
Copy Markdown
Author

Thanks @ArtUkrainskiy and @devnexen — you're both right, and I want to be explicit about what's happening here so we don't merge dead code.

The php_network_getaddresses check is unreachable.

php_network_getaddresses(const char *host, ...) takes a C string with no length. The only way to size it is strlen(host), which stops at the first \0. So memchr(host, '\0', strlen(host)) is NULL by definition — no caller can ever make that branch fire. That's why @ArtUkrainskiy couldn't reproduce it and why no test can be written for it: there is nothing to trigger.

The real fix is already in the callers.

CVE-2025-1220 was fixed in 5ef0dc7 (GHSA-3cr5-j632-f35r) at the layer that actually has the length available — fsockopen, stream_socket_client, dns_get_record, gethostbyname, etc., which reject embedded nulls using the zend_string length before the value reaches php_network_getaddresses. Moving the check down to php_network_getaddresses cannot catch anything the callers don't already catch, because the length information is gone by the time it gets there.

I'm dropping that hunk from this PR.

The one part of the patch that is not dead is the check in php_network_parse_network_address_with_port(const char *addr, size_t addrlen, ...). That function takes an explicit addrlen and is PHPAPI-exported, so third-party extensions can call it directly with a length that extends past an embedded null. I'll keep that hunk as defense-in-depth.

Force-pushed to remove the unreachable php_network_getaddresses change. If maintainers would prefer to close this entirely as redundant with 5ef0dc7, that's also fine by me — the parse_network_address_with_port check is the only thing here that isn't already covered elsewhere.

@SleepTheGod

Copy link
Copy Markdown
Author

Pulling this back to what actually needs fixing in this PR, and what shouldn't be here at all.

Keep — the php_network_parse_network_address_with_port null-byte guard

PHPAPI zend_result php_network_parse_network_address_with_port(
        const char *addr, size_t addrlen, struct sockaddr *sa, socklen_t *sl)
{
    ...
    /* Defense-in-depth: callers already validate via zend_string length,
     * but this function is PHPAPI-exported and third-party extensions can
     * call it directly with an addrlen that extends past an embedded null. */
    if (memchr(addr, '\0', addrlen) != NULL) {
        return FAILURE;
    }
    ...
}

This is the only hunk that isn't already covered by 5ef0dc7. addrlen is passed explicitly and may be larger than strlen(addr), so the check is reachable from any extension calling this function directly. It stays.

Drop — the php_network_getaddresses block

The size_t host_len = strlen(host); memchr(host, '\0', host_len) check and the "hostname must not contain null bytes" warning it emits are unreachable. host is a const char * with no companion length, so memchr over strlen(host) can never find a null. No caller can trigger it, and no test can be written for it — which is exactly what @ArtUkrainskiy reproduced. It's dead code and it obscures the one guard that matters.

Drop — the CVE-2025-1220 framing

5ef0dc7 / GHSA-3cr5-j632-f35r already ships the user-visible fix in the callers (fsockopen, stream_socket_client, dns_get_record, gethostbyname), where the zend_string length is still available. Retitling this PR to something like "main/network: reject null bytes in parse_network_address_with_port (defense-in-depth)" is honest about what it does. Keeping the CVE reference implies this is the fix, which it isn't, and it will confuse the changelog.

Separate PR — the session-engine hardening

The CVE-2026-109618 / CVE-2026-109619 work is not main/network.c and should not be in this diff:

  • PS(in_rfc1867_update) = false reset in php_rshutdown_session_globals(), so a longjmp() out of pre-RINIT deserialization cannot leave the guard set for the next request on the same worker.
  • zend_try / zend_catch around php_session_rfc1867_update() and php_session_rfc1867_cleanup(), clearing the flag before zend_bailout() rethrows.
  • ZVAL_UNDEF(&PS(http_session_vars)) immediately after zval_ptr_dtor() in php_session_track_init(), so a bailout during array_init() cannot leave a dangling pointer that rshutdown double-frees.

Those go in a PR against ext/session/session.c. Mixing network-layer hardening with session-engine hardening in one PR is how the wrong file ends up blamed for the wrong CVE.

Bottom line for this PR: one file, one hunk, one exported function. Everything else is either redundant with 5ef0dc7 or belongs in a different PR. Force-pushing that now.

Comment thread main/network.c
@SleepTheGod

Copy link
Copy Markdown
Author

Done — switched to zend_char_has_nul_byte() in the force-push. This PR now contains only the php_network_parse_network_address_with_port() defense-in-depth check; the unreachable php_network_getaddresses() hunk has been removed.

Thanks for the review, @TimWolla.
I will leave this for you guys to close or resolve.

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.

4 participants