On malformed transport strings

8 years ago by Sara Golemon — view source — reply

unread

My fix to https://bugs.php.net/bug.php?id=74216 tightened down the
definition of what a valid transport string looks like.

Previously, transport strings like
"tcp://127.0.0.1:80:81:82/your/moms/face" would be accepted by PHP as
perfectly valid URIs. Since this was never documented as a feature of
transports, but rather a simple oversight during the Bush era, I added
some sanity checking to verify there was no garbage after the port
number in IP based transport strings.

Then they came out of the woodwork...

https://bugs.php.net/bug.php?id=74429 depends on the previous "ignore
garbage" behavior in order to create named persistent streams. (ex:
"tcp://127.0.0.1:80/main_connection" and
"tcp://127.0.0.1:80/secondary") In short, the persistence key is
defined by the whole transport string, while the address/port parsing
ensures that the uniqueifying portion doesn't prevent the connection
from happening. The project referenced in that bug report and predis
both utilize this behavior.

https://bugs.php.net/bug.php?id=74432 also popped up today as a bug in
how mysqli (specifically via mysqlnd) invokes connections to the
backend. Ultimately, an address provided blindly gets a (potentially
default) port number tacked onto the end. Again, undocumented
behavior working up until the 74216 fix.

I'm inclined to revert to prior "ignore garbage" behavior on the 7.0
and 7.1 branches to avoid BC break trauma (though I do think raising a
warning is advised). What's uncertain in my mind is whether or not we
take a hard line on "Use the API as documented" for 7.2 or if some
other middle ground is appropriate. Particularly given the use case
of named persistent transports. The right way to do that would be to
have a new API for named transports, possibly just as a context
option.

-Sara

https://pbs.twimg.com/media/C9uYe-uUQAAPpbK.jpg

8 years ago by Anatol Belski — view source — reply

unread

Hi Sara,

-----Original Message-----
From: php@golemon.com [mailto:php@golemon.com] On Behalf Of Sara
Golemon
Sent: Thursday, April 20, 2017 10:56 PM
To: PHP internals internals@lists.php.net
Subject: [PHP-DEV] On malformed transport strings

My fix to https://bugs.php.net/bug.php?id=74216 tightened down the definition
of what a valid transport string looks like.

Previously, transport strings like
"tcp://127.0.0.1:80:81:82/your/moms/face" would be accepted by PHP as
perfectly valid URIs. Since this was never documented as a feature of
transports, but rather a simple oversight during the Bush era, I added some
sanity checking to verify there was no garbage after the port number in IP based
transport strings.

Then they came out of the woodwork...

https://bugs.php.net/bug.php?id=74429 depends on the previous "ignore
garbage" behavior in order to create named persistent streams. (ex:
"tcp://127.0.0.1:80/main_connection" and
"tcp://127.0.0.1:80/secondary") In short, the persistence key is defined by the
whole transport string, while the address/port parsing ensures that the
uniqueifying portion doesn't prevent the connection from happening. The
project referenced in that bug report and predis both utilize this behavior.

Thanks for getting onto this. Leaving aside, that the functionality is undocumented, in general it appears to be useful in some situations. Probably some would ask for that anyway, if it were not provided due to the implementation bug ?

https://bugs.php.net/bug.php?id=74432 also popped up today as a bug in how
mysqli (specifically via mysqlnd) invokes connections to the backend. Ultimately,
an address provided blindly gets a (potentially
default) port number tacked onto the end. Again, undocumented behavior
working up until the 74216 fix.

I'm inclined to revert to prior "ignore garbage" behavior on the 7.0 and 7.1
branches to avoid BC break trauma (though I do think raising a warning is
advised). What's uncertain in my mind is whether or not we take a hard line on
"Use the API as documented" for 7.2 or if some other middle ground is
appropriate. Particularly given the use case of named persistent transports. The
right way to do that would be to have a new API for named transports, possibly
just as a context option.

I'd be suggesting this as well. Either we could make this part only backward compatible, as suggested in your follow up patch, or one could check whether a solution were possible to fix the initial fsockopen() issue without affecting the ip parsing parts globally. Depends probably on how sensible the work amount would be. Please be aware, that an action should be taken next days before Tuesday, so then we can ask for tests on the RCs. And otherwise, an official way closing the actual undocumented gap could be suggested for 7.2.

Thanks

Anatol

8 years ago by Anatol Belski — view source — reply

unread

Hi Sara,

-----Original Message-----
From: Anatol Belski [mailto:weltling@outlook.de] On Behalf Of Anatol Belski
Sent: Saturday, April 22, 2017 12:41 PM
To: Sara Golemon pollita@php.net; PHP internals internals@lists.php.net
Subject: RE: [PHP-DEV] On malformed transport strings

I'm inclined to revert to prior "ignore garbage" behavior on the 7.0
and 7.1 branches to avoid BC break trauma (though I do think raising a
warning is advised). What's uncertain in my mind is whether or not we
take a hard line on "Use the API as documented" for 7.2 or if some
other middle ground is appropriate. Particularly given the use case
of named persistent transports. The right way to do that would be to
have a new API for named transports, possibly just as a context option.

I'd be suggesting this as well. Either we could make this part only backward
compatible, as suggested in your follow up patch, or one could check whether a
solution were possible to fix the initial fsockopen() issue without affecting the ip
parsing parts globally. Depends probably on how sensible the work amount
would be. Please be aware, that an action should be taken next days before
Tuesday, so then we can ask for tests on the RCs. And otherwise, an official way
closing the actual undocumented gap could be suggested for 7.2.

I've applied the patch you've suggested in bug #74429, so it's going to be included in RCs. Given the initial security issue is not impacted, BC can be kept.

Thanks

Anatol

8 years ago by Sara Golemon — view source — reply

unread

I've applied the patch you've suggested in bug #74429, so it's going to be included in RCs. Given the initial security issue is not impacted, BC can be kept.

I thought about the security implications of that quick fix and while
it doesn't impact the specifics of the bug that led to the tightening,
a very slightly modified version would still work, e.g.:

$userSuppliedAddress = '1.2.3.4:8000/'
$fp = fsockopen($userSuppliedAddress, 80); // Will connect to port
8000, not the hard-coded 80.

So I'm not actually keen on that as a "fix" as it still leaves the
vulnerability of overloading address and causes things like
mysqli_connect() to break when provided with a port in the address.
Double-whammy bad.

-Sara

8 years ago by Anatol Belski — view source — reply

unread

Hi Sara,

-----Original Message-----
From: php@golemon.com [mailto:php@golemon.com] On Behalf Of Sara
Golemon
Sent: Tuesday, April 25, 2017 7:15 PM
To: Anatol Belski ab@php.net
Cc: PHP internals internals@lists.php.net
Subject: Re: [PHP-DEV] On malformed transport strings

I've applied the patch you've suggested in bug #74429, so it's going to be
included in RCs. Given the initial security issue is not impacted, BC can be kept.

I thought about the security implications of that quick fix and while it doesn't
impact the specifics of the bug that led to the tightening, a very slightly modified
version would still work, e.g.:

$userSuppliedAddress = '1.2.3.4:8000/'
$fp = fsockopen($userSuppliedAddress, 80); // Will connect to port 8000, not
the hard-coded 80.

So I'm not actually keen on that as a "fix" as it still leaves the vulnerability of
overloading address and causes things like
mysqli_connect() to break when provided with a port in the address.
Double-whammy bad.

Thanks for this additional check. My action was actually based on the comment with the patch link, looks like the situation has now changed a bit. We're still quite limited in choice in this case. For one, there's a low security impact, however the fix uncovered several inconsistent places breaching apps. For what it matters, there are already 2-3 dups regarding mysqli and stream client regressions. Given they come so short in time, that's not a good sign. Though, the reports still came late enough, that an appropriate fix could not be done before the next RC.

At the start, there was only a fail with a broken test, but now we see that it's a real impact. The use of the undocumented functionality is clearly an abuse, but it was also not tested in any of our test suite cases to ensure it doesn't work. One can guess, there might be beyond places of same. There's a security impact, even low, so we cannot simply revert to the old behavior, and there are inconsistencies which was discovered too late in the pipe. Not a satisfactory situation at any end. Clearly we could stand pat by ignoring the regressions, as the behavior is undocumented, however it seems not sensible, as for my terms at least.

In the end, after evaluating the situation, I would still suggest to keep your follow up fix as a temporary solution in the next release. This way at least one issue is fixed, the stream client, while the initial patch is a bit slackened. A better fix can be worked out till the follow up release, also targeting the mysqli regression which still persists. This way, one regression is fixed, the initial patch is weakened a bit but as the impact was low - it's something one can temporarily live with, and a good solution were to expect in the next possible future. An alternative were to revert the hotfix in the final and keep the regressions.

Regards

Anatol

8 years ago by Sara Golemon — view source — reply

unread

Thanks for this additional check. My action was actually based on the comment with the patch link, looks like the situation has now changed a bit. We're still quite limited in choice in this case. For one, there's a low security impact, however the fix uncovered several inconsistent places breaching apps. For what it matters, there are already 2-3 dups regarding mysqli and stream client regressions. Given they come so short in time, that's not a good sign. Though, the reports still came late enough, that an appropriate fix could not be done before the next RC.

The fact that there are dups tells me that, despite the fact that
bab0b99f3 made into 7.0.18/7.1.4 releases, we should fully revert the
hard error (leaving a soft warning behind). The security implications
of the original fix are fairly minor* compared to the much larger
regression of actually breaking sites which otherwise worked before.

In the end, after evaluating the situation, I would still suggest to keep your follow up fix as a temporary solution in the next release. This way at least one issue is fixed, the stream client, while the initial patch is a bit slackened. A better fix can be worked out till the follow up release, also targeting the mysqli regression which still persists. This way, one regression is fixed, the initial patch is weakened a bit but as the impact was low - it's something one can temporarily live with, and a good solution were to expect in the next possible future. An alternative were to revert the hotfix in the final and keep the regressions.

Given that there is a release with bab0b99f3 in it, I suppose we're
already regressed and a little clowny looking. 7.0 is your branch, so
if you're cool with some uses still being slightly borky, then so am
I. I'll do up some diffs for 7.0.20/7.1.6 to downgrade the hard
errors to warnings (keep it hard error for 7.2.0) and address issues
like the mysqli_connect implicit port duplication.

-Sara

8 years ago by Anatol Belski — view source — reply

unread

-----Original Message-----
From: php@golemon.com [mailto:php@golemon.com] On Behalf Of Sara
Golemon
Sent: Wednesday, April 26, 2017 5:35 PM
To: Anatol Belski ab@php.net
Cc: PHP internals internals@lists.php.net; Joe Watkins krakjoe@php.net;
Davey Shafik davey@php.net; Remi Collet remi@php.net
Subject: Re: [PHP-DEV] On malformed transport strings

Thanks for this additional check. My action was actually based on the
comment with the patch link, looks like the situation has now changed a bit.
We're still quite limited in choice in this case. For one, there's a low security
impact, however the fix uncovered several inconsistent places breaching apps.
For what it matters, there are already 2-3 dups regarding mysqli and stream
client regressions. Given they come so short in time, that's not a good sign.
Though, the reports still came late enough, that an appropriate fix could not be
done before the next RC.

The fact that there are dups tells me that, despite the fact that
bab0b99f3 made into 7.0.18/7.1.4 releases, we should fully revert the hard error
(leaving a soft warning behind). The security implications of the original fix are
fairly minor* compared to the much larger regression of actually breaking sites
which otherwise worked before.

