From 7e11115a11ba423f6e9034e607985dd323388fed Mon Sep 17 00:00:00 2001 From: 2GT-Rich <107219997+2GT-Rich@users.noreply.github.com> Date: Fri, 31 Jul 2026 21:31:25 -0700 Subject: [PATCH] fix(netflow): reject zero-length template fields to stop decoder DoS (v0.17.6-beta) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A NetFlow v9/IPFIX template declaring a field of length 0 produced a template with minLength 0. parseDataSet's loop condition (off + minLength <= setEnd) then stayed true while the field pass advanced `off` by 0 — an infinite loop. Impact: the collector socket is UDP and unauthenticated, so any host able to reach the NetFlow port could hang the entire backend with a single 36-byte packet — Node is single-threaded, so the event loop never yields again and the API, poller, sockets and health checks all stop. Only reachable while NetFlow is enabled (off by default). Two layers of defense in decoder.ts: - parseTemplates rejects any field with length 0 (meaningless in v9/IPFIX) and refuses to cache a template whose computed minLength is 0. - parseDataSet asserts forward progress: if a full field pass consumes no bytes, break instead of spinning. Guards any future zero-width path. Adds 5 regression tests covering the hang case, a zero-length field among valid ones, a variable-length field whose length byte is 0, absurd/truncated field lengths, and that valid v9/IPFIX traffic still decodes afterwards. Verified: the exact packet that previously hung the process now returns immediately with no template cached; 20 attack packets fired at the live collector left the backend healthy and serving, and real exporter flows kept decoding (102 -> 152 flows across the test). 119 backend tests pass. Found via an adversarial review; the report identified the infinite loop but attributed it to the variable-length branch, which provably terminates. Co-Authored-By: Claude Opus 4.8 --- README.md | 2 +- backend/package.json | 2 +- .../netflow/__tests__/decoder.test.ts | 81 +++++++++++++++++++ backend/src/services/netflow/decoder.ts | 10 ++- frontend/package.json | 2 +- frontend/src/version.ts | 2 +- 6 files changed, 94 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index d915d6b..b5e5e4c 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ A self-hosted, full-stack network management platform for MikroTik devices. Monitor, configure, and manage your entire MikroTik infrastructure — routers, switches, and wireless access points — from a single web interface. -![Version](https://img.shields.io/badge/version-0.17.5_Beta-blue) +![Version](https://img.shields.io/badge/version-0.17.6_Beta-blue) ![License](https://img.shields.io/badge/license-AGPLv3-blue) ![Docker](https://img.shields.io/badge/docker-compose-2496ED?logo=docker&logoColor=white) ![TypeScript](https://img.shields.io/badge/TypeScript-5.3-3178C6?logo=typescript&logoColor=white) diff --git a/backend/package.json b/backend/package.json index 6a0a086..8de973c 100644 --- a/backend/package.json +++ b/backend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-backend", - "version": "0.17.5-beta", + "version": "0.17.6-beta", "description": "Mikrotik Network Management Platform - Backend API", "main": "dist/index.js", "scripts": { diff --git a/backend/src/services/netflow/__tests__/decoder.test.ts b/backend/src/services/netflow/__tests__/decoder.test.ts index fb3c72c..bad03ac 100644 --- a/backend/src/services/netflow/__tests__/decoder.test.ts +++ b/backend/src/services/netflow/__tests__/decoder.test.ts @@ -269,3 +269,84 @@ describe('IPFIX decoding', () => { expect(result.flows[0].srcAddr).toMatch(/^fd00:/); }); }); + +describe('malformed / hostile packets', () => { + // Regression: a template field of length 0 made a record zero-width, so the + // data-set loop never advanced and spun forever — a single unauthenticated UDP + // packet could hang the whole (single-threaded) backend. If this regresses the + // decode call never returns, so these tests hang rather than fail — that is + // still a loud CI failure, and the cache assertions pin the actual guard. + it('rejects a template containing a zero-length field instead of looping forever', () => { + const cache: TemplateCache = new Map(); + const packet = ipfixPacket(1, [ + flowset(2, templateBody(256, [{ id: 1, length: 0 }])), + flowset(256, Buffer.alloc(4)), + ]); + + const result = decodePacket(packet, '10.0.0.9', cache); + expect(result.templatesParsed).toBe(0); + expect(cache.size).toBe(0); + expect(result.flows).toHaveLength(0); + }); + + it('rejects the whole template when a zero-length field sits among valid ones', () => { + const cache: TemplateCache = new Map(); + const packet = ipfixPacket(1, [ + flowset(2, templateBody(257, [ + { id: 8, length: 4 }, + { id: 12, length: 0 }, // malformed + { id: 1, length: 4 }, + ])), + ]); + + expect(decodePacket(packet, '10.0.0.9', cache).templatesParsed).toBe(0); + expect(cache.size).toBe(0); + }); + + it('terminates on a variable-length field whose length byte is zero', () => { + const cache: TemplateCache = new Map(); + const packet = ipfixPacket(1, [ + flowset(2, templateBody(258, [{ id: 1, length: 0xffff }])), + flowset(258, Buffer.alloc(8)), // every length byte reads as 0 + ]); + + const result = decodePacket(packet, '10.0.0.9', cache); + expect(result.templatesParsed).toBe(1); + expect(result.flows).toHaveLength(0); + }); + + it('handles absurd field lengths and truncated records without throwing', () => { + const cache: TemplateCache = new Map(); + const huge = ipfixPacket(1, [ + flowset(2, templateBody(259, [{ id: 1, length: 60000 }])), + flowset(259, Buffer.alloc(8)), + ]); + expect(() => decodePacket(huge, '10.0.0.9', cache)).not.toThrow(); + + // Declares a 16-byte IPv6 field but supplies only 4 bytes of data + const truncated = ipfixPacket(1, [ + flowset(2, templateBody(260, [{ id: 27, length: 16 }, { id: 1, length: 4 }])), + flowset(260, Buffer.alloc(4)), + ]); + const result = decodePacket(truncated, '10.0.0.9', cache); + expect(result.flows).toHaveLength(0); + }); + + it('still decodes valid traffic after rejecting a malformed template', () => { + const cache: TemplateCache = new Map(); + decodePacket( + ipfixPacket(1, [flowset(2, templateBody(256, [{ id: 1, length: 0 }]))]), + '10.0.0.9', + cache + ); + + const good = v9Packet(0, [ + flowset(0, templateBody(256, V9_FIELDS)), + flowset(256, record('192.168.1.10', '1.1.1.1', 50000, 53, 17, 120, 2)), + ]); + const result = decodePacket(good, '10.0.0.9', cache); + expect(result.templatesParsed).toBe(1); + expect(result.flows).toHaveLength(1); + expect(result.flows[0].bytes).toBe(120); + }); +}); diff --git a/backend/src/services/netflow/decoder.ts b/backend/src/services/netflow/decoder.ts index 008282a..9380162 100644 --- a/backend/src/services/netflow/decoder.ts +++ b/backend/src/services/netflow/decoder.ts @@ -106,10 +106,13 @@ function parseTemplates( id = id & 0x7fff; enterprise = true; } + // A zero-length field is meaningless in v9/IPFIX and makes a record + // zero-width, which would spin parseDataSet forever. Reject the template. + if (length === 0) { ok = false; break; } fields.push({ id, length, enterprise }); minLength += length === 0xffff ? 1 : length; } - if (!ok) break; + if (!ok || minLength === 0) break; cache.set(`${exporterKey}|${sourceId}|${templateId}`, { fields, minLength }); parsed++; } @@ -126,6 +129,7 @@ function parseDataSet( let off = start; // Records are packed back to back; trailing bytes < minLength are padding while (off + template.minLength <= setEnd) { + const recordStart = off; let srcAddr = ''; let dstAddr = ''; let srcPort = 0; @@ -167,6 +171,10 @@ function parseDataSet( } if (!valid) break; + // Forward-progress guard: a record that consumed no bytes would loop forever. + // Template validation should prevent this, but never let a malformed packet + // hang the (single-threaded) event loop. + if (off <= recordStart) break; // Only emit records that look like flows (options/stats records lack these) if (srcAddr && dstAddr && bytes >= 0) { flows.push({ srcAddr, dstAddr, srcPort, dstPort, protocol, bytes, packets }); diff --git a/frontend/package.json b/frontend/package.json index 3c6851b..5028086 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mikrotik-manager-frontend", - "version": "0.17.5-beta", + "version": "0.17.6-beta", "private": true, "type": "module", "scripts": { diff --git a/frontend/src/version.ts b/frontend/src/version.ts index 100dadb..29c2567 100644 --- a/frontend/src/version.ts +++ b/frontend/src/version.ts @@ -1 +1 @@ -export const APP_VERSION = 'v0.17.5 Beta'; +export const APP_VERSION = 'v0.17.6 Beta';