Everything

php

On malformed transport strings

php.internals

Sara Golemon

9 years ago

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

Anatoliy Belsky

9 years ago

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

Anatoliy Belsky

9 years ago

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

Sara Golemon

9 years ago

On Tue, Apr 25, 2017 at 5:15 AM, Anatol Belski <ab@php.net> wrote:

> 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

Anatoliy Belsky

9 years ago

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
>
> On Tue, Apr 25, 2017 at 5:15 AM, Anatol Belski <ab@php.net> wrote:
> > 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

Sara Golemon

9 years ago

On Wed, Apr 26, 2017 at 6:20 AM, Anatol Belski <ab@php.net> wrote:

> 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

Anatoliy Belsky

9 years ago

> -----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
>
> On Wed, Apr 26, 2017 at 6:20 AM, Anatol Belski <ab@php.net> wrote:
> > 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

Sara Golemon

9 years ago

On Wed, Apr 26, 2017 at 1:19 PM, Anatol Belski <ab@php.net> wrote:

> 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

Anatoliy Belsky

9 years ago

> -----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
>
> On Wed, Apr 26, 2017 at 1:19 PM, Anatol Belski <ab@php.net> wrote:
> > 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