In the end, after evaluating the situation, I would still suggest to keep your
follow up fix as a temporary solution in the next release. This way at least one
issue is fixed, the stream client, while the initial patch is a bit slackened. A better
fix can be worked out till the follow up release, also targeting the mysqli
regression which still persists. This way, one regression is fixed, the initial patch
is weakened a bit but as the impact was low - it's something one can temporarily
live with, and a good solution were to expect in the next possible future. An
alternative were to revert the hotfix in the final and keep the regressions.

Given that there is a release with bab0b99f3 in it, I suppose we're already
regressed and a little clowny looking. 7.0 is your branch, so if you're cool with
some uses still being slightly borky, then so am I. I'll do up some diffs for
7.0.20/7.1.6 to downgrade the hard errors to warnings (keep it hard error for
7.2.0) and address issues like the mysqli_connect implicit port duplication.

Well, there is indeed a breakage in the legacy area, and there is still a security context. The former requires quite some care, and the latter should not be ignored. With cda7dcf4 one regression is solved, even if temporarily. From what I see now, the mysqli part is even more critical, fe the connection strings from the WP docs https://codex.wordpress.org/Editing_wp-config.php#Set_Database_Host . Whereby I can tell, that many major projects handle this properly, fe https://github.com/s9y/Serendipity/blob/master/include/db/mysqli.inc.php#L224 .

Maybe when you've some patch anytime soon, let's test it and then consider whether we can include it into final? Or, at least some minimal specific approach to fix the mysqli part, and care about the general solution for the subsequent release. What I'd basically avoid is making changes in stress, as there might be other beyond places and we shouldn't risk to introduce more breach than there already is. Instead, that requires a cold head and a lot of QA ?

Regards

Anatol

8 years ago by Sara Golemon — view source — reply

unread

What I'd basically avoid is making changes in stress,
as there might be other beyond places and we shouldn't
risk to introduce more breach than there already is.
Instead, that requires a cold head and a lot of QA ?

Which is precisely why I'm advocating reverting the whole lot. I've
just sat down to try to at least address the mysqli_connect part and
it's hairy. Basically we've built in precisely the kind of bad
assumption that I was initially grousing about frameworks having done.

I don't mean to ignore the security issue presented by 74216, I just
recognize that my initial fix was made hastily and we should allocate
more time to fix it properly (with all that lovely QA and testing).

-Sara

8 years ago by Anatol Belski — view source — reply

unread

-----Original Message-----
From: php@golemon.com [mailto:php@golemon.com] On Behalf Of Sara
Golemon
Sent: Thursday, April 27, 2017 12:10 AM
To: Anatol Belski ab@php.net
Cc: PHP internals internals@lists.php.net; Joe Watkins krakjoe@php.net;
Davey Shafik davey@php.net; Remi Collet remi@php.net
Subject: Re: [PHP-DEV] On malformed transport strings

What I'd basically avoid is making changes in stress, as there might
be other beyond places and we shouldn't risk to introduce more breach
than there already is.
Instead, that requires a cold head and a lot of QA ?

Which is precisely why I'm advocating reverting the whole lot. I've just sat down
to try to at least address the mysqli_connect part and it's hairy. Basically we've
built in precisely the kind of bad assumption that I was initially grousing about
frameworks having done.

I don't mean to ignore the security issue presented by 74216, I just recognize
that my initial fix was made hastily and we should allocate more time to fix it
properly (with all that lovely QA and testing).

For what I could tell however, your fix was not made hastily. It was pushed 1.5 months before 7.0.18 and the impact is only discovered in the final by the apps using undocumented functionality. That was my reason trying to keep the actual patch. The fact the undocumented functionality seems to be indeed in (ab)use is the explanation, why it took that long to discover. I think we're clear now, that the BC impact in this case is overweight.

ACK, I will go by reverting these revs from the dev and release branch, please correct me if I miss some

cda7dcf4cacef3346f9dc2a4dc947e6a74769259
bab0b99f376dac9170ac81382a5ed526938d595a

reopen bug #74216 and retag the RC. Joe, Davey, seems you should retag as well. Thanks for this productive discussion, Sara.

Regards

Anatol