fix(netflow): reject zero-length template fields to stop decoder DoS (v0.17.6-beta)
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
b6d161fc9d
commit
7e11115a11
@@ -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": {
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 });
|
||||
|
||||
Reference in New Issue
Block a user