Skip to content

Commit 99d42cd

Browse files
clueclue
committed
Validate incoming DNS response messages to avoid cache poisoning attacks
1 parent 4dd2cde commit 99d42cd

3 files changed

Lines changed: 109 additions & 41 deletions

File tree

‎src/Model/Message.php‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
namespace React\Dns\Model;
44

55
use React\Dns\Query\Query;
6-
use React\Dns\Model\Record;
76

87
class Message
98
{
@@ -73,8 +72,29 @@ public static function createResponseWithAnswersForQuery(Query $query, array $an
7372
return $response;
7473
}
7574

75+
/**
76+
* generates a random 16 bit message ID
77+
*
78+
* This uses a CSPRNG so that an outside attacker that is sending spoofed
79+
* DNS response messages can not guess the message ID to avoid possible
80+
* cache poisoning attacks.
81+
*
82+
* The `random_int()` function is only available on PHP 7+ or when
83+
* https://github.com/paragonie/random_compat is installed. As such, using
84+
* the latest supported PHP version is highly recommended. This currently
85+
* falls back to a less secure random number generator on older PHP versions
86+
* in the hope that this system is properly protected against outside
87+
* attackers, for example by using one of the common local DNS proxy stubs.
88+
*
89+
* @return int
90+
* @see self::getId()
91+
* @codeCoverageIgnore
92+
*/
7693
private static function generateId()
7794
{
95+
if (function_exists('random_int')) {
96+
return random_int(0, 0xffff);
97+
}
7898
return mt_rand(0, 0xffff);
7999
}
80100

@@ -99,6 +119,22 @@ public function __construct()
99119
$this->header = new HeaderBag();
100120
}
101121

122+
/**
123+
* Returns the 16 bit message ID
124+
*
125+
* The response message ID has to match the request message ID. This allows
126+
* the receiver to verify this is the correct response message. An outside
127+
* attacker may try to inject fake responses by "guessing" the message ID,
128+
* so this should use a proper CSPRNG to avoid possible cache poisoning.
129+
*
130+
* @return int
131+
* @see self::generateId()
132+
*/
133+
public function getId()
134+
{
135+
return $this->header->get('id');
136+
}
137+
102138
public function prepare()
103139
{
104140
$this->header->populateCounts($this);

‎src/Query/DatagramTransportExecutor.php‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -125,23 +125,29 @@ public function query($nameserver, Query $query)
125125
});
126126

127127
$parser = $this->parser;
128-
$loop->addReadStream($socket, function ($socket) use ($loop, $deferred, $query, $parser) {
128+
$loop->addReadStream($socket, function ($socket) use ($loop, $deferred, $query, $parser, $request) {
129129
// try to read a single data packet from the DNS server
130130
// ignoring any errors, this is uses UDP packets and not a stream of data
131131
$data = @\fread($socket, 512);
132132

133-
// we only react to the first message, so immediately remove socket from loop and close
134-
$loop->removeReadStream($socket);
135-
\fclose($socket);
136-
137133
try {
138134
$response = $parser->parseMessage($data);
139135
} catch (\Exception $e) {
140-
// reject if we received an invalid message from remote server
141-
$deferred->reject($e);
136+
// ignore and await next if we received an invalid message from remote server
137+
// this may as well be a fake response from an attacker (possible DOS)
142138
return;
143139
}
144140

141+
// ignore and await next if we received an unexpected response ID
142+
// this may as well be a fake response from an attacker (possible cache poisoning)
143+
if ($response->getId() !== $request->getId()) {
144+
return;
145+
}
146+
147+
// we only react to the first valid message, so remove socket from loop and close
148+
$loop->removeReadStream($socket);
149+
\fclose($socket);
150+
145151
if ($response->header->isTruncated()) {
146152
$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'));
147153
return;

‎tests/Query/DatagramTransportExecutorTest.php‎

Lines changed: 59 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
use React\Dns\Query\Query;
88
use React\Dns\Model\Message;
99
use React\EventLoop\Factory;
10+
use React\Dns\Protocol\Parser;
11+
use React\Dns\Protocol\BinaryDumper;
1012

1113
class DatagramTransportExecutorTest extends TestCase
1214
{
@@ -60,7 +62,7 @@ public function testQueryRejectsOnCancellation()
6062
$promise->then(null, $this->expectCallableOnce());
6163
}
6264

63-
public function testQueryRejectsIfServerRejectsNetworkPacket()
65+
public function testQueryKeepsPendingIfServerRejectsNetworkPacket()
6466
{
6567
$loop = Factory::create();
6668

@@ -77,19 +79,11 @@ function ($e) use (&$wait) {
7779
}
7880
);
7981

80-
// run loop for short period to ensure we detect connection ICMP rejection error
81-
\Clue\React\Block\sleep(0.01, $loop);
82-
if ($wait) {
83-
\Clue\React\Block\sleep(0.2, $loop);
84-
if ($wait) {
85-
$this->markTestSkipped('Did not receive an error (your OS may drop UDP packets to unbound port?)');
86-
}
87-
}
88-
89-
$this->assertFalse($wait);
82+
\Clue\React\Block\sleep(0.2, $loop);
83+
$this->assertTrue($wait);
9084
}
9185

92-
public function testQueryRejectsIfServerSendInvalidMessage()
86+
public function testQueryKeepsPendingIfServerSendInvalidMessage()
9387
{
9488
$loop = Factory::create();
9589

@@ -113,33 +107,64 @@ function ($e) use (&$wait) {
113107
}
114108
);
115109

116-
// run loop for short period to ensure we detect connection ICMP rejection error
117-
\Clue\React\Block\sleep(0.01, $loop);
118-
if ($wait) {
119-
\Clue\React\Block\sleep(0.2, $loop);
120-
}
110+
\Clue\React\Block\sleep(0.2, $loop);
111+
$this->assertTrue($wait);
112+
}
121113

122-
$this->assertFalse($wait);
114+
public function testQueryKeepsPendingIfServerSendInvalidId()
115+
{
116+
$parser = new Parser();
117+
$dumper = new BinaryDumper();
118+
119+
$loop = Factory::create();
120+
121+
$server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND);
122+
$loop->addReadStream($server, function ($server) use ($parser, $dumper) {
123+
$data = stream_socket_recvfrom($server, 512, 0, $peer);
124+
125+
$message = $parser->parseMessage($data);
126+
$message->header->set('id', 0);
127+
128+
stream_socket_sendto($server, $dumper->toBinary($message), 0, $peer);
129+
});
130+
131+
$address = stream_socket_get_name($server, false);
132+
$executor = new DatagramTransportExecutor($loop, $parser, $dumper);
133+
134+
$query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN);
135+
136+
$wait = true;
137+
$promise = $executor->query($address, $query)->then(
138+
null,
139+
function ($e) use (&$wait) {
140+
$wait = false;
141+
throw $e;
142+
}
143+
);
144+
145+
\Clue\React\Block\sleep(0.2, $loop);
146+
$this->assertTrue($wait);
123147
}
124148

125149
public function testQueryRejectsIfServerSendsTruncatedResponse()
126150
{
127-
$response = new Message();
128-
$response->header->set('tc', 1);
129-
130-
$parser = $this->getMockBuilder('React\Dns\Protocol\Parser')->getMock();
131-
$parser->expects($this->once())->method('parseMessage')->with('data')->willReturn($response);
151+
$parser = new Parser();
152+
$dumper = new BinaryDumper();
132153

133154
$loop = Factory::create();
134155

135156
$server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND);
136-
$loop->addReadStream($server, function ($server) {
157+
$loop->addReadStream($server, function ($server) use ($parser, $dumper) {
137158
$data = stream_socket_recvfrom($server, 512, 0, $peer);
138-
stream_socket_sendto($server, 'data', 0, $peer);
159+
160+
$message = $parser->parseMessage($data);
161+
$message->header->set('tc', 1);
162+
163+
stream_socket_sendto($server, $dumper->toBinary($message), 0, $peer);
139164
});
140165

141166
$address = stream_socket_get_name($server, false);
142-
$executor = new DatagramTransportExecutor($loop, $parser);
167+
$executor = new DatagramTransportExecutor($loop, $parser, $dumper);
143168

144169
$query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN);
145170

@@ -163,21 +188,22 @@ function ($e) use (&$wait) {
163188

164189
public function testQueryResolvesIfServerSendsValidResponse()
165190
{
166-
$response = new Message();
167-
168-
$parser = $this->getMockBuilder('React\Dns\Protocol\Parser')->getMock();
169-
$parser->expects($this->once())->method('parseMessage')->with('data')->willReturn($response);
191+
$parser = new Parser();
192+
$dumper = new BinaryDumper();
170193

171194
$loop = Factory::create();
172195

173196
$server = stream_socket_server('udp://127.0.0.1:0', $errno, $errstr, STREAM_SERVER_BIND);
174-
$loop->addReadStream($server, function ($server) {
197+
$loop->addReadStream($server, function ($server) use ($parser, $dumper) {
175198
$data = stream_socket_recvfrom($server, 512, 0, $peer);
176-
stream_socket_sendto($server, 'data', 0, $peer);
199+
200+
$message = $parser->parseMessage($data);
201+
202+
stream_socket_sendto($server, $dumper->toBinary($message), 0, $peer);
177203
});
178204

179205
$address = stream_socket_get_name($server, false);
180-
$executor = new DatagramTransportExecutor($loop, $parser);
206+
$executor = new DatagramTransportExecutor($loop, $parser, $dumper);
181207

182208
$query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN);
183209

0 commit comments

Comments
 (0)