php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #74429 Remote socket URI with unique persistence identifier broken
Submitted: 2017-04-13 09:26 UTC Modified: 2017-05-24 13:16 UTC
Votes:2
Avg. Score:5.0 ± 0.0
Reproduced:2 of 2 (100.0%)
Same Version:1 (50.0%)
Same OS:1 (50.0%)
From: martijn dot grendelman at isaac dot nl Assigned: pollita
Status: Closed Package: Streams related
PHP Version: 7.0.18RC1 OS: Any
Private report: No CVE-ID:
 [2017-04-13 09:26 UTC] martijn dot grendelman at isaac dot nl
Description:
------------
Commit https://github.com/php/php-src/commit/bab0b99f376dac9170ac81382a5ed526938d595a changes the way remote socket specifiers are parsed for a stream socket.

URIs like 'tcp://localhost:80/uniqueid' now result in an error. That they used to work seems like an undocumented feature, but I know it to be used in at least one open source project: https://github.com/colinmollenhour/credis.

Was this change intended to break this behaviour?

Best regards,
Martijn Grendelman




Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-04-13 09:28 UTC] krakjoe@php.net
-Assigned To: +Assigned To: pollita
 [2017-04-13 16:22 UTC] ab@php.net
@martijn dot grendelman at isaac dot nl, thanks for reporting. Ref to bug #74216 for details. While it's a low security impact in fsockopen(), same piece is used for parsing in stream_socket_client().

Indeed, the "feature" is not documented so was not guaranteed to work. The way it is exploited is misleading, as tcp:// has no optional parameters. Nevertheless, probably we should indeed fix this case, especially the use case looks falling under the current scheme and the functionality itself makes sense. Fe one can say, a new optional parameter is introduced to tcp:// sheme, so then something like tcp://ip:port/id=bla would be a valid use. Or otherwise, for BC, anything after "/" would be read as an id. I'd suggest this

- don't affect the current fix to fsockopen
- add optional parameter to the tcp:// scheme in all branches, say id=bla
- otherwise treat anything before "/" as a unique id, if failed to parse

The last one - only for stable branches as a BC measure, master should be clean failing if the optional parameter couldn't be parsed, call it "id" or whatever. Sara, what do you think?

@martijn i also can't avoid mentioning, that this patch is around for more than a month and is also present in the RCs. So for one, it is sad to see pure QA participation quote, and it also probably shows the use case makes no wide impact.

Thanks.
 [2017-04-13 16:27 UTC] ab@php.net
Typo,

- otherwise treat anything AFTER "/" as a unique id, if failed to parse

Thanks.
 [2017-04-14 06:49 UTC] martijn dot grendelman at isaac dot nl
Hi,

I'm just a user, and I normally don't run RCs. It takes time for PHP releases to make their way to the distributions and to users' computers, so I only noticed yesterday when upgrading one of our servers to PHP 7.0.18.

Cheers,
Martijn.
 [2017-04-18 00:48 UTC] dominic at varspool dot com
Here's another use case (one that I introduced, so I'm feeling guilty about): https://github.com/nrk/predis/pull/139

The problem being that Redis has the concept of multiple databases per server (so, the host, port etc. will match). So, if you want to use a persistent connections, you end up with cross-writes between the databases. (More worked example at that link.)
 [2017-04-18 00:51 UTC] dominic at varspool dot com
This user-contributed note should be amended/removed following an outcome: http://php.net/manual/en/function.stream-socket-client.php#105393
 [2017-04-18 21:16 UTC] pollita@php.net
No. That "undocumented feature" was never intended to work, and the use of it is an abuse of the API.

As to allowing multiple persistent connections to a backend resource, I think we could add official support for it via a new API, and perhaps even matching this use case, but it's going to take some discussion because I don't want to trivially walk into codifying something for no better reason that "It worked on accident at some point."
 [2017-04-18 21:40 UTC] pollita@php.net
For what it's worth, https://gist.github.com/sgolemon/51f329655a94523546f081bf9c9afd31 is probably safe enough to use as it would still cover the security case given by bug #74216, it would allow colinmollenhour's hack to keep working, and it's fairly "URI-ish", so it's not an unreasonable looking thing.

I'm not personally convinced though.
 [2017-04-25 12:11 UTC] ab@php.net
-Status: Assigned +Status: Closed
 [2017-04-25 12:11 UTC] ab@php.net
The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.


 [2017-04-28 09:43 UTC] dominic at varspool dot com
Thanks for the patch ab! Further discussion of the future of this undocumented hack (as it has rightly been called), and any possible replacement (maybe a stream context option?) will, I presume, take place on the mailing list?

I'd recommend nuking the user note at http://php.net/manual/en/function.stream-socket-client.php#105393 to limit the spread of the problem, in any case, though.
 [2017-04-28 17:50 UTC] pollita@php.net
I need to sit down later and collate all the myriad places bad transport strings get created (both intentionally and unintentionally) and I'll start a thread on internals to that effect.  I'll be happy to cc you at the time to make sure you see it. :D
 [2017-05-23 17:11 UTC] boen dot robot at gmail dot com
Hello.

I'm the user who posted the user doc note (after accidently finding it when developing https://github.com/pear2/Net_Transmitter)

So... This is now "fixed" how? Is the after slash ID once again a thing for now, until a documented replacement is in place?

Abyway, as far as a replacement goes, I'd personally be perfectly fine with tcp://ip:port/id=bla AND breaking BC (since this was an undocumented feature/hack...).

Libraries like credis, predis and Net_Transmitter can easily be amended to prefix "id=" before the actual ID, which would work in all past versions where this undocumented feature/hack was working AND will work with that option introduced.

And BTW, if the PHP site allowed users to amend their user contributed doc notes, I totally would. Meanwhile, I hope you only nuke it after this feature gets properly introduced OR amend it to include a link to this bug report.
 [2017-05-24 13:16 UTC] pollita@php.net
> So... This is now "fixed" how?

At the moment, we're reverted back to before bab0b99f so things are "fixed" in the sense that the status quo is maintained.  The named persistent sockets is a justifiable (if abusive) use of the socket streams API, so we're going to have to come up with a "right way" to do it.

I'm doubtful about being able to get that "right way" into 7.2 at this point (with feature freeze so close) so it'll probably end up in 7.3 which means we can't really remove the trailing identifiers support until some time after that.

BC is such a joy.
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sun Aug 20 17:01:35 2017 UTC