Skip to content

Commit 5dacd4e

Browse files
fix(dns): answer NODATA for a name that exists, not NXDOMAIN (#192)
The bridge only looked a name up for A and AAAA questions. Every other type fell through with no address, and buildResponse read "no address" as "no such name", so a claimed name answered NXDOMAIN for TXT, MX and HTTPS/SVCB. That is the failure the buildResponse comment already exists to prevent: NXDOMAIN denies the whole name, not the question that was asked, so a resolver may apply it to everything else it has in flight. Chrome and Safari send HTTPS/SVCB alongside the A and AAAA pair on every navigation, so the denial landed in the same page load it broke. The same collapse hit an address question whose name has no address to serve — a live target that is a hostname, or one that names a port, both of which targetAddress refuses on purpose. The name exists; it just has nothing to put in an A record. buildResponse now takes existence as its own argument, defaulting to Boolean(address) so every four-argument caller is byte-identical. answerPolicy reports existence and address together; answerFor keeps its old signature and result and delegates to it. Co-authored-by: clawedassistant26 <307253840+clawedassistant26@users.noreply.github.com>
1 parent 29a268a commit 5dacd4e

2 files changed

Lines changed: 338 additions & 15 deletions

File tree

‎src/dns.mjs‎

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -158,17 +158,23 @@ function ipv6(address) {
158158
* query every browser sends alongside the AAAA one has to come back NOERROR
159159
* with no answers. Answering NXDOMAIN there teaches the resolver the name is
160160
* gone and takes the AAAA lookup down with it.
161+
*
162+
* `exists` is that distinction on its own. Holding an address implies the name
163+
* exists, so it defaults to exactly that, but the reverse does not hold: a name
164+
* can exist and have no address to hand back — because the question was for a
165+
* type this bridge does not serve, or because the target is a hostname rather
166+
* than an address. Those are NODATA, not NXDOMAIN.
161167
*/
162-
export function buildResponse(query, buf, address, ttl = DEFAULT_TTL) {
168+
export function buildResponse(query, buf, address, ttl = DEFAULT_TTL, exists = Boolean(address)) {
163169
const question = buf.subarray(12, query.questionEnd);
164170
const wantsV6 = query.type === TYPE_AAAA;
165171
const rdata = address ? (wantsV6 ? ipv6(address) : ipv4(address)) : null;
166172

167173
if (!rdata) {
168174
return Buffer.concat([
169175
header(query.id, {
170-
// We have an address, just not in the family asked for: NODATA.
171-
rcode: address ? RCODE_OK : RCODE_NXDOMAIN,
176+
// The name is here, we just have nothing to say for this question: NODATA.
177+
rcode: exists ? RCODE_OK : RCODE_NXDOMAIN,
172178
answers: 0,
173179
recursionDesired: query.recursionDesired,
174180
}),
@@ -291,18 +297,32 @@ export async function resolveName(
291297
/* -------------------------------------------------------------------- server */
292298

293299
/**
294-
* The address to answer with, or null for NXDOMAIN.
300+
* What to say about a name: whether it is here at all, and the address to
301+
* answer with when there is one.
295302
*
296303
* Kept separate from the socket so the policy is testable on its own. A name we
297-
* could not look up gets NXDOMAIN rather than the parking address: a registry
298-
* outage must not silently redirect every name on the machine to a parking page.
304+
* could not look up is not here rather than parked: a registry outage must not
305+
* silently redirect every name on the machine to a parking page.
306+
*
307+
* `wantsAddress` is false for the questions this bridge does not serve (TXT, MX,
308+
* HTTPS/SVCB). Those still need to know the name is here, because saying
309+
* NXDOMAIN to one question denies the name for every other one too.
299310
*/
300-
export async function answerFor(name, options = {}) {
301-
const { parkingAddress } = options;
311+
export async function answerPolicy(name, options = {}) {
312+
const { parkingAddress, wantsAddress = true } = options;
302313
const result = await resolveName(name, options);
303-
if (result.status === "live") return targetAddress(result.target);
304-
if (result.status === "parked") return parkingAddress || null;
305-
return null;
314+
const exists = result.status === "live" || result.status === "parked";
315+
if (!exists || !wantsAddress) return { exists, address: null };
316+
if (result.status === "live") return { exists, address: targetAddress(result.target) };
317+
return { exists, address: parkingAddress || null };
318+
}
319+
320+
/**
321+
* The address to answer with, or null when there is none.
322+
*/
323+
export async function answerFor(name, options = {}) {
324+
const { address } = await answerPolicy(name, options);
325+
return address;
306326
}
307327

308328
/**
@@ -351,14 +371,19 @@ export function createServer(options = {}) {
351371
const query = parseQuery(msg);
352372
if (!query) return; // malformed, or a response — say nothing at all
353373
let address = null;
374+
let exists = false;
354375
// Only address questions can be answered with an address; everything else
355-
// (MX, TXT, SRV) gets an honest empty NOERROR/NXDOMAIN rather than a lie.
356-
if ((query.type === TYPE_A || query.type === TYPE_AAAA) && query.class === CLASS_IN) {
357-
address = await answerFor(query.name, options).catch(() => null);
376+
// (MX, TXT, HTTPS) gets an honest empty NOERROR rather than a lie. It still
377+
// has to be looked up: a browser asks HTTPS/SVCB beside every A and AAAA,
378+
// and NXDOMAIN to that one denies the name for the whole page load.
379+
if (query.class === CLASS_IN) {
380+
const wantsAddress = query.type === TYPE_A || query.type === TYPE_AAAA;
381+
const policy = await answerPolicy(query.name, { ...options, wantsAddress }).catch(() => null);
382+
if (policy) ({ exists, address } = policy);
358383
}
359384
onQuery({ name: query.name, type: query.type, address });
360385
try {
361-
socket.send(buildResponse(query, msg, address, ttl), rinfo.port, rinfo.address);
386+
socket.send(buildResponse(query, msg, address, ttl, exists), rinfo.port, rinfo.address);
362387
} catch {
363388
/* client vanished — nothing useful to do */
364389
}

‎test/dns-nodata.test.mjs‎

Lines changed: 298 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,298 @@
1+
/**
2+
* A name that exists must not answer NXDOMAIN.
3+
*
4+
* NXDOMAIN denies the whole name, not just the question that was asked, so a
5+
* resolver may apply it to every other type it has in flight. A browser asks
6+
* HTTPS/SVCB beside every A and AAAA, which is why answering that one question
7+
* wrongly took the page load down with it.
8+
*/
9+
import test from "node:test";
10+
import assert from "node:assert/strict";
11+
import dgram from "node:dgram";
12+
13+
import { answerFor, answerPolicy, buildResponse, createServer, encodeName, parseQuery } from "../src/dns.mjs";
14+
15+
const TYPE_A = 1;
16+
const TYPE_MX = 15;
17+
const TYPE_TXT = 16;
18+
const TYPE_AAAA = 28;
19+
const TYPE_HTTPS = 65; // SVCB-compatible; Chrome and Safari send it on every navigation
20+
const RCODE_OK = 0;
21+
const RCODE_NXDOMAIN = 3;
22+
23+
/** A query the way a resolver would send it. */
24+
function query(name, { id = 0x1234, type = TYPE_A, cls = 1, rd = true } = {}) {
25+
const head = Buffer.alloc(12);
26+
head.writeUInt16BE(id, 0);
27+
head.writeUInt16BE(rd ? 0x0100 : 0, 2);
28+
head.writeUInt16BE(1, 4);
29+
const tail = Buffer.alloc(4);
30+
tail.writeUInt16BE(type, 0);
31+
tail.writeUInt16BE(cls, 2);
32+
return Buffer.concat([head, encodeName(name), tail]);
33+
}
34+
35+
const rcode = (reply) => reply.readUInt16BE(2) & 0x0f;
36+
const answers = (reply) => reply.readUInt16BE(6);
37+
38+
/** A registry that always gives the same verdict, counting how often it is asked. */
39+
function registry(body, { ok = true } = {}) {
40+
const calls = [];
41+
const fetchImpl = async (url) => {
42+
calls.push(url);
43+
return { ok, json: async () => body };
44+
};
45+
return { fetchImpl, calls };
46+
}
47+
48+
const parked = () => registry({ name_registered: true, target: null });
49+
const live = (target) => registry({ name_registered: true, target });
50+
const down = () => registry({}, { ok: false });
51+
52+
/** Send one query to a live server and hand back the raw reply. */
53+
async function ask(server, buf) {
54+
const client = dgram.createSocket("udp4");
55+
try {
56+
return await new Promise((resolve, reject) => {
57+
const timer = setTimeout(() => reject(new Error("no reply")), 5000);
58+
client.once("message", (msg) => {
59+
clearTimeout(timer);
60+
resolve(msg);
61+
});
62+
client.send(buf, server.port, "127.0.0.1");
63+
});
64+
} finally {
65+
client.close();
66+
}
67+
}
68+
69+
async function serve(t, { fetchImpl }, extra = {}) {
70+
const server = await createServer({
71+
port: 0,
72+
parkingAddress: "198.51.100.9",
73+
fetchImpl,
74+
...extra,
75+
});
76+
t.after(() => server.close());
77+
return server;
78+
}
79+
80+
/* ------------------------------------------- the questions this bridge does not serve */
81+
82+
for (const [label, type] of [
83+
["HTTPS/SVCB", TYPE_HTTPS],
84+
["TXT", TYPE_TXT],
85+
["MX", TYPE_MX],
86+
]) {
87+
test(`a ${label} question about a parked name is NODATA, not NXDOMAIN`, async (t) => {
88+
const server = await serve(t, parked());
89+
const reply = await ask(server, query("scrambled.eggs", { type }));
90+
assert.equal(rcode(reply), RCODE_OK, `${label} must not deny the name`);
91+
assert.equal(answers(reply), 0, "nothing to answer with");
92+
});
93+
}
94+
95+
test("a HTTPS/SVCB question about a live name is NODATA, not NXDOMAIN", async (t) => {
96+
const server = await serve(t, live("203.0.113.7"));
97+
const reply = await ask(server, query("scrambled.eggs", { type: TYPE_HTTPS }));
98+
assert.equal(rcode(reply), RCODE_OK);
99+
assert.equal(answers(reply), 0);
100+
});
101+
102+
test("the A question beside it still answers, so the pair does not contradict itself", async (t) => {
103+
const server = await serve(t, live("203.0.113.7"));
104+
const https = await ask(server, query("scrambled.eggs", { type: TYPE_HTTPS }));
105+
const a = await ask(server, query("scrambled.eggs", { type: TYPE_A }));
106+
assert.equal(rcode(https), RCODE_OK, "HTTPS must not say the name is gone");
107+
assert.equal(rcode(a), RCODE_OK, "while A says it is here");
108+
assert.equal(answers(a), 1);
109+
});
110+
111+
/* ------------------------------------------------- a name with no address to serve */
112+
113+
test("a live name pointed at a hostname is NODATA, not NXDOMAIN", async (t) => {
114+
// targetAddress() refuses a hostname on purpose — the bridge does not do
115+
// clearnet DNS. The name still exists, so denying it is the wrong answer.
116+
const server = await serve(t, live("example.com"));
117+
const reply = await ask(server, query("scrambled.eggs", { type: TYPE_A }));
118+
assert.equal(rcode(reply), RCODE_OK);
119+
assert.equal(answers(reply), 0);
120+
});
121+
122+
test("a live name whose target names a port is NODATA, not NXDOMAIN", async (t) => {
123+
const server = await serve(t, live("example.com:8080"));
124+
const reply = await ask(server, query("scrambled.eggs", { type: TYPE_A }));
125+
assert.equal(rcode(reply), RCODE_OK);
126+
assert.equal(answers(reply), 0);
127+
});
128+
129+
/* ------------------------------------------------------- NXDOMAIN is still reachable */
130+
131+
test("a name the registry does not know is still NXDOMAIN, on every type", async (t) => {
132+
const server = await serve(t, down());
133+
for (const type of [TYPE_A, TYPE_AAAA, TYPE_TXT, TYPE_MX, TYPE_HTTPS]) {
134+
const reply = await ask(server, query("scrambled.eggs", { type }));
135+
assert.equal(rcode(reply), RCODE_NXDOMAIN, `type ${type} must still deny an unknown name`);
136+
assert.equal(answers(reply), 0);
137+
}
138+
});
139+
140+
test("a name the registry cannot hold is still NXDOMAIN, on every type", async (t) => {
141+
const reg = parked();
142+
const server = await serve(t, reg);
143+
for (const type of [TYPE_A, TYPE_HTTPS]) {
144+
const reply = await ask(server, query("deep.sub.eggs", { type }));
145+
assert.equal(rcode(reply), RCODE_NXDOMAIN, `type ${type} on a non-name`);
146+
}
147+
assert.equal(reg.calls.length, 0, "a shape the registry cannot hold is never looked up");
148+
});
149+
150+
test("a question in another class is still NXDOMAIN and is never looked up", async (t) => {
151+
const reg = parked();
152+
const server = await serve(t, reg);
153+
const reply = await ask(server, query("scrambled.eggs", { type: TYPE_A, cls: 3 }));
154+
assert.equal(rcode(reply), RCODE_NXDOMAIN);
155+
assert.equal(reg.calls.length, 0, "CHAOS is not ours to answer");
156+
});
157+
158+
/* ---------------------------------------------------------------- lookup accounting */
159+
160+
test("a question we do not serve costs exactly one lookup, not zero and not several", async (t) => {
161+
// Counted rather than inferred from the reply: a status can be right by
162+
// accident, a call count cannot.
163+
const reg = parked();
164+
const server = await serve(t, reg);
165+
await ask(server, query("scrambled.eggs", { type: TYPE_HTTPS }));
166+
assert.equal(reg.calls.length, 1);
167+
});
168+
169+
test("a malformed query is still never answered and never looked up", async (t) => {
170+
const reg = parked();
171+
const server = await serve(t, reg);
172+
const client = dgram.createSocket("udp4");
173+
t.after(() => client.close());
174+
let replied = false;
175+
client.on("message", () => {
176+
replied = true;
177+
});
178+
client.send(Buffer.alloc(4), server.port, "127.0.0.1");
179+
await new Promise((r) => setTimeout(r, 250));
180+
assert.equal(replied, false, "say nothing at all");
181+
assert.equal(reg.calls.length, 0);
182+
});
183+
184+
/* --------------------------------------------------- the answers that already worked */
185+
186+
test("a parked name still answers A with the parking address", async (t) => {
187+
const server = await serve(t, parked());
188+
const reply = await ask(server, query("scrambled.eggs"));
189+
assert.equal(rcode(reply), RCODE_OK);
190+
assert.equal(answers(reply), 1);
191+
assert.deepEqual([...reply.subarray(reply.length - 4)], [198, 51, 100, 9]);
192+
});
193+
194+
test("a parked name still answers AAAA with NODATA", async (t) => {
195+
const server = await serve(t, parked());
196+
const reply = await ask(server, query("scrambled.eggs", { type: TYPE_AAAA }));
197+
assert.equal(rcode(reply), RCODE_OK);
198+
assert.equal(answers(reply), 0);
199+
});
200+
201+
test("a live IPv6 target still answers AAAA with sixteen bytes and NODATA on A", async (t) => {
202+
const server = await serve(t, live("2606:4700:4700::1111"));
203+
const v6 = await ask(server, query("scrambled.eggs", { type: TYPE_AAAA }));
204+
assert.equal(answers(v6), 1);
205+
assert.equal(v6.length - v6.indexOf(v6.subarray(v6.length - 16)), 16);
206+
const v4 = await ask(server, query("scrambled.eggs", { type: TYPE_A }));
207+
assert.equal(rcode(v4), RCODE_OK, "the family it does not have is NODATA, as before");
208+
assert.equal(answers(v4), 0);
209+
});
210+
211+
/* ------------------------------------------------------------ the units underneath */
212+
213+
test("buildResponse keeps its old meaning when it is not told whether the name exists", () => {
214+
// Every existing caller passes four arguments. Those replies must not move.
215+
for (const [name, type, address] of [
216+
["scrambled.eggs", TYPE_A, null],
217+
["scrambled.eggs", TYPE_A, "203.0.113.7"],
218+
["scrambled.eggs", TYPE_AAAA, "203.0.113.7"],
219+
["scrambled.eggs", TYPE_AAAA, "2606:4700:4700::1111"],
220+
]) {
221+
const buf = query(name, { type });
222+
const q = parseQuery(buf);
223+
assert.deepEqual(
224+
buildResponse(q, buf, address, 30),
225+
buildResponse(q, buf, address, 30, Boolean(address)),
226+
`four-argument form unchanged for ${type}/${address}`,
227+
);
228+
}
229+
});
230+
231+
test("buildResponse says NODATA for a name that exists with no address", () => {
232+
const buf = query("scrambled.eggs", { type: TYPE_HTTPS });
233+
const q = parseQuery(buf);
234+
assert.equal(rcode(buildResponse(q, buf, null, 30, true)), RCODE_OK);
235+
assert.equal(answers(buildResponse(q, buf, null, 30, true)), 0);
236+
assert.equal(rcode(buildResponse(q, buf, null, 30, false)), RCODE_NXDOMAIN);
237+
});
238+
239+
test("answerPolicy reports a parked name as here even when no address is wanted", async () => {
240+
const { fetchImpl } = parked();
241+
assert.deepEqual(await answerPolicy("scrambled.eggs", { fetchImpl, parkingAddress: "198.51.100.9" }), {
242+
exists: true,
243+
address: "198.51.100.9",
244+
});
245+
assert.deepEqual(
246+
await answerPolicy("scrambled.eggs", {
247+
fetchImpl,
248+
parkingAddress: "198.51.100.9",
249+
wantsAddress: false,
250+
}),
251+
{ exists: true, address: null },
252+
);
253+
});
254+
255+
test("answerPolicy reports a name it could not look up as not here", async () => {
256+
const { fetchImpl } = down();
257+
for (const wantsAddress of [true, false]) {
258+
assert.deepEqual(
259+
await answerPolicy("scrambled.eggs", { fetchImpl, parkingAddress: "198.51.100.9", wantsAddress }),
260+
{ exists: false, address: null },
261+
);
262+
}
263+
});
264+
265+
test("answerFor still returns exactly what it used to", async () => {
266+
assert.equal(
267+
await answerFor("scrambled.eggs", { fetchImpl: parked().fetchImpl, parkingAddress: "198.51.100.9" }),
268+
"198.51.100.9",
269+
"parked → the parking address",
270+
);
271+
assert.equal(
272+
await answerFor("scrambled.eggs", { fetchImpl: live("203.0.113.7").fetchImpl }),
273+
"203.0.113.7",
274+
"live → its address",
275+
);
276+
assert.equal(
277+
await answerFor("scrambled.eggs", { fetchImpl: live("example.com").fetchImpl }),
278+
null,
279+
"a hostname target has no address",
280+
);
281+
assert.equal(
282+
await answerFor("scrambled.eggs", { fetchImpl: down().fetchImpl, parkingAddress: "198.51.100.9" }),
283+
null,
284+
"a registry outage does not park every name",
285+
);
286+
assert.equal(await answerFor("deep.sub.eggs", { fetchImpl: parked().fetchImpl }), null, "not a name");
287+
});
288+
289+
test("onQuery still reports no address for the questions we do not serve", async (t) => {
290+
const seen = [];
291+
const server = await serve(t, parked(), { onQuery: (q) => seen.push(q) });
292+
await ask(server, query("scrambled.eggs", { type: TYPE_HTTPS }));
293+
await ask(server, query("scrambled.eggs", { type: TYPE_A }));
294+
assert.deepEqual(seen, [
295+
{ name: "scrambled.eggs", type: TYPE_HTTPS, address: null },
296+
{ name: "scrambled.eggs", type: TYPE_A, address: "198.51.100.9" },
297+
]);
298+
});

0 commit comments

Comments
 (0)