Hi Sara, > -----Original Message----- > From: p...@golemon.com [mailto:p...@golemon.com] On Behalf Of Sara > Golemon > Sent: Tuesday, April 25, 2017 7:15 PM > To: Anatol Belski <a...@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 <a...@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
