From 4dd2cde7e5aabaa45c10dedfbb6ed15706110dfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Sun, 17 Jun 2018 12:53:41 +0200 Subject: [PATCH 1/2] Add DatagramTransportExecutor --- README.md | 46 ++++- examples/04-query-a-and-aaaa.php | 7 +- src/Query/DatagramTransportExecutor.php | 155 ++++++++++++++ src/Query/Executor.php | 4 + src/Resolver/Factory.php | 6 +- tests/Query/DatagramTransportExecutorTest.php | 189 ++++++++++++++++++ 6 files changed, 394 insertions(+), 13 deletions(-) create mode 100644 src/Query/DatagramTransportExecutor.php create mode 100644 tests/Query/DatagramTransportExecutorTest.php diff --git a/README.md b/README.md index 5ee7a510..0e12f3de 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,7 @@ easily be used to create a DNS server. * [Caching](#caching) * [Custom cache adapter](#custom-cache-adapter) * [Advanced usage](#advanced-usage) + * [DatagramTransportExecutor](#datagramtransportexecutor) * [HostsFileExecutor](#hostsfileexecutor) * [Install](#install) * [Tests](#tests) @@ -117,13 +118,20 @@ See also the wiki for possible [cache implementations](https://github.com/reactp ## Advanced Usage -For more advanced usages one can utilize the `React\Dns\Query\Executor` directly. +### DatagramTransportExecutor + +The `DatagramTransportExecutor` can be used to +send DNS queries over a datagram transport such as UDP. + +This is the main class that sends a DNS query to your DNS server and is used +internally by the `Resolver` for the actual message transport. + +For more advanced usages one can utilize this class directly. The following example looks up the `IPv6` address for `igor.io`. ```php $loop = Factory::create(); - -$executor = new Executor($loop, new Parser(), new BinaryDumper(), null); +$executor = new DatagramTransportExecutor($loop); $executor->query( '8.8.8.8:53', @@ -135,11 +143,41 @@ $executor->query( }, 'printf'); $loop->run(); - ``` See also the [fourth example](examples). +Note that this executor does not implement a timeout, so you will very likely +want to use this in combination with a `TimeoutExecutor` like this: + +```php +$executor = new TimeoutExecutor( + new DatagramTransportExecutor($loop), + 3.0, + $loop +); +``` + +Also note that this executor uses an unreliable UDP transport and that it +does not implement any retry logic, so you will likely want to use this in +combination with a `RetryExecutor` like this: + +```php +$executor = new RetryExecutor( + new TimeoutExecutor( + new DatagramTransportExecutor($loop), + 3.0, + $loop + ) +); +``` + +> Internally, this class uses PHP's UDP sockets and does not take advantage + of [react/datagram](https://github.com/reactphp/datagram) purely for + organizational reasons to avoid a cyclic dependency between the two + packages. Higher-level components should take advantage of the Datagram + component instead of reimplementing this socket logic from scratch. + ### HostsFileExecutor Note that the above `Executor` class always performs an actual DNS query. diff --git a/examples/04-query-a-and-aaaa.php b/examples/04-query-a-and-aaaa.php index c4f52fcf..b5320e54 100644 --- a/examples/04-query-a-and-aaaa.php +++ b/examples/04-query-a-and-aaaa.php @@ -1,17 +1,14 @@ query( + * '8.8.8.8:53', + * new Query($name, Message::TYPE_AAAA, Message::CLASS_IN, time()) + * )->then(function (Message $message) { + * foreach ($message->answers as $answer) { + * echo 'IPv6: ' . $answer->data . PHP_EOL; + * } + * }, 'printf'); + * + * $loop->run(); + * ``` + * + * See also the [fourth example](examples). + * + * Note that this executor does not implement a timeout, so you will very likely + * want to use this in combination with a `TimeoutExecutor` like this: + * + * ```php + * $executor = new TimeoutExecutor( + * new DatagramTransportExecutor($loop), + * 3.0, + * $loop + * ); + * ``` + * + * Also note that this executor uses an unreliable UDP transport and that it + * does not implement any retry logic, so you will likely want to use this in + * combination with a `RetryExecutor` like this: + * + * ```php + * $executor = new RetryExecutor( + * new TimeoutExecutor( + * new DatagramTransportExecutor($loop), + * 3.0, + * $loop + * ) + * ); + * ``` + * + * > Internally, this class uses PHP's UDP sockets and does not take advantage + * of [react/datagram](https://github.com/reactphp/datagram) purely for + * organizational reasons to avoid a cyclic dependency between the two + * packages. Higher-level components should take advantage of the Datagram + * component instead of reimplementing this socket logic from scratch. + */ +class DatagramTransportExecutor implements ExecutorInterface +{ + private $loop; + private $parser; + private $dumper; + + /** + * @param LoopInterface $loop + * @param null|Parser $parser optional/advanced: DNS protocol parser to use + * @param null|BinaryDumper $dumper optional/advanced: DNS protocol dumper to use + */ + public function __construct(LoopInterface $loop, Parser $parser = null, BinaryDumper $dumper = null) + { + if ($parser === null) { + $parser = new Parser(); + } + if ($dumper === null) { + $dumper = new BinaryDumper(); + } + + $this->loop = $loop; + $this->parser = $parser; + $this->dumper = $dumper; + } + + public function query($nameserver, Query $query) + { + $request = Message::createRequestForQuery($query); + + $queryData = $this->dumper->toBinary($request); + if (isset($queryData[512])) { + return \React\Promise\reject(new \RuntimeException( + 'DNS query for ' . $query->name . ' failed: Query too large for UDP transport' + )); + } + + // UDP connections are instant, so try connection without a loop or timeout + $socket = @\stream_socket_client("udp://$nameserver", $errno, $errstr, 0); + if ($socket === false) { + return \React\Promise\reject(new \RuntimeException( + 'DNS query for ' . $query->name . ' failed: Unable to connect to DNS server (' . $errstr . ')', + $errno + )); + } + + // set socket to non-blocking and immediately try to send (fill write buffer) + \stream_set_blocking($socket, false); + \fwrite($socket, $queryData); + + $loop = $this->loop; + $deferred = new Deferred(function () use ($loop, $socket, $query) { + // cancellation should remove socket from loop and close socket + $loop->removeReadStream($socket); + \fclose($socket); + + throw new CancellationException('DNS query for ' . $query->name . ' has been cancelled'); + }); + + $parser = $this->parser; + $loop->addReadStream($socket, function ($socket) use ($loop, $deferred, $query, $parser) { + // try to read a single data packet from the DNS server + // ignoring any errors, this is uses UDP packets and not a stream of data + $data = @\fread($socket, 512); + + // we only react to the first message, so immediately remove socket from loop and close + $loop->removeReadStream($socket); + \fclose($socket); + + try { + $response = $parser->parseMessage($data); + } catch (\Exception $e) { + // reject if we received an invalid message from remote server + $deferred->reject($e); + return; + } + + if ($response->header->isTruncated()) { + $deferred->reject(new \RuntimeException('DNS query for ' . $query->name . ' failed: The server returned a truncated result for a UDP query, but retrying via TCP is currently not supported')); + return; + } + + $deferred->resolve($response); + }); + + return $deferred->promise(); + } +} diff --git a/src/Query/Executor.php b/src/Query/Executor.php index 4c51f2b1..3020492e 100644 --- a/src/Query/Executor.php +++ b/src/Query/Executor.php @@ -11,6 +11,10 @@ use React\Stream\DuplexResourceStream; use React\Stream\Stream; +/** + * @deprecated unused, exists for BC only + * @see DatagramTransportExecutor + */ class Executor implements ExecutorInterface { private $loop; diff --git a/src/Resolver/Factory.php b/src/Resolver/Factory.php index 12a912f0..ebae6c60 100644 --- a/src/Resolver/Factory.php +++ b/src/Resolver/Factory.php @@ -5,10 +5,8 @@ use React\Cache\ArrayCache; use React\Cache\CacheInterface; use React\Dns\Config\HostsFile; -use React\Dns\Protocol\Parser; -use React\Dns\Protocol\BinaryDumper; use React\Dns\Query\CachedExecutor; -use React\Dns\Query\Executor; +use React\Dns\Query\DatagramTransportExecutor; use React\Dns\Query\ExecutorInterface; use React\Dns\Query\HostsFileExecutor; use React\Dns\Query\RecordCache; @@ -71,7 +69,7 @@ private function decorateHostsFileExecutor(ExecutorInterface $executor) protected function createExecutor(LoopInterface $loop) { return new TimeoutExecutor( - new Executor($loop, new Parser(), new BinaryDumper(), null), + new DatagramTransportExecutor($loop), 5.0, $loop ); diff --git a/tests/Query/DatagramTransportExecutorTest.php b/tests/Query/DatagramTransportExecutorTest.php new file mode 100644 index 00000000..982b0ef7 --- /dev/null +++ b/tests/Query/DatagramTransportExecutorTest.php @@ -0,0 +1,189 @@ +getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + $loop->expects($this->never())->method('addReadStream'); + + $dumper = $this->getMockBuilder('React\Dns\Protocol\BinaryDumper')->getMock(); + $dumper->expects($this->once())->method('toBinary')->willReturn(str_repeat('.', 513)); + + $executor = new DatagramTransportExecutor($loop, null, $dumper); + + $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + $promise = $executor->query('8.8.8.8:53', $query); + + $this->assertInstanceOf('React\Promise\PromiseInterface', $promise); + $promise->then(null, $this->expectCallableOnce()); + } + + public function testQueryRejectsIfServerConnectionFails() + { + $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + $loop->expects($this->never())->method('addReadStream'); + + $executor = new DatagramTransportExecutor($loop); + + $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + $promise = $executor->query('///', $query); + + $this->assertInstanceOf('React\Promise\PromiseInterface', $promise); + $promise->then(null, $this->expectCallableOnce()); + } + + /** + * @group internet + */ + public function testQueryRejectsOnCancellation() + { + $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + $loop->expects($this->once())->method('addReadStream'); + $loop->expects($this->once())->method('removeReadStream'); + + $executor = new DatagramTransportExecutor($loop); + + $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + $promise = $executor->query('8.8.8.8:53', $query); + $promise->cancel(); + + $this->assertInstanceOf('React\Promise\PromiseInterface', $promise); + $promise->then(null, $this->expectCallableOnce()); + } + + public function testQueryRejectsIfServerRejectsNetworkPacket() + { + $loop = Factory::create(); + + $executor = new DatagramTransportExecutor($loop); + + $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + + $wait = true; + $promise = $executor->query('127.0.0.1:1', $query)->then( + null, + function ($e) use (&$wait) { + $wait = false; + throw $e; + } + ); + + // run loop for short period to ensure we detect connection ICMP rejection error + \Clue\React\Block\sleep(0.01, $loop); + if ($wait) { + \Clue\React\Block\sleep(0.2, $loop); + if ($wait) { + $this->markTestSkipped('Did not receive an error (your OS may drop UDP packets to unbound port?)'); + } + } + + $this->assertFalse($wait); + } + + public function testQueryRejectsIfServerSendInvalidMessage() + { + $loop = Factory::create(); + + $server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND); + $loop->addReadStream($server, function ($server) { + $data = stream_socket_recvfrom($server, 512, 0, $peer); + stream_socket_sendto($server, 'invalid', 0, $peer); + }); + + $address = stream_socket_get_name($server, false); + $executor = new DatagramTransportExecutor($loop); + + $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + + $wait = true; + $promise = $executor->query($address, $query)->then( + null, + function ($e) use (&$wait) { + $wait = false; + throw $e; + } + ); + + // run loop for short period to ensure we detect connection ICMP rejection error + \Clue\React\Block\sleep(0.01, $loop); + if ($wait) { + \Clue\React\Block\sleep(0.2, $loop); + } + + $this->assertFalse($wait); + } + + public function testQueryRejectsIfServerSendsTruncatedResponse() + { + $response = new Message(); + $response->header->set('tc', 1); + + $parser = $this->getMockBuilder('React\Dns\Protocol\Parser')->getMock(); + $parser->expects($this->once())->method('parseMessage')->with('data')->willReturn($response); + + $loop = Factory::create(); + + $server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND); + $loop->addReadStream($server, function ($server) { + $data = stream_socket_recvfrom($server, 512, 0, $peer); + stream_socket_sendto($server, 'data', 0, $peer); + }); + + $address = stream_socket_get_name($server, false); + $executor = new DatagramTransportExecutor($loop, $parser); + + $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + + $wait = true; + $promise = $executor->query($address, $query)->then( + null, + function ($e) use (&$wait) { + $wait = false; + throw $e; + } + ); + + // run loop for short period to ensure we detect connection ICMP rejection error + \Clue\React\Block\sleep(0.01, $loop); + if ($wait) { + \Clue\React\Block\sleep(0.2, $loop); + } + + $this->assertFalse($wait); + } + + public function testQueryResolvesIfServerSendsValidResponse() + { + $response = new Message(); + + $parser = $this->getMockBuilder('React\Dns\Protocol\Parser')->getMock(); + $parser->expects($this->once())->method('parseMessage')->with('data')->willReturn($response); + + $loop = Factory::create(); + + $server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND); + $loop->addReadStream($server, function ($server) { + $data = stream_socket_recvfrom($server, 512, 0, $peer); + stream_socket_sendto($server, 'data', 0, $peer); + }); + + $address = stream_socket_get_name($server, false); + $executor = new DatagramTransportExecutor($loop, $parser); + + $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + + $promise = $executor->query($address, $query); + $response = \Clue\React\Block\await($promise, $loop, 0.2); + + $this->assertInstanceOf('React\Dns\Model\Message', $response); + } +} From 99d42cd6246b50fe0174248e49558571d947fae1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 22 Jun 2018 12:09:43 +0200 Subject: [PATCH 2/2] Validate incoming DNS response messages to avoid cache poisoning attacks --- src/Model/Message.php | 38 +++++++- src/Query/DatagramTransportExecutor.php | 20 ++-- tests/Query/DatagramTransportExecutorTest.php | 92 ++++++++++++------- 3 files changed, 109 insertions(+), 41 deletions(-) diff --git a/src/Model/Message.php b/src/Model/Message.php index cbe41b74..1b7421c6 100644 --- a/src/Model/Message.php +++ b/src/Model/Message.php @@ -3,7 +3,6 @@ namespace React\Dns\Model; use React\Dns\Query\Query; -use React\Dns\Model\Record; class Message { @@ -73,8 +72,29 @@ public static function createResponseWithAnswersForQuery(Query $query, array $an return $response; } + /** + * generates a random 16 bit message ID + * + * This uses a CSPRNG so that an outside attacker that is sending spoofed + * DNS response messages can not guess the message ID to avoid possible + * cache poisoning attacks. + * + * The `random_int()` function is only available on PHP 7+ or when + * https://github.com/paragonie/random_compat is installed. As such, using + * the latest supported PHP version is highly recommended. This currently + * falls back to a less secure random number generator on older PHP versions + * in the hope that this system is properly protected against outside + * attackers, for example by using one of the common local DNS proxy stubs. + * + * @return int + * @see self::getId() + * @codeCoverageIgnore + */ private static function generateId() { + if (function_exists('random_int')) { + return random_int(0, 0xffff); + } return mt_rand(0, 0xffff); } @@ -99,6 +119,22 @@ public function __construct() $this->header = new HeaderBag(); } + /** + * Returns the 16 bit message ID + * + * The response message ID has to match the request message ID. This allows + * the receiver to verify this is the correct response message. An outside + * attacker may try to inject fake responses by "guessing" the message ID, + * so this should use a proper CSPRNG to avoid possible cache poisoning. + * + * @return int + * @see self::generateId() + */ + public function getId() + { + return $this->header->get('id'); + } + public function prepare() { $this->header->populateCounts($this); diff --git a/src/Query/DatagramTransportExecutor.php b/src/Query/DatagramTransportExecutor.php index 81ff323b..914a5a20 100644 --- a/src/Query/DatagramTransportExecutor.php +++ b/src/Query/DatagramTransportExecutor.php @@ -125,23 +125,29 @@ public function query($nameserver, Query $query) }); $parser = $this->parser; - $loop->addReadStream($socket, function ($socket) use ($loop, $deferred, $query, $parser) { + $loop->addReadStream($socket, function ($socket) use ($loop, $deferred, $query, $parser, $request) { // try to read a single data packet from the DNS server // ignoring any errors, this is uses UDP packets and not a stream of data $data = @\fread($socket, 512); - // we only react to the first message, so immediately remove socket from loop and close - $loop->removeReadStream($socket); - \fclose($socket); - try { $response = $parser->parseMessage($data); } catch (\Exception $e) { - // reject if we received an invalid message from remote server - $deferred->reject($e); + // ignore and await next if we received an invalid message from remote server + // this may as well be a fake response from an attacker (possible DOS) return; } + // ignore and await next if we received an unexpected response ID + // this may as well be a fake response from an attacker (possible cache poisoning) + if ($response->getId() !== $request->getId()) { + return; + } + + // we only react to the first valid message, so remove socket from loop and close + $loop->removeReadStream($socket); + \fclose($socket); + if ($response->header->isTruncated()) { $deferred->reject(new \RuntimeException('DNS query for ' . $query->name . ' failed: The server returned a truncated result for a UDP query, but retrying via TCP is currently not supported')); return; diff --git a/tests/Query/DatagramTransportExecutorTest.php b/tests/Query/DatagramTransportExecutorTest.php index 982b0ef7..d666e297 100644 --- a/tests/Query/DatagramTransportExecutorTest.php +++ b/tests/Query/DatagramTransportExecutorTest.php @@ -7,6 +7,8 @@ use React\Dns\Query\Query; use React\Dns\Model\Message; use React\EventLoop\Factory; +use React\Dns\Protocol\Parser; +use React\Dns\Protocol\BinaryDumper; class DatagramTransportExecutorTest extends TestCase { @@ -60,7 +62,7 @@ public function testQueryRejectsOnCancellation() $promise->then(null, $this->expectCallableOnce()); } - public function testQueryRejectsIfServerRejectsNetworkPacket() + public function testQueryKeepsPendingIfServerRejectsNetworkPacket() { $loop = Factory::create(); @@ -77,19 +79,11 @@ function ($e) use (&$wait) { } ); - // run loop for short period to ensure we detect connection ICMP rejection error - \Clue\React\Block\sleep(0.01, $loop); - if ($wait) { - \Clue\React\Block\sleep(0.2, $loop); - if ($wait) { - $this->markTestSkipped('Did not receive an error (your OS may drop UDP packets to unbound port?)'); - } - } - - $this->assertFalse($wait); + \Clue\React\Block\sleep(0.2, $loop); + $this->assertTrue($wait); } - public function testQueryRejectsIfServerSendInvalidMessage() + public function testQueryKeepsPendingIfServerSendInvalidMessage() { $loop = Factory::create(); @@ -113,33 +107,64 @@ function ($e) use (&$wait) { } ); - // run loop for short period to ensure we detect connection ICMP rejection error - \Clue\React\Block\sleep(0.01, $loop); - if ($wait) { - \Clue\React\Block\sleep(0.2, $loop); - } + \Clue\React\Block\sleep(0.2, $loop); + $this->assertTrue($wait); + } - $this->assertFalse($wait); + public function testQueryKeepsPendingIfServerSendInvalidId() + { + $parser = new Parser(); + $dumper = new BinaryDumper(); + + $loop = Factory::create(); + + $server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND); + $loop->addReadStream($server, function ($server) use ($parser, $dumper) { + $data = stream_socket_recvfrom($server, 512, 0, $peer); + + $message = $parser->parseMessage($data); + $message->header->set('id', 0); + + stream_socket_sendto($server, $dumper->toBinary($message), 0, $peer); + }); + + $address = stream_socket_get_name($server, false); + $executor = new DatagramTransportExecutor($loop, $parser, $dumper); + + $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + + $wait = true; + $promise = $executor->query($address, $query)->then( + null, + function ($e) use (&$wait) { + $wait = false; + throw $e; + } + ); + + \Clue\React\Block\sleep(0.2, $loop); + $this->assertTrue($wait); } public function testQueryRejectsIfServerSendsTruncatedResponse() { - $response = new Message(); - $response->header->set('tc', 1); - - $parser = $this->getMockBuilder('React\Dns\Protocol\Parser')->getMock(); - $parser->expects($this->once())->method('parseMessage')->with('data')->willReturn($response); + $parser = new Parser(); + $dumper = new BinaryDumper(); $loop = Factory::create(); $server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND); - $loop->addReadStream($server, function ($server) { + $loop->addReadStream($server, function ($server) use ($parser, $dumper) { $data = stream_socket_recvfrom($server, 512, 0, $peer); - stream_socket_sendto($server, 'data', 0, $peer); + + $message = $parser->parseMessage($data); + $message->header->set('tc', 1); + + stream_socket_sendto($server, $dumper->toBinary($message), 0, $peer); }); $address = stream_socket_get_name($server, false); - $executor = new DatagramTransportExecutor($loop, $parser); + $executor = new DatagramTransportExecutor($loop, $parser, $dumper); $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); @@ -163,21 +188,22 @@ function ($e) use (&$wait) { public function testQueryResolvesIfServerSendsValidResponse() { - $response = new Message(); - - $parser = $this->getMockBuilder('React\Dns\Protocol\Parser')->getMock(); - $parser->expects($this->once())->method('parseMessage')->with('data')->willReturn($response); + $parser = new Parser(); + $dumper = new BinaryDumper(); $loop = Factory::create(); $server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND); - $loop->addReadStream($server, function ($server) { + $loop->addReadStream($server, function ($server) use ($parser, $dumper) { $data = stream_socket_recvfrom($server, 512, 0, $peer); - stream_socket_sendto($server, 'data', 0, $peer); + + $message = $parser->parseMessage($data); + + stream_socket_sendto($server, $dumper->toBinary($message), 0, $peer); }); $address = stream_socket_get_name($server, false); - $executor = new DatagramTransportExecutor($loop, $parser); + $executor = new DatagramTransportExecutor($loop, $parser, $dumper); $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN);