Skip to content

Commit

Permalink
Detect invalid port in xp_socket parse ip address
Browse files Browse the repository at this point in the history
For historical reasons, fsockopen() accepts the port and hostname
separately: fsockopen('127.0.0.1', 80)

However, with the introdcution of stream transports in PHP 4.3,
it became possible to include the port in the hostname specifier:

fsockopen('127.0.0.1:80')
Or more formally: fsockopen('tcp://127.0.0.1:80')

Confusing results when these two forms are combined, however.
fsockopen('127.0.0.1:80', 443) results in fsockopen() attempting
to connect to '127.0.0.1:80:443' which any reasonable stack would
consider invalid.

Unfortunately, PHP parses the address looking for the first colon
(with special handling for IPv6, don't worry) and calls atoi()
from there.  atoi() in turn, simply stops parsing at the first
non-numeric character and returns the value so far.

The end result is that the explicitly supplied port is treated
as ignored garbage, rather than producing an error.

This diff replaces atoi() with strtol() and inspects the
stop character.  If additional "garbage" of any kind is found,
it fails and returns an error.
  • Loading branch information
sgolemon committed Mar 7, 2017
1 parent 549a30d commit bab0b99
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 11 deletions.
37 changes: 37 additions & 0 deletions ext/standard/tests/streams/parseip-001.phpt
@@ -0,0 +1,37 @@
--TEST--
Use of double-port in fsockopen()
--FILE--
<?php

$try = [
'127.0.0.1:80',
'tcp://127.0.0.1:80',
'[::1]:80',
'tcp://[::1]:80',
'localhost:80',
'tcp://localhost:80',
];

foreach ($try as $addr) {
echo "== $addr ==\n";
var_dump(@fsockopen($addr, 81, $errno, $errstr), $errstr);
}
--EXPECTF--
== 127.0.0.1:80 ==
bool(false)
string(41) "Failed to parse address "127.0.0.1:80:81""
== tcp://127.0.0.1:80 ==
bool(false)
string(41) "Failed to parse address "127.0.0.1:80:81""
== [::1]:80 ==
bool(false)
string(37) "Failed to parse address "[::1]:80:81""
== tcp://[::1]:80 ==
bool(false)
string(37) "Failed to parse address "[::1]:80:81""
== localhost:80 ==
bool(false)
string(41) "Failed to parse address "localhost:80:81""
== tcp://localhost:80 ==
bool(false)
string(41) "Failed to parse address "localhost:80:81""
29 changes: 18 additions & 11 deletions main/streams/xp_socket.c
Expand Up @@ -571,37 +571,44 @@ static inline char *parse_ip_address_ex(const char *str, size_t str_len, int *po
char *host = NULL;

#ifdef HAVE_IPV6
char *p;

if (*(str) == '[' && str_len > 1) {
/* IPV6 notation to specify raw address with port (i.e. [fe80::1]:80) */
p = memchr(str + 1, ']', str_len - 2);
char *p = memchr(str + 1, ']', str_len - 2), *e = NULL;
if (!p || *(p + 1) != ':') {
if (get_err) {
*err = strpprintf(0, "Failed to parse IPv6 address \"%s\"", str);
}
return NULL;
}
*portno = atoi(p + 2);
*portno = strtol(p + 2, &e, 10);
if (e && *e) {
if (get_err) {
*err = strpprintf(0, "Failed to parse address \"%s\"", str);
}
return NULL;
}
return estrndup(str + 1, p - str - 1);
}
#endif

if (str_len) {
colon = memchr(str, ':', str_len - 1);
} else {
colon = NULL;
}

if (colon) {
*portno = atoi(colon + 1);
host = estrndup(str, colon - str);
} else {
if (get_err) {
*err = strpprintf(0, "Failed to parse address \"%s\"", str);
char *e = NULL;
*portno = strtol(colon + 1, &e, 10);
if (!e || !*e) {
return estrndup(str, colon - str);
}
return NULL;
}

return host;
if (get_err) {
*err = strpprintf(0, "Failed to parse address \"%s\"", str);
}
return NULL;
}

static inline char *parse_ip_address(php_stream_xport_param *xparam, int *portno)
Expand Down

9 comments on commit bab0b99

@tinuzz
Copy link

@tinuzz tinuzz commented on bab0b99 Apr 13, 2017

Choose a reason for hiding this comment

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

This commit breaks some undocumented, but possibly widely used feature.

Using a remote socket description with an extra identifier like this:
tcp://localhost:80/foo
used to work, and now breaks.

The entire remote socket specifier is used as a 'persistent_id', which is used to get a cached persistent connection if available. Adding something unique to the end of the URI would force a new connection to be created instead of reusing a cached one.

I don't know if this was meant to work this way (it is not documented for stream_socket_client() but mentioned in the user comments), so I'm not sure if this is a bug. I know of at least one piece of software that relies on this to work, which is the Magento Redis cache/session storage module.

Regards,
Martijn.

@colinmollenhour
Copy link

@colinmollenhour colinmollenhour commented on bab0b99 Apr 13, 2017

Choose a reason for hiding this comment

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

Also mentioned on StackOverflow and a regression has already been reported by a user on a production system.

@tinuzz
Copy link

@tinuzz tinuzz commented on bab0b99 Apr 13, 2017

Choose a reason for hiding this comment

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

FYI, I also filed this bug report with PHP.

Cheers,
Martijn.

@kelunik
Copy link
Member

Choose a reason for hiding this comment

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

Artax also relies on URI fragments for transparent connection limiting per host when using a proxy.

@AgoraSecurity
Copy link

Choose a reason for hiding this comment

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

On what versions of PHP has this patch been applied?
I read the Changelog and I saw that in version:
7.0.18 - Fixed bug #74216 (Correctly fail on invalid IP address ports).
7.1.4 - Fixed bug #74216 (Correctly fail on invalid IP address ports).

Anyhow:
7.0.19 - Patch for bug #74216 was reverted.

It's not very clear what versions have been patched

Thanks!

@hikatu1264
Copy link

Choose a reason for hiding this comment

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

Hi,

Did this bug fix in 5.6.x?
If I want to avoid #74216, do I have to upgrade to 7.1.x?

I read 5.6.37 and I think this bug is not fixed

thanks.

@GarrettVD
Copy link

Choose a reason for hiding this comment

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

Any clarification on which versions are affected? Has CVE-2017-7189 been patched?

@brianmay
Copy link

@brianmay brianmay commented on bab0b99 Jul 30, 2019

Choose a reason for hiding this comment

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

Why are related bug reports #74216 and #74192 private and inaccessible? Any chance this could be fixed?

@cfi-gb
Copy link

@cfi-gb cfi-gb commented on bab0b99 Aug 26, 2020

Choose a reason for hiding this comment

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

I have written a mail to security@php.net asking for providing public information about the status of CVE-2017-7189.

But based on some research bab0b99 was added to PHP 7.0.18 and 7.1.4 fixing CVE-2017-7189:

https://github.com/php/php-src/blob/php-7.0.18/main/streams/xp_socket.c#L568-L612
https://github.com/php/php-src/blob/php-7.1.4/main/streams/xp_socket.c#L568-L612

but the code / changes got removed in 7.0.19 and 7.1.5 again:

https://github.com/php/php-src/blob/php-7.0.19/main/streams/xp_socket.c#L568-L605
https://github.com/php/php-src/blob/php-7.1.5/main/streams/xp_socket.c#L568-L605

and is still the same up to the most recent PHP 7.4.9 release:

https://github.com/php/php-src/blob/php-7.4.9/main/streams/xp_socket.c#L578-L615

Due to https://bugs.php.net/bug.php?id=74192 being set to private it is currently unclear if this isn't fixed since the revert or if some other code changes were made fixing this CVE in a different way.

Please sign in to comment.