php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #74216 Misbehavior of "fsockopen" may introduce a security threat
Submitted: 2017-03-07 07:36 UTC Modified: 2017-04-27 10:44 UTC
Votes:3
Avg. Score:4.3 ± 0.9
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:0 (0.0%)
From: f dot fadzil at sec-consult dot com Assigned: pollita
Status: Re-Opened Package: Sockets related
PHP Version: 7.1.2 OS: *
Private report: No CVE-ID:
View Add Comment Developer Edit
Anyone can comment on a bug. Have a simpler test case? Does it work for you on a different platform? Let us know!
Just going to say 'Me too!'? Don't clutter the database with that please — but make sure to vote on the bug!
Your email address:
MUST BE VALID
Solve the problem:
23 + 33 = ?
Subscribe to this entry?

 
 [2017-03-07 07:36 UTC] f dot fadzil at sec-consult dot com
Description:
------------
My name is Fikri Fadzil, a security consultant with SEC Consult. Based on our research, we found that the "fsockopen" function will use the port number which is defined in hostname instead of the port number passed to the second parameter of the function.

The example below will explain the misbehavior.

// This will go to port 80
fsockopen("192.168.184.132", 80, $errno, $errstr, 30);

// This will go to port 53
fsockopen("192.168.184.132:53", 80, $errno, $errstr, 30);

As many developers believe that the function will initiate the socket connection to the defined hostname (first parameter) on the specified port (second parameter), this bug will create a new attack vector for a known common web application vulnerability (e.g. Server Side Request Forgery). Due to that, a security advisory will be written and published upon the patch released.

Test script:
---------------
<?php
fsockopen("192.168.184.132:53", 80, $errno, $errstr, 30);
?>

Expected result:
----------------
A socket connection should be initiated on port 80 of 192.168.184.132.

Actual result:
--------------
// Received a request to port 53 instead of 80.
~$ sudo tcpdump -i eth0 tcp
01:34:04.989628 IP 192.168.184.1.30315 > 192.168.184.132.domain: Flags [S], seq 3305536277, win 8192, options [mss 1460,nop,wscale 8,nop,nop,sackOK], length 0
01:34:05.490734 IP 192.168.184.1.30315 > 192.168.184.132.domain: Flags [S], seq 3305536277, win 8192, options [mss 1460,nop,wscale 8,nop,nop,sackOK], length 0
01:34:05.992429 IP 192.168.184.1.30315 > 192.168.184.132.domain: Flags [S], seq 3305536277, win 8192, options [mss 1460,nop,nop,sackOK], length 0

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-03-07 19:12 UTC] pollita@php.net
-Status: Open +Status: Verified -Assigned To: +Assigned To: pollita
 [2017-03-07 19:12 UTC] pollita@php.net
This doesn't surprise me overmuch given how fsockopen() was originally designed versus how it was changed in 4.3 (or maybe it was 5.0) with the introductions of stream transports.

I agree that the result is confusing at best and potentially exploitable.  In fact, looking at the fsockopen() code, I can see that trying to use IPv6 addresses at all will simply not work (since fsockopen makes incorrect assumptions about the underlying format).

In terms of exploitability, I would offer that user-supplied input should *always* be filtered/verified before use in any context.  Not to shift the blame, but the ease with which this vector could be mitigated from userspace makes me less concerned with this as a security vulnerability.

In any case, I'll see what sort of a BC fix I can pull together.
 [2017-03-07 19:41 UTC] stas@php.net
The manual kinda mentions that port is an option:

When specifying a numerical IPv6 address (e.g. fe80::1), you must enclose the IP in square brackets—for example, tcp://[fe80::1]:80.

but very tangentially. More explicitly it is documented here: http://php.net/manual/en/transports.inet.php

Internet Domain sockets expect a port number in addition to a target address. In the case of fsockopen() this is specified in a second parameter and therefore does not impact the formatting of transport URL. With stream_socket_client() and related functions as with traditional URLs however, the port number is specified as a suffix of the transport URL delimited by a colon.

Which seems to be not matching actual fsockopen code, given this report. If not BC concerns, I'd say just drop the port handling from fsockopen, or at least make the port param override the URL param. The general rule is if you configure something implicitly and explicitly, explicit should win. So we need to figure out what to do with the BC part...
 [2017-03-07 19:54 UTC] pollita@php.net
@stas

Once tests are done, I'm planning to land this:  https://gist.github.com/sgolemon/fb1c3b9782987f7a7b10c11a6baafcd4

TL;DR - Use strtol() instead of atoi() to find out when there's "garbage" after the port number.

This shouldn't break any existing functionality apart from code that's already broken.
 [2017-03-07 20:13 UTC] pollita@php.net
-Status: Verified +Status: Closed
 [2017-03-08 16:45 UTC] f dot fadzil at sec-consult dot com
Thanks for your very fast response and fix in Github. May I know, when will be the next PHP release which will contain the fix for this issue?

FYI, we are writing a security advisory for this and will submit this issue to HackerOne IBB.
 [2017-03-08 20:47 UTC] pollita@php.net
I'm not a release manager, but since it's been applied to the 7.0 and 7.1 branches (in addition to master) it'll be included whenever 7.0.17 or 7.1.3 make it out.

If I took a completely wild and unreliable guess, I'd say probably within the next three weeks, but don't take that to the bank.
 [2017-03-09 12:36 UTC] ab@php.net
The corresponding RCs are already out, but I'd no reason to pass on QA as the bug can be triggered only on specific data. This would shift the fix a version back. I'd do so, until there were something urgent i've overseen.

Sara, seems you forgot to add a NEWS item.

Thanks.
 [2017-03-09 19:09 UTC] pollita@php.net
@ab Yeah, this isn't critical enough to force a pick onto the coming release, we can wait for the one after that.

Ugh, I always forget NEWS.  I'll push a commit for that.
 [2017-03-30 06:35 UTC] remi@php.net
Just FYI, this break Guzzle test suite
https://github.com/guzzle/guzzle/issues/1790

BTW, I think this have to be fixed in Guzzle ("127.0.0.1:8126/" includes some garbage)
 [2017-03-30 07:36 UTC] remi@php.net
PR open for discussion: https://github.com/php/php-src/pull/2443
 [2017-04-27 10:44 UTC] ab@php.net
-Status: Closed +Status: Re-Opened
 [2017-04-27 10:44 UTC] ab@php.net
The patches regarding this ticket was reopened due to issues in legacy areas. While the fix itself is correct, the abuse of undocumented functionality made an overweight impact in real life apps. Thus, re-open the ticket. See more https://externals.io/thread/831 .

Thanks.
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC