From c989246bd0ed93fcc5bef7d25dd356b0483d4a31 Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Mon, 13 Dec 2021 19:14:21 +0100 Subject: [PATCH 1/2] Handle & report server request errors as 'requestError' events --- README.md | 4 ++ server/dns.js | 2 + server/doh.js | 115 ++++++++++++++++++++++++++------------------------ server/tcp.js | 10 +++-- server/udp.js | 8 +++- 5 files changed, 79 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index e612f73..40c1e65 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,10 @@ server.on('request', (request, response, rinfo) => { console.log(request.header.id, request.questions[0]); }); +server.on('requestError', (error) => { + console.log('Client sent an invalid request', error); +}); + server.on('listening', () => { console.log(server.address()); }); diff --git a/server/dns.js b/server/dns.js index 50b778d..affbc64 100644 --- a/server/dns.js +++ b/server/dns.js @@ -35,8 +35,10 @@ class DNSServer extends EventEmitter { }); const emitRequest = (request, send, client) => this.emit('request', request, send, client); + const emitRequestError = (error) => this.emit('requestError', error); for (const server of servers) { server.on('request', emitRequest); + server.on('requestError', emitRequestError); } if (options.handle) { diff --git a/server/doh.js b/server/doh.js index 00c4b85..2beea89 100644 --- a/server/doh.js +++ b/server/doh.js @@ -43,69 +43,74 @@ class Server extends EventEmitter { } async handleRequest(client, res) { - const { method, url, headers } = client; - const { pathname, searchParams: query } = new URL(url, 'http://unused/'); - const { cors } = this; - if (cors === true) { - res.setHeader('Access-Control-Allow-Origin', '*'); - } else if (typeof cors === 'string') { - res.setHeader('Access-Control-Allow-Origin', cors); - res.setHeader('Vary', 'Origin'); - } else if (typeof cors === 'function') { - const isAllowed = cors(headers.origin); - res.setHeader('Access-Control-Allow-Origin', isAllowed ? headers.origin : 'false'); - res.setHeader('Vary', 'Origin'); - } - // debug - debug('request', method, url); - // We are only handling get and post as reqired by rfc - if ((method !== 'GET' && method !== 'POST')) { - res.writeHead(405, { 'Content-Type': 'text/plain' }); - res.write('405 Method not allowed\n'); - res.end(); - return; - } - // Check so the uri is correct - if (pathname !== '/dns-query') { - res.writeHead(404, { 'Content-Type': 'text/plain' }); - res.write('404 Not Found\n'); - res.end(); - return; - } - // Make sure the requestee is requesting the correct content type - const contentType = headers.accept; - if (contentType !== 'application/dns-message') { - res.writeHead(400, { 'Content-Type': 'text/plain' }); - res.write('400 Bad Request: Illegal content type\n'); - res.end(); - return; - } - let queryData; - if (method === 'GET') { - // Parse query string for the request data - const dns = query.get('dns'); - if (!dns) { - res.writeHead(400, { 'Content-Type': 'text/plain' }); - res.write('400 Bad Request: No query defined\n'); + try { + const { method, url, headers } = client; + const { pathname, searchParams: query } = new URL(url, 'http://unused/'); + const { cors } = this; + if (cors === true) { + res.setHeader('Access-Control-Allow-Origin', '*'); + } else if (typeof cors === 'string') { + res.setHeader('Access-Control-Allow-Origin', cors); + res.setHeader('Vary', 'Origin'); + } else if (typeof cors === 'function') { + const isAllowed = cors(headers.origin); + res.setHeader('Access-Control-Allow-Origin', isAllowed ? headers.origin : 'false'); + res.setHeader('Vary', 'Origin'); + } + // debug + debug('request', method, url); + // We are only handling get and post as reqired by rfc + if ((method !== 'GET' && method !== 'POST')) { + res.writeHead(405, { 'Content-Type': 'text/plain' }); + res.write('405 Method not allowed\n'); res.end(); return; } - // Decode from Base64Url Encoding - const base64 = decodeBase64URL(dns); - if (!base64) { + // Check so the uri is correct + if (pathname !== '/dns-query') { + res.writeHead(404, { 'Content-Type': 'text/plain' }); + res.write('404 Not Found\n'); + res.end(); + return; + } + // Make sure the requestee is requesting the correct content type + const contentType = headers.accept; + if (contentType !== 'application/dns-message') { res.writeHead(400, { 'Content-Type': 'text/plain' }); - res.write('400 Bad Request: Invalid query data\n'); + res.write('400 Bad Request: Illegal content type\n'); res.end(); return; } - // Decode Base64 to buffer - queryData = Buffer.from(base64, 'base64'); - } else if (method === 'POST') { - queryData = await readStream(client); + let queryData; + if (method === 'GET') { + // Parse query string for the request data + const dns = query.get('dns'); + if (!dns) { + res.writeHead(400, { 'Content-Type': 'text/plain' }); + res.write('400 Bad Request: No query defined\n'); + res.end(); + return; + } + // Decode from Base64Url Encoding + const base64 = decodeBase64URL(dns); + if (!base64) { + res.writeHead(400, { 'Content-Type': 'text/plain' }); + res.write('400 Bad Request: Invalid query data\n'); + res.end(); + return; + } + // Decode Base64 to buffer + queryData = Buffer.from(base64, 'base64'); + } else if (method === 'POST') { + queryData = await readStream(client); + } + // Parse DNS query and Raise event. + const message = Packet.parse(queryData); + this.emit('request', message, this.response.bind(this, res), client); + } catch (e) { + this.emit('requestError', e); + res.destroy(); } - // Parse DNS query and Raise event. - const message = Packet.parse(queryData); - this.emit('request', message, this.response.bind(this, res), client); } /** diff --git a/server/tcp.js b/server/tcp.js index 14d6815..e970612 100644 --- a/server/tcp.js +++ b/server/tcp.js @@ -11,9 +11,13 @@ class Server extends tcp.Server { } async handle(client) { - const data = await Packet.readStream(client); - const message = Packet.parse(data); - this.emit('request', message, this.response.bind(this, client), client); + try { + const data = await Packet.readStream(client); + const message = Packet.parse(data); + this.emit('request', message, this.response.bind(this, client), client); + } catch (e) { + this.emit('requestError', e); + } } response(client, message) { diff --git a/server/udp.js b/server/udp.js index ef02dd3..7cbd1df 100644 --- a/server/udp.js +++ b/server/udp.js @@ -20,8 +20,12 @@ class Server extends udp.Socket { } handle(data, rinfo) { - const message = Packet.parse(data); - this.emit('request', message, this.response.bind(this, rinfo), rinfo); + try { + const message = Packet.parse(data); + this.emit('request', message, this.response.bind(this, rinfo), rinfo); + } catch (e) { + this.emit('requestError', e); + } } response(rinfo, message) { From a63cd9e3cd001da40808c2c5b94ece5e97eefd65 Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Mon, 13 Dec 2021 19:45:21 +0100 Subject: [PATCH 2/2] Add a test for invalid packet handling --- server/tcp.js | 1 + test/index.js | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/server/tcp.js b/server/tcp.js index e970612..bbc5252 100644 --- a/server/tcp.js +++ b/server/tcp.js @@ -17,6 +17,7 @@ class Server extends tcp.Server { this.emit('request', message, this.response.bind(this, client), client); } catch (e) { this.emit('requestError', e); + client.destroy(); } } diff --git a/test/index.js b/test/index.js index 8b4698d..ab34dcf 100644 --- a/test/index.js +++ b/test/index.js @@ -2,6 +2,8 @@ const assert = require('assert'); const test = require('./test'); const { Packet, createDOHServer, createServer, TCPClient, DOHClient, UDPClient } = require('..'); const http = require('http'); +const tcp = require('net'); +const udp = require('dgram'); /* TODO: below is unused, either delete or use const request = Buffer.from([ @@ -335,6 +337,44 @@ test('server/all#simple-request', async() => { await server.close(); }); +test('server/all#invalid-request', async() => { + const server = createServer({ + doh : true, + tcp : true, + udp : true, + handle : () => {}, + }); + const servers = await server.listen(); + assert.ok(servers.udp.port > 1000); + assert.ok(servers.tcp.port > 1000); + assert.ok(servers.doh.port > 1000); + + const errors = []; + server.on('requestError', (e) => { + errors.push(e); + }); + + const tcpSocket = tcp.connect({ port: servers.tcp.port, host: '127.0.0.1' }); + tcpSocket.on('connect', () => tcpSocket.end('INVALID')); + + const udpSocket = udp.createSocket('udp4'); + udpSocket.send('INVALID', servers.udp.port, '127.0.0.1', () => udpSocket.close()); + + const dohConn = http.get(`http://127.0.0.1:${servers.doh.port}/dns-query?dns=INVALID`, { + headers: { accept: 'application/dns-message' }, + }).on('error', () => {}); + + await Promise.all([ + new Promise((resolve) => tcpSocket.on('close', resolve)), + new Promise((resolve) => udpSocket.on('close', resolve)), + new Promise((resolve) => dohConn.on('close', resolve)), + ]); + + assert.equal(errors.length, 3); + + await server.close(); +}); + function get(url, options) { return new Promise((resolve, reject) => { try {