Fix crash on bad inbound SOCKS connections

Author: pimterry

src/server/socks-server.ts

@@ -79,7 +79,7 @@ export function buildSocksServer(options: SocksServerOptions): SocksServer {
     return net.createServer(handleSocksConnect);
 
 
-    async function handleSocksConnect(this: net.Server, socket: net.Socket) {
+    async function handleSocksConnect(this: net.Server, socket: net.Socket): Promise<void> {
         const server = this;
         // Until we pass this socket onwards, we handle (and drop) any errors on it:
         socket.on('error', ignoreError);
@@ -88,18 +88,18 @@ export function buildSocksServer(options: SocksServerOptions): SocksServer {
             const firstByte = await readBytes(socket, 1);;
             const version = firstByte[0];
             if (version === 0x04) {
-                return handleSocksV4(socket, (address: SocksTcpAddress) => {
+                await handleSocksV4(socket, (address: SocksTcpAddress) => {
                     socket.removeListener('error', ignoreError);
                     server.emit('socks-tcp-connect', socket, address);
                 });
             } else if (version === 0x05) {
-                return handleSocksV5(socket, (address: SocksTcpAddress) => {
+                await handleSocksV5(socket, (address: SocksTcpAddress) => {
                     socket.removeListener('error', ignoreError);
                     server.emit('socks-tcp-connect', socket, address);
                 });
             } else {
                 // Should never happen, since this is sniffed by Httpolyglot, but just in case:
-                return resetOrDestroy(socket);
+                resetOrDestroy(socket);
             }
         } catch (err) {
             // We log but otherwise ignore failures, e.g. if the client closes the
@@ -329,7 +329,7 @@ async function handleUsernamePasswordMetadata(socket: net.Socket) {
 async function readBytes(socket: net.Socket, length?: number | undefined): Promise<Buffer> {
     const buffer = socket.read(length);
     if (buffer === null) {
-        return new Promise((resolve, reject) => {
+        return new Promise<Buffer>((resolve, reject) => {
             socket.once('readable', () => resolve(readBytes(socket, length)));
             socket.once('close', () => reject(new Error('Socket closed')));
             socket.once('error', reject);

test/integration/proxying/socks-proxying.spec.ts

@@ -10,9 +10,11 @@ import {
     getLocal
 } from "../../..";
 import {
+    delay,
     expect,
     getDeferred,
     nodeOnly,
+    openRawSocket,
     openSocksSocket,
     sendRawRequest
 } from "../../test-utils";
@@ -141,6 +143,30 @@ nodeOnly(() => {
                 expect((await passthroughEvent).port).to.equal(remoteServer.port.toString());
             });
 
+            it("should not crash given a failed SOCKS handshake", async () => {
+                const events = [];
+                await server.on('request-initiated', (req) => events.push(req));
+                await server.on('client-error', (err) => events.push(err));
+                await server.on('tls-client-error', (err) => events.push(err));
+                await server.on('raw-passthrough-opened', (err) => events.push(err));
+
+                const socket = await openRawSocket(server);
+                socket.write(Buffer.from([0x05, 0x01, 0x00])); // Version 5, 1 method, no auth
+
+                const result = await new Promise((resolve, reject) => {
+                    socket.once('data', resolve);
+                    socket.on('error', reject);
+                })
+                expect(result).to.deep.equal(Buffer.from([0x05, 0x0])); // Server accepts no auth
+
+                // Server is now waiting for destination - we reset the connection instead
+                socket.resetAndDestroy();
+
+                // No crash! And no events.
+                await delay(10);
+                expect(events.length).to.equal(0);
+            });
+
         });
 
         describe("with only custom metadata auth supported", () => {