Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] On malformed transport strings

Posted by Sara Golemon 
Sara Golemon
[PHP-DEV] On malformed transport strings
April 20, 2017 11:00PM
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Anatol Belski
RE: [PHP-DEV] On malformed transport strings
April 22, 2017 12:50PM
Hi Sara,

> -----Original Message-----
> From: php@golemon.com [mailto:[email protected]] On Behalf Of Sara
> Golemon
> Sent: Thursday, April 20, 2017 10:56 PM
> To: PHP internals <[email protected]>
> 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
Anatol Belski
RE: [PHP-DEV] On malformed transport strings
April 25, 2017 02:20PM
Hi Sara,

> -----Original Message-----
> From: Anatol Belski [mailto:[email protected]] On Behalf Of Anatol Belski
> Sent: Saturday, April 22, 2017 12:41 PM
> To: Sara Golemon <[email protected]>; PHP internals <[email protected]>
> 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
Re: [PHP-DEV] On malformed transport strings
April 25, 2017 07:20PM
On Tue, Apr 25, 2017 at 5:15 AM, Anatol Belski <[email protected]> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Anatol Belski
RE: [PHP-DEV] On malformed transport strings
April 26, 2017 01:30PM
Hi Sara,

> -----Original Message-----
> From: php@golemon.com [mailto:[email protected]] On Behalf Of Sara
> Golemon
> Sent: Tuesday, April 25, 2017 7:15 PM
> To: Anatol Belski <[email protected]>
> Cc: PHP internals <[email protected]>
> Subject: Re: [PHP-DEV] On malformed transport strings
>
> On Tue, Apr 25, 2017 at 5:15 AM, Anatol Belski <[email protected]> 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
Re: [PHP-DEV] On malformed transport strings
April 26, 2017 05:40PM
On Wed, Apr 26, 2017 at 6:20 AM, Anatol Belski <[email protected]> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Anatol Belski
RE: [PHP-DEV] On malformed transport strings
April 26, 2017 08:30PM
> -----Original Message-----
> From: php@golemon.com [mailto:[email protected]] On Behalf Of Sara
> Golemon
> Sent: Wednesday, April 26, 2017 5:35 PM
> To: Anatol Belski <[email protected]>
> Cc: PHP internals <[email protected]>; Joe Watkins <[email protected]>;
> Davey Shafik <[email protected]>; Remi Collet <[email protected]>
> Subject: Re: [PHP-DEV] On malformed transport strings
>
> On Wed, Apr 26, 2017 at 6:20 AM, Anatol Belski <[email protected]> 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
Sara Golemon
Re: [PHP-DEV] On malformed transport strings
April 27, 2017 12:20AM
On Wed, Apr 26, 2017 at 1:19 PM, Anatol Belski <[email protected]> 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
Anatol Belski
RE: [PHP-DEV] On malformed transport strings
April 27, 2017 03:10AM
> -----Original Message-----
> From: php@golemon.com [mailto:[email protected]] On Behalf Of Sara
> Golemon
> Sent: Thursday, April 27, 2017 12:10 AM
> To: Anatol Belski <[email protected]>
> Cc: PHP internals <[email protected]>; Joe Watkins <[email protected]>;
> Davey Shafik <[email protected]>; Remi Collet <[email protected]>
> Subject: Re: [PHP-DEV] On malformed transport strings
>
> On Wed, Apr 26, 2017 at 1:19 PM, Anatol Belski <[email protected]> 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
Sorry, only registered users may post in this forum.

Click here to